[Intel-gfx] [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

Ville Syrjälä ville.syrjala at linux.intel.com
Thu Jan 22 07:56:54 PST 2015


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 at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC


More information about the Intel-gfx mailing list