From 92890583627ee2a0518e55b063fcff86826fef96 Mon Sep 17 00:00:00 2001 From: Matt Roper Date: Mon, 19 Jan 2015 08:31:49 -0800 Subject: drm/plane-helper: Skip prepare_fb/cleanup_fb when newfb==oldfb When commiting a plane update where the framebuffer doesn't change, we can skip the prepare_fb/cleanup_fb steps. This also allows us to avoid an unnecessary vblank wait at the end of the operation when we're just moving a plane and not changing its image (e.g., for a cursor). At the moment, i915 is the only upstream driver using the transitional plane helpers, and thus the only driver affected by this change. Note that this replicates a corresponding change in the atomic helpers implemented in commit ab58e3384b9f9863bfd029b458ff337d381bf6d2 Author: Daniel Vetter Date: Mon Nov 24 20:42:42 2014 +0100 drm/atomic-helper: Skip vblank waits for unchanged fbs Reported-by: Jeremiah Mahler Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88540 Signed-off-by: Matt Roper Tested-by: Tested-by: Rodrigo Vivi Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_plane_helper.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index f24c4cfe674b..7a5814afe31a 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -435,7 +435,8 @@ int drm_plane_helper_commit(struct drm_plane *plane, goto out; } - if (plane_funcs->prepare_fb && plane_state->fb) { + if (plane_funcs->prepare_fb && plane_state->fb && + plane_state->fb != old_fb) { ret = plane_funcs->prepare_fb(plane, plane_state->fb); if (ret) goto out; @@ -456,6 +457,13 @@ int drm_plane_helper_commit(struct drm_plane *plane, crtc_funcs[i]->atomic_flush(crtc[i]); } + /* + * If we only moved the plane and didn't change fb's, there's no need to + * wait for vblank. + */ + if (plane->state->fb == old_fb) + goto out; + for (i = 0; i < 2; i++) { if (!crtc[i]) continue; -- cgit From 6a425c2a9b37ca3d2c37e3c1cdf973dba53eaa79 Mon Sep 17 00:00:00 2001 From: Matt Roper Date: Thu, 15 Jan 2015 18:34:22 -0800 Subject: drm/plane-helper: Fix transitional helper kerneldocs drm_plane_helper_{update,disable} are not specific to primary planes; fix some copy/paste summaries to avoid confusion. Cc: dri-devel@lists.freedesktop.org Signed-off-by: Matt Roper Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_plane_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 7a5814afe31a..510fe03a1fc1 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -492,7 +492,7 @@ out: } /** - * drm_plane_helper_update() - Helper for primary plane update + * drm_plane_helper_update() - Transitional helper for plane update * @plane: plane object to update * @crtc: owning CRTC of owning plane * @fb: framebuffer to flip onto plane @@ -549,7 +549,7 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, EXPORT_SYMBOL(drm_plane_helper_update); /** - * drm_plane_helper_disable() - Helper for primary plane disable + * drm_plane_helper_disable() - Transitional helper for plane disable * @plane: plane to disable * * Provides a default plane disable handler using the atomic plane update -- cgit From 960cd9d4fef6dd9e235c0e5c0d4ed027f8a48025 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Wed, 21 Jan 2015 08:47:38 +0100 Subject: drm: Add standardized boolean props Not a new type exposed to userspace, just a standard way to create them since between range, bitmask and enum there's 3 different ways to pull out a boolean prop. Also add the kerneldoc for the recently added new prop types, which Rob forgot all about. v2: Fixup kerneldoc, spotted by Rob. Cc: Rob Clark Reviewed-by: Rob Clark Reviewed-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 66 +++++++++++++++++++++++++++++++++++++++++++--- include/drm/drm_crtc.h | 2 ++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index df90048de92e..c0bbb00beba7 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3810,7 +3810,7 @@ static struct drm_property *property_create_range(struct drm_device *dev, } /** - * drm_property_create_range - create a new ranged property type + * drm_property_create_range - create a new unsigned ranged property type * @dev: drm device * @flags: flags specifying the property type * @name: name of the property @@ -3821,8 +3821,8 @@ static struct drm_property *property_create_range(struct drm_device *dev, * object with drm_object_attach_property. The returned property object must be * freed with drm_property_destroy. * - * Userspace is allowed to set any integer value in the (min, max) range - * inclusive. + * Userspace is allowed to set any unsigned integer value in the (min, max) + * range inclusive. * * Returns: * A pointer to the newly created property on success, NULL on failure. @@ -3836,6 +3836,24 @@ struct drm_property *drm_property_create_range(struct drm_device *dev, int flags } EXPORT_SYMBOL(drm_property_create_range); +/** + * drm_property_create_signed_range - create a new signed ranged property type + * @dev: drm device + * @flags: flags specifying the property type + * @name: name of the property + * @min: minimum value of the property + * @max: maximum value of the property + * + * This creates a new generic drm property which can then be attached to a drm + * object with drm_object_attach_property. The returned property object must be + * freed with drm_property_destroy. + * + * Userspace is allowed to set any signed integer value in the (min, max) + * range inclusive. + * + * Returns: + * A pointer to the newly created property on success, NULL on failure. + */ struct drm_property *drm_property_create_signed_range(struct drm_device *dev, int flags, const char *name, int64_t min, int64_t max) @@ -3845,6 +3863,23 @@ struct drm_property *drm_property_create_signed_range(struct drm_device *dev, } EXPORT_SYMBOL(drm_property_create_signed_range); +/** + * drm_property_create_object - create a new object property type + * @dev: drm device + * @flags: flags specifying the property type + * @name: name of the property + * @type: object type from DRM_MODE_OBJECT_* defines + * + * This creates a new generic drm property which can then be attached to a drm + * object with drm_object_attach_property. The returned property object must be + * freed with drm_property_destroy. + * + * Userspace is only allowed to set this to any property value of the given + * @type. Only useful for atomic properties, which is enforced. + * + * Returns: + * A pointer to the newly created property on success, NULL on failure. + */ struct drm_property *drm_property_create_object(struct drm_device *dev, int flags, const char *name, uint32_t type) { @@ -3852,6 +3887,9 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, flags |= DRM_MODE_PROP_OBJECT; + if (WARN_ON(!(flags & DRM_MODE_PROP_ATOMIC))) + return NULL; + property = drm_property_create(dev, flags, name, 1); if (!property) return NULL; @@ -3862,6 +3900,28 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, } EXPORT_SYMBOL(drm_property_create_object); +/** + * drm_property_create_bool - create a new boolean property type + * @dev: drm device + * @flags: flags specifying the property type + * @name: name of the property + * + * This creates a new generic drm property which can then be attached to a drm + * object with drm_object_attach_property. The returned property object must be + * freed with drm_property_destroy. + * + * This is implemented as a ranged property with only {0, 1} as valid values. + * + * Returns: + * A pointer to the newly created property on success, NULL on failure. + */ +struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, + const char *name) +{ + return drm_property_create_range(dev, flags, name, 0, 1); +} +EXPORT_SYMBOL(drm_property_create_bool); + /** * drm_property_add_enum - add a possible value to an enumeration property * @property: enumeration property to change diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 0ecfb7c80601..c4e36f60b3ef 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1345,6 +1345,8 @@ struct drm_property *drm_property_create_signed_range(struct drm_device *dev, int64_t min, int64_t max); struct drm_property *drm_property_create_object(struct drm_device *dev, int flags, const char *name, uint32_t type); +struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, + const char *name); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name); -- cgit From eab3bbeffd152125ae0f90863b8e9bc8eef49423 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 22 Jan 2015 16:36:21 +0100 Subject: drm/atomic: Add drm_crtc_state->active This is the infrastructure for DPMS ported to the atomic world. Fundamental changes compare to legacy DPMS are: - No more per-connector dpms state, instead there's just one per each display pipeline. So if you clone either you have to unclone first if you only want to switch off one screen, or you just switch of everything (like all desktops do). This massively reduces complexity for cloning since now there's no more half-enabled cloned configs to consider. - Only on/off, dpms standby/suspend are as dead as real CRTs. Again reduces complexity a lot. Now especially for backwards compat the really important part for dpms support is that dpms on always succeeds (except for hw death and unplugged cables ofc). Which means everything that could fail (like configuration checking, resources assignments and buffer management) must be done irrespective from ->active. ->active is really only a toggle to change the hardware state. More precisely: - Drivers MUST NOT look at ->active in their ->atomic_check callbacks. Changes to ->active MUST always suceed if nothing else changes. - Drivers using the atomic helpers MUST NOT look at ->active anywhere, period. The helpers will take care of calling the respective enable/modeset/disable hooks as necessary. As before the helpers will carefully keep track of the state and not call any hooks unecessarily, so still no double-disables or enables like with crtc helpers. - ->mode_set hooks are only called when the mode or output configuration changes, not for changes in ->active state. - Drivers which reconstruct the state objects in their ->reset hooks or through some other hw state readout infrastructure must ensure that ->active reflects actual hw state. This just implements the core bits and helper logic, a subsequent patch will implement the helper code to implement legacy dpms with this. v2: Rebase on top of the drm ioctl work: - Move crtc checks to the core check function. - Also check for ->active_changed when deciding whether a modeset might happen (for the ALLOW_MODESET mode). - Expose the ->active state with an atomic prop. v3: Review from Rob - Spelling fix in comment. - Extract needs_modeset helper to consolidate the ->mode_changed || ->active_changed checks. v4: Fixup fumble between crtc->state and crtc_state. Cc: Rob Clark Reviewed-by: Thierry Reding Tested-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic.c | 18 +++++++++++-- drivers/gpu/drm/drm_atomic_helper.c | 54 +++++++++++++++++++++++++++++++++---- drivers/gpu/drm/drm_crtc.c | 10 +++++++ include/drm/drm_crtc.h | 3 +++ 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1e38dfc8e462..ee68267bb326 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -241,7 +241,13 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, struct drm_crtc_state *state, struct drm_property *property, uint64_t val) { - if (crtc->funcs->atomic_set_property) + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + + /* FIXME: Mode prop is missing, which also controls ->enable. */ + if (property == config->prop_active) { + state->active = val; + } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); return -EINVAL; } @@ -282,6 +288,13 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, * * TODO: Add generic modeset state checks once we support those. */ + + if (state->active && !state->enable) { + DRM_DEBUG_KMS("[CRTC:%d] active without enabled\n", + crtc->base.id); + return -EINVAL; + } + return 0; } @@ -976,7 +989,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) if (!crtc) continue; - if (crtc_state->mode_changed) { + if (crtc_state->mode_changed || + crtc_state->active_changed) { DRM_DEBUG_KMS("[CRTC:%d] requires full modeset\n", crtc->base.id); return -EINVAL; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 541ba833ed36..3f17933b1d2c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -330,6 +330,12 @@ mode_fixup(struct drm_atomic_state *state) return 0; } +static bool +needs_modeset(struct drm_crtc_state *state) +{ + return state->mode_changed || state->active_changed; +} + /** * drm_atomic_helper_check - validate state object for modeset changes * @dev: DRM device @@ -404,12 +410,27 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, crtc = state->crtcs[i]; crtc_state = state->crtc_states[i]; - if (!crtc || !crtc_state->mode_changed) + if (!crtc) continue; - DRM_DEBUG_KMS("[CRTC:%d] needs full modeset, enable: %c\n", + /* + * We must set ->active_changed after walking connectors for + * otherwise an update that only changes active would result in + * a full modeset because update_connector_routing force that. + */ + if (crtc->state->active != crtc_state->active) { + DRM_DEBUG_KMS("[CRTC:%d] active changed\n", + crtc->base.id); + crtc_state->active_changed = true; + } + + if (!needs_modeset(crtc_state)) + continue; + + DRM_DEBUG_KMS("[CRTC:%d] needs all connectors, enable: %c, active: %c\n", crtc->base.id, - crtc_state->enable ? 'y' : 'n'); + crtc_state->enable ? 'y' : 'n', + crtc_state->active ? 'y' : 'n'); ret = drm_atomic_add_affected_connectors(state, crtc); if (ret != 0) @@ -545,6 +566,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) struct drm_connector *connector; struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder; + struct drm_crtc_state *old_crtc_state; old_conn_state = old_state->connector_states[i]; connector = old_state->connectors[i]; @@ -554,6 +576,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (!old_conn_state || !old_conn_state->crtc) continue; + old_crtc_state = old_state->crtc_states[drm_crtc_index(old_conn_state->crtc)]; + + if (!old_crtc_state->active) + continue; + encoder = old_conn_state->best_encoder; /* We shouldn't get this far if we didn't previously have @@ -586,11 +613,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) for (i = 0; i < ncrtcs; i++) { struct drm_crtc_helper_funcs *funcs; struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; crtc = old_state->crtcs[i]; + old_crtc_state = old_state->crtc_states[i]; /* Shut down everything that needs a full modeset. */ - if (!crtc || !crtc->state->mode_changed) + if (!crtc || !needs_modeset(crtc->state)) + continue; + + if (!old_crtc_state->active) continue; funcs = crtc->helper_private; @@ -697,6 +729,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) mode = &new_crtc_state->mode; adjusted_mode = &new_crtc_state->adjusted_mode; + if (!new_crtc_state->mode_changed) + continue; + /* * Each encoder has at most one connector (since we always steal * it away), so we won't call call mode_set hooks twice. @@ -749,7 +784,10 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, crtc = old_state->crtcs[i]; /* Need to filter out CRTCs where only planes change. */ - if (!crtc || !crtc->state->mode_changed) + if (!crtc || !needs_modeset(crtc->state)) + continue; + + if (!crtc->state->active) continue; funcs = crtc->helper_private; @@ -768,6 +806,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, if (!connector || !connector->state->best_encoder) continue; + if (!connector->state->crtc->state->active) + continue; + encoder = connector->state->best_encoder; funcs = encoder->helper_private; @@ -1518,6 +1559,7 @@ retry: WARN_ON(set->num_connectors); crtc_state->enable = false; + crtc_state->active = false; ret = drm_atomic_set_crtc_for_plane(primary_state, NULL); if (ret != 0) @@ -1532,6 +1574,7 @@ retry: WARN_ON(!set->num_connectors); crtc_state->enable = true; + crtc_state->active = true; drm_mode_copy(&crtc_state->mode, set->mode); ret = drm_atomic_set_crtc_for_plane(primary_state, crtc); @@ -1894,6 +1937,7 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc) if (state) { state->mode_changed = false; + state->active_changed = false; state->planes_changed = false; state->event = NULL; } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index c0bbb00beba7..419f9d915c78 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -691,6 +691,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (cursor) cursor->possible_crtcs = 1 << drm_crtc_index(crtc); + if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { + drm_object_attach_property(&crtc->base, config->prop_active, 0); + } + return 0; } EXPORT_SYMBOL(drm_crtc_init_with_planes); @@ -1481,6 +1485,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_crtc_id = prop; + prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC, + "ACTIVE"); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_active = prop; + return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c4e36f60b3ef..0a738e1d4f37 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -253,6 +253,7 @@ struct drm_atomic_state; * @enable: whether the CRTC should be enabled, gates all other state * @active: whether the CRTC is actively displaying (used for DPMS) * @mode_changed: for use by helpers and drivers when computing state updates + * @active_changed: for use by helpers and drivers when computing state updates * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes * @last_vblank_count: for helpers and drivers to capture the vblank of the * update to ensure framebuffer cleanup isn't done too early @@ -278,6 +279,7 @@ struct drm_crtc_state { /* computed state bits used by helpers and drivers */ bool planes_changed : 1; bool mode_changed : 1; + bool active_changed : 1; /* attached planes bitmask: * WARNING: transitional helpers do not maintain plane_mask so @@ -1113,6 +1115,7 @@ struct drm_mode_config { struct drm_property *prop_crtc_h; struct drm_property *prop_fb_id; struct drm_property *prop_crtc_id; + struct drm_property *prop_active; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; -- cgit From b486e0e6d599b9ca8667fb9a7d49b7383ee963c7 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 22 Jan 2015 18:53:05 +0100 Subject: drm/atomic-helper: add connector->dpms() implementation This builds on top of the crtc->active infrastructure to implement legacy DPMS. My choice of semantics is somewhat arbitrary, but the entire pipe is enabled as along as one output is still enabled. Of course it also clamps everything that's not ON to OFF. v2: Fix spelling in one comment. v3: Don't do an async commit (Thierry) v4: Dan Carpenter noticed missing error case handling. Cc: Thierry Reding Reviewed-by: Thierry Reding Tested-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 2 + 2 files changed, 79 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3f17933b1d2c..6112ec261c3b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1886,6 +1886,83 @@ backoff: } EXPORT_SYMBOL(drm_atomic_helper_page_flip); +/** + * drm_atomic_helper_connector_dpms() - connector dpms helper implementation + * @connector: affected connector + * @mode: DPMS mode + * + * This is the main helper function provided by the atomic helper framework for + * implementing the legacy DPMS connector interface. It computes the new desired + * ->active state for the corresponding CRTC (if the connector is enabled) and + * updates it. + */ +void drm_atomic_helper_connector_dpms(struct drm_connector *connector, + int mode) +{ + struct drm_mode_config *config = &connector->dev->mode_config; + struct drm_atomic_state *state; + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + struct drm_connector *tmp_connector; + int ret; + bool active = false; + + if (mode != DRM_MODE_DPMS_ON) + mode = DRM_MODE_DPMS_OFF; + + connector->dpms = mode; + crtc = connector->state->crtc; + + if (!crtc) + return; + + /* FIXME: ->dpms has no return value so can't forward the -ENOMEM. */ + state = drm_atomic_state_alloc(connector->dev); + if (!state) + return; + + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); +retry: + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return; + + WARN_ON(!drm_modeset_is_locked(&config->connection_mutex)); + + list_for_each_entry(tmp_connector, &config->connector_list, head) { + if (connector->state->crtc != crtc) + continue; + + if (connector->dpms == DRM_MODE_DPMS_ON) { + active = true; + break; + } + } + crtc_state->active = active; + + ret = drm_atomic_commit(state); + if (ret != 0) + goto fail; + + /* Driver takes ownership of state on successful async commit. */ + return; +fail: + if (ret == -EDEADLK) + goto backoff; + + drm_atomic_state_free(state); + + WARN(1, "Driver bug: Changing ->active failed with ret=%i\n", ret); + + return; +backoff: + drm_atomic_state_clear(state); + drm_atomic_legacy_backoff(state); + + goto retry; +} +EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); + /** * DOC: atomic state reset and initialization * diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 2095917ff8c7..cf501df9e513 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -82,6 +82,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t flags); +void drm_atomic_helper_connector_dpms(struct drm_connector *connector, + int mode); /* default implementations for state handling */ void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc); -- cgit From f02ad907cd9e7fe3a6405d2d005840912f1ed258 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 22 Jan 2015 16:36:23 +0100 Subject: drm/atomic-helpers: Recover full cursor plane behaviour Cursor plane updates have historically been fully async and mutliple updates batched together for the next vsync. And userspace relies upon that. Since implementing a full queue of async atomic updates is a bit of work lets just recover the cursor specific behaviour with a hint flag and some hacks to drop the vblank wait. v2: Fix kerneldoc, reported by Wu Fengguang. Reviewed-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 13 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6112ec261c3b..d0c3611402da 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -909,6 +909,11 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, if (!crtc->state->enable) continue; + /* Legacy cursor ioctls are completely unsynced, and userspace + * relies on that (by doing tons of cursor updates). */ + if (old_state->legacy_cursor_update) + continue; + if (!framebuffer_changed(dev, old_state, crtc)) continue; @@ -1335,6 +1340,9 @@ retry: if (ret != 0) goto fail; + if (plane == crtc->cursor) + state->legacy_cursor_update = true; + /* Driver takes ownership of state on successful commit. */ return 0; fail: @@ -1410,6 +1418,9 @@ retry: plane_state->src_h = 0; plane_state->src_w = 0; + if (plane == plane->crtc->cursor) + state->legacy_cursor_update = true; + ret = drm_atomic_commit(state); if (ret != 0) goto fail; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 0a738e1d4f37..019c9b562144 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -909,6 +909,7 @@ struct drm_bridge { * struct struct drm_atomic_state - the global state object for atomic updates * @dev: parent DRM device * @allow_modeset: allow full modeset + * @legacy_cursor_update: hint to enforce legacy cursor ioctl semantics * @planes: pointer to array of plane pointers * @plane_states: pointer to array of plane states pointers * @crtcs: pointer to array of CRTC pointers @@ -921,6 +922,7 @@ struct drm_bridge { struct drm_atomic_state { struct drm_device *dev; bool allow_modeset : 1; + bool legacy_cursor_update : 1; struct drm_plane **planes; struct drm_plane_state **plane_states; struct drm_crtc **crtcs; -- cgit From ee0a89cf3c2c550e6d877dda21dd2947afb90cb6 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 22 Jan 2015 16:36:24 +0100 Subject: drm/atomic-helpers: Saner encoder/crtc callbacks For historical reasons going all the way back to how the Xrandr code was implemented the semantics of the callbacks used to enable/disable crtcs and encoders are ... interesting. But with atomic helpers all that complexity has been binned, with only a well-defined on/off action left. Unfortunately the names stuck. Let's fix that by adding enable/disable hooks every, make them the preferred variant for atomic and update documentations. Later on we add debug warnings when drivers have deprecated hooks. But while everything is in-flight with lots of drivers converting to atomic that's a bit too much - better wait for things to settle a bit first. v2: Fix kerneldoc, reported by Wu Fengguang. Reviewed-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++----- include/drm/drm_crtc_helper.h | 20 ++++++++++++++++++-- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d0c3611402da..379b6c833bb2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -599,7 +599,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) encoder->bridge->funcs->disable(encoder->bridge); /* Right function depends upon target state. */ - if (connector->state->crtc) + if (connector->state->crtc && funcs->prepare) funcs->prepare(encoder); else if (funcs->disable) funcs->disable(encoder); @@ -628,7 +628,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs = crtc->helper_private; /* Right function depends upon target state. */ - if (crtc->state->enable) + if (crtc->state->enable && funcs->prepare) funcs->prepare(crtc); else if (funcs->disable) funcs->disable(crtc); @@ -792,8 +792,12 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, funcs = crtc->helper_private; - if (crtc->state->enable) - funcs->commit(crtc); + if (crtc->state->enable) { + if (funcs->enable) + funcs->enable(crtc); + else + funcs->commit(crtc); + } } for (i = 0; i < old_state->num_connector; i++) { @@ -819,7 +823,10 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, if (encoder->bridge) encoder->bridge->funcs->pre_enable(encoder->bridge); - funcs->commit(encoder); + if (funcs->enable) + funcs->enable(encoder); + else + funcs->commit(encoder); if (encoder->bridge) encoder->bridge->funcs->enable(encoder->bridge); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index e76828d81a8b..4b19f7a20d62 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -58,11 +58,19 @@ enum mode_set_atomic { * @mode_set_base_atomic: non-blocking mode set (used for kgdb support) * @load_lut: load color palette * @disable: disable CRTC when no longer in use + * @enable: enable CRTC * @atomic_check: check for validity of an atomic state * @atomic_begin: begin atomic update * @atomic_flush: flush atomic update * * The helper operations are called by the mid-layer CRTC helper. + * + * Note that with atomic helpers @dpms, @prepare and @commit hooks are + * deprecated. Used @enable and @disable instead exclusively. + * + * With legacy crtc helpers there's a big semantic difference between @disable + * and the other hooks: @disable also needs to release any resources acquired in + * @mode_set (like shared PLLs). */ struct drm_crtc_helper_funcs { /* @@ -93,8 +101,8 @@ struct drm_crtc_helper_funcs { /* reload the current crtc LUT */ void (*load_lut)(struct drm_crtc *crtc); - /* disable crtc when not in use - more explicit than dpms off */ void (*disable)(struct drm_crtc *crtc); + void (*enable)(struct drm_crtc *crtc); /* atomic helpers */ int (*atomic_check)(struct drm_crtc *crtc, @@ -115,8 +123,16 @@ struct drm_crtc_helper_funcs { * @get_crtc: return CRTC that the encoder is currently attached to * @detect: connection status detection * @disable: disable encoder when not in use (overrides DPMS off) + * @enable: enable encoder * * The helper operations are called by the mid-layer CRTC helper. + * + * Note that with atomic helpers @dpms, @prepare and @commit hooks are + * deprecated. Used @enable and @disable instead exclusively. + * + * With legacy crtc helpers there's a big semantic difference between @disable + * and the other hooks: @disable also needs to release any resources acquired in + * @mode_set (like shared PLLs). */ struct drm_encoder_helper_funcs { void (*dpms)(struct drm_encoder *encoder, int mode); @@ -135,8 +151,8 @@ struct drm_encoder_helper_funcs { /* detect for DAC style encoders */ enum drm_connector_status (*detect)(struct drm_encoder *encoder, struct drm_connector *connector); - /* disable encoder when not in use - more explicit than dpms off */ void (*disable)(struct drm_encoder *encoder); + void (*enable)(struct drm_encoder *encoder); }; /** -- cgit From 95d6eb3b134e1826ed04cc92b224d93de13e281f Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 22 Jan 2015 16:36:25 +0100 Subject: drm/atomic-helper: debug output for modesets With the combination of ->enable and ->active it's a bit complicated to follow what exactly is going on sometimes within a full modeset. Add debug output to make this all traceable. Reviewed-by: Thierry Reding Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 379b6c833bb2..6efb94116237 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -591,6 +591,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs = encoder->helper_private; + DRM_DEBUG_KMS("disabling [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); + /* * Each encoder has at most one connector (since we always steal * it away), so we won't call call disable hooks twice. @@ -627,6 +630,10 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs = crtc->helper_private; + DRM_DEBUG_KMS("disabling [CRTC:%d]\n", + crtc->base.id); + + /* Right function depends upon target state. */ if (crtc->state->enable && funcs->prepare) funcs->prepare(crtc); @@ -707,8 +714,12 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) funcs = crtc->helper_private; - if (crtc->state->enable) + if (crtc->state->enable) { + DRM_DEBUG_KMS("modeset on [CRTC:%d]\n", + crtc->base.id); + funcs->mode_set_nofb(crtc); + } } for (i = 0; i < old_state->num_connector; i++) { @@ -732,6 +743,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) if (!new_crtc_state->mode_changed) continue; + DRM_DEBUG_KMS("modeset on [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); + /* * Each encoder has at most one connector (since we always steal * it away), so we won't call call mode_set hooks twice. @@ -793,6 +807,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, funcs = crtc->helper_private; if (crtc->state->enable) { + DRM_DEBUG_KMS("enabling [CRTC:%d]\n", + crtc->base.id); + if (funcs->enable) funcs->enable(crtc); else @@ -816,6 +833,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, encoder = connector->state->best_encoder; funcs = encoder->helper_private; + DRM_DEBUG_KMS("enabling [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); + /* * Each encoder has at most one connector (since we always steal * it away), so we won't call call enable hooks twice. -- cgit From 9469244d869623e8b54d9f3d4d00737e377af273 Mon Sep 17 00:00:00 2001 From: Ander Conselvan de Oliveira Date: Fri, 23 Jan 2015 09:27:59 +0200 Subject: drm/atomic: Fix potential use of state after free The atomic helpers rely on drm_atomic_state_clear() to reset an atomic state if a retry is needed due to the w/w mutexes. The subsequent calls to drm_atomic_get_{crtc,plane,...}_state() would then return the stale pointers in state->{crtc,plane,...}_states. Signed-off-by: Ander Conselvan de Oliveira Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ee68267bb326..dab838b02fb7 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -134,6 +134,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state) connector->funcs->atomic_destroy_state(connector, state->connector_states[i]); + state->connector_states[i] = NULL; } for (i = 0; i < config->num_crtc; i++) { @@ -144,6 +145,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state) crtc->funcs->atomic_destroy_state(crtc, state->crtc_states[i]); + state->crtc_states[i] = NULL; } for (i = 0; i < config->num_total_plane; i++) { @@ -154,6 +156,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state) plane->funcs->atomic_destroy_state(plane, state->plane_states[i]); + state->plane_states[i] = NULL; } } EXPORT_SYMBOL(drm_atomic_state_clear); -- cgit