On Thu, Jan 22, 2015 at 04:36:21PM +0100, Daniel Vetter wrote:
> 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.
So we now have multiple conflicting properties for the same thing? I
don't particularly relish that idea.
I suppose a rather easy way to way to deal with that would be to hide
the dpms properties from non-atomic clients, and conversly hide the
active property from legacy clients.
>
> 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 <robdclark@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> 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 56e3256d5249..30a136b8a4fc 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);
> @@ -1391,6 +1395,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 8022ab11958a..4d3f3b874dd6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -249,6 +249,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
> @@ -274,6 +275,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
> @@ -1128,6 +1130,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;
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
