[Intel-gfx] [PATCH 1/2] drm/i915: Use enabled value from crtc_state rather than crtc (v2)

Daniel Vetter daniel at ffwll.ch
Wed Feb 25 15:14:54 PST 2015


On Wed, Feb 25, 2015 at 01:12:16PM -0800, Matt Roper wrote:
> As vendors transition their drivers from legacy to atomic there's some
> duplication of data between drm_crtc and drm_crtc_state (since
> unconverted drivers likely won't have a state structure).
> 
> i915 is partially converted and does have a crtc->state structure, but
> still uses direct crtc fields internally in many places, which causes
> the two sets of data to get out of sync.  As of commit
> 
>         commit 31c946e85ce6b48ce0f25e3cdca8362e4fe8b300
>         Author: Daniel Vetter <daniel.vetter at ffwll.ch>
>         Date:   Sun Feb 22 12:24:17 2015 +0100
> 
>             drm: If available use atomic state in getcrtc ioctl
> 
>             This way drivers fully converted to atomic don't need to update these
>             legacy state variables in their modeset code any more.
> 
>             Reviewed-by: Rob Clark <robdclark at gmail.com>
>             Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> the DRM core starts assuming that the presence of a ->state structure
> implies that it should make use of the values stored there which, on
> i915, leads to the core code using stale values for CRTC 'enabled'
> status.
> 
> Let's switch over to using the state value of 'enable' internally rather
> than using the drm_crtc field.  This ensures that our driver internals
> are working from the same data that the DRM core is, avoiding
> mismatches.
> 
> This patch was generated with Coccinelle using the following semantic
> patch:
> 
>         <smpl>
>         @@
>         struct drm_crtc C;
>         struct drm_crtc *CP;
>         @@
>         (
>         - C.enabled
>         + C.state->enable
>         |
>         - CP->enabled
>         + CP->state->enable
>         )
> 
>         // For assignments, we still update the legacy value as well as the state value
>         // so add an extra assignment statement for that.
>         @@
>         struct drm_crtc C;
>         struct drm_crtc *CP;
>         expression E;
>         @@
>         (
>           C.state->enable = E;
>         + C.enabled = E;
>         |
>           CP->state->enable = E;
>         + CP->enabled = E;
>         )
>         </smpl>
> 
> The crtc->mode and crtc->hwmode fields should probably be transitioned
> over as well eventually, but we seem to do an okay job of keeping those
> up-to-date already so I want to minimize the changes that will clash
> with Ander's in-progress atomic work.
> 
> v2: Don't remove the assignments to the legacy value when we assign to
>     the state value.  A second cocci stanza takes care of adding the
>     legacy assignment back where appropriate.  (Daniel)
> 
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>

Both patches applied, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
>  3 files changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 07e257c..9baecb7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -791,7 +791,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  		return -EINVAL;
>  	}
>  
> -	if (!crtc->enabled) {
> +	if (!crtc->state->enable) {
>  		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
>  		return -EBUSY;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9bc54cc..acbb362 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3077,7 +3077,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  
>  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return crtc->base.enabled && crtc->active &&
> +	return crtc->base.state->enable && crtc->active &&
>  		crtc->config->has_pch_encoder;
>  }
>  
> @@ -4215,7 +4215,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	bool reenable_ips = false;
>  
>  	/* The clocks have to be on to load the palette. */
> -	if (!crtc->enabled || !intel_crtc->active)
> +	if (!crtc->state->enable || !intel_crtc->active)
>  		return;
>  
>  	if (!HAS_PCH_SPLIT(dev_priv->dev)) {
> @@ -4328,7 +4328,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	WARN_ON(!crtc->enabled);
> +	WARN_ON(!crtc->state->enable);
>  
>  	if (intel_crtc->active)
>  		return;
> @@ -4436,7 +4436,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	WARN_ON(!crtc->enabled);
> +	WARN_ON(!crtc->state->enable);
>  
>  	if (intel_crtc->active)
>  		return;
> @@ -4783,7 +4783,7 @@ static void modeset_update_crtc_power_domains(struct drm_device *dev)
>  	for_each_intel_crtc(dev, crtc) {
>  		enum intel_display_power_domain domain;
>  
> -		if (!crtc->base.enabled)
> +		if (!crtc->base.state->enable)
>  			continue;
>  
>  		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
> @@ -5004,7 +5004,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
>  
>  	/* disable/enable all currently active pipes while we change cdclk */
>  	for_each_intel_crtc(dev, intel_crtc)
> -		if (intel_crtc->base.enabled)
> +		if (intel_crtc->base.state->enable)
>  			*prepare_pipes |= (1 << intel_crtc->pipe);
>  }
>  
> @@ -5044,7 +5044,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	bool is_dsi;
>  
> -	WARN_ON(!crtc->enabled);
> +	WARN_ON(!crtc->state->enable);
>  
>  	if (intel_crtc->active)
>  		return;
> @@ -5127,7 +5127,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	WARN_ON(!crtc->enabled);
> +	WARN_ON(!crtc->state->enable);
>  
>  	if (intel_crtc->active)
>  		return;
> @@ -5326,7 +5326,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* crtc should still be enabled when we disable it. */
> -	WARN_ON(!crtc->enabled);
> +	WARN_ON(!crtc->state->enable);
>  
>  	dev_priv->display.crtc_disable(crtc);
>  	dev_priv->display.off(crtc);
> @@ -5404,7 +5404,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  
>  			crtc = encoder->base.crtc;
>  
> -			I915_STATE_WARN(!crtc->enabled, "crtc not enabled\n");
> +			I915_STATE_WARN(!crtc->state->enable,
> +					"crtc not enabled\n");
>  			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
>  			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
>  			     "encoder active on the wrong pipe\n");
> @@ -8717,7 +8718,7 @@ retry:
>  		i++;
>  		if (!(encoder->possible_crtcs & (1 << i)))
>  			continue;
> -		if (possible_crtc->enabled)
> +		if (possible_crtc->state->enable)
>  			continue;
>  		/* This can occur when applying the pipe A quirk on resume. */
>  		if (to_intel_crtc(possible_crtc)->new_enabled)
> @@ -8785,7 +8786,7 @@ retry:
>  	return true;
>  
>   fail:
> -	intel_crtc->new_enabled = crtc->enabled;
> +	intel_crtc->new_enabled = crtc->state->enable;
>  	if (intel_crtc->new_enabled)
>  		intel_crtc->new_config = intel_crtc->config;
>  	else
> @@ -9945,7 +9946,7 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>  	}
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		crtc->new_enabled = crtc->base.enabled;
> +		crtc->new_enabled = crtc->base.state->enable;
>  
>  		if (crtc->new_enabled)
>  			crtc->new_config = crtc->config;
> @@ -9975,6 +9976,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
>  	}
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		crtc->base.state->enable = crtc->new_enabled;
>  		crtc->base.enabled = crtc->new_enabled;
>  	}
>  }
> @@ -10386,7 +10388,7 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
>  
>  	/* Check for pipes that will be enabled/disabled ... */
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->base.enabled == intel_crtc->new_enabled)
> +		if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
>  			continue;
>  
>  		if (!intel_crtc->new_enabled)
> @@ -10461,10 +10463,10 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  
>  	/* Double check state. */
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		WARN_ON(intel_crtc->base.enabled != intel_crtc_in_use(&intel_crtc->base));
> +		WARN_ON(intel_crtc->base.state->enable != intel_crtc_in_use(&intel_crtc->base));
>  		WARN_ON(intel_crtc->new_config &&
>  			intel_crtc->new_config != intel_crtc->config);
> -		WARN_ON(intel_crtc->base.enabled != !!intel_crtc->new_config);
> +		WARN_ON(intel_crtc->base.state->enable != !!intel_crtc->new_config);
>  	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -10851,7 +10853,7 @@ check_crtc_state(struct drm_device *dev)
>  		DRM_DEBUG_KMS("[CRTC:%d]\n",
>  			      crtc->base.base.id);
>  
> -		I915_STATE_WARN(crtc->active && !crtc->base.enabled,
> +		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
>  		for_each_intel_encoder(dev, encoder) {
> @@ -10865,9 +10867,10 @@ check_crtc_state(struct drm_device *dev)
>  		I915_STATE_WARN(active != crtc->active,
>  		     "crtc's computed active state doesn't match tracked active state "
>  		     "(expected %i, found %i)\n", active, crtc->active);
> -		I915_STATE_WARN(enabled != crtc->base.enabled,
> +		I915_STATE_WARN(enabled != crtc->base.state->enable,
>  		     "crtc's computed enabled state doesn't match tracked enabled state "
> -		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
> +		     "(expected %i, found %i)\n", enabled,
> +				crtc->base.state->enable);
>  
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							   &pipe_config);
> @@ -10931,7 +10934,7 @@ check_shared_dpll_state(struct drm_device *dev)
>  		     pll->on, active);
>  
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->base.enabled && intel_crtc_to_shared_dpll(crtc) == pll)
> +			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
>  				enabled_crtcs++;
>  			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
>  				active_crtcs++;
> @@ -11117,7 +11120,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		intel_crtc_disable(&intel_crtc->base);
>  
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> -		if (intel_crtc->base.enabled)
> +		if (intel_crtc->base.state->enable)
>  			dev_priv->display.crtc_disable(&intel_crtc->base);
>  	}
>  
> @@ -11173,7 +11176,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	/* FIXME: add subpixel order */
>  done:
> -	if (ret && crtc->enabled)
> +	if (ret && crtc->state->enable)
>  		crtc->mode = *saved_mode;
>  
>  	kfree(saved_mode);
> @@ -11269,7 +11272,7 @@ static int intel_set_config_save_state(struct drm_device *dev,
>  	 */
>  	count = 0;
>  	for_each_crtc(dev, crtc) {
> -		config->save_crtc_enabled[count++] = crtc->enabled;
> +		config->save_crtc_enabled[count++] = crtc->state->enable;
>  	}
>  
>  	count = 0;
> @@ -11503,7 +11506,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  			}
>  		}
>  
> -		if (crtc->new_enabled != crtc->base.enabled) {
> +		if (crtc->new_enabled != crtc->base.state->enable) {
>  			DRM_DEBUG_KMS("crtc %sabled, full mode switch\n",
>  				      crtc->new_enabled ? "en" : "dis");
>  			config->mode_changed = true;
> @@ -13392,6 +13395,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			}
>  
>  		WARN_ON(crtc->active);
> +		crtc->base.state->enable = false;
>  		crtc->base.enabled = false;
>  	}
>  
> @@ -13408,7 +13412,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * have active connectors/encoders. */
>  	intel_crtc_update_dpms(&crtc->base);
>  
> -	if (crtc->active != crtc->base.enabled) {
> +	if (crtc->active != crtc->base.state->enable) {
>  		struct intel_encoder *encoder;
>  
>  		/* This can happen either due to bugs in the get_hw_state
> @@ -13416,9 +13420,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * pipe A quirk. */
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
>  			      crtc->base.base.id,
> -			      crtc->base.enabled ? "enabled" : "disabled",
> +			      crtc->base.state->enable ? "enabled" : "disabled",
>  			      crtc->active ? "enabled" : "disabled");
>  
> +		crtc->base.state->enable = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		/* Because we only establish the connector -> encoder ->
> @@ -13555,6 +13560,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		crtc->active = dev_priv->display.get_pipe_config(crtc,
>  								 crtc->config);
>  
> +		crtc->base.state->enable = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  		crtc->primary_enabled = primary_get_hw_state(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 071b96d..24e8730 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -509,7 +509,7 @@ static int intel_lvds_set_property(struct drm_connector *connector,
>  		intel_connector->panel.fitting_mode = value;
>  
>  		crtc = intel_attached_encoder(connector)->base.crtc;
> -		if (crtc && crtc->enabled) {
> +		if (crtc && crtc->state->enable) {
>  			/*
>  			 * If the CRTC is enabled, the display will be changed
>  			 * according to the new panel fitting mode.
> -- 
> 1.8.5.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


More information about the Intel-gfx mailing list