Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.
Date: Wed, 29 Jul 2015 14:49:45 +0300	[thread overview]
Message-ID: <1438170585.3839.8.camel@gmail.com> (raw)
In-Reply-To: <1438000541-3219-7-git-send-email-maarten.lankhorst@linux.intel.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Instead of allocating pipe_config on the stack use the old crtc_state,
> it's only going to freed from this point on.
> 
> All crtc's encoders are now only checked once during modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
>  1 file changed, 56 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fbb257d4728c..e3afe611a78c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc_state);
>  	struct drm_atomic_state *state = crtc_state->state;
> -	int ret, idx = crtc->base.id;
> +	int ret;
>  	bool mode_changed = needs_modeset(crtc_state);
>  
>  	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
> @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> -		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
> -		idx, crtc->state->active, intel_crtc->active);
> -
>  	if (mode_changed && !crtc_state->active)
>  		intel_crtc->atomic.update_wm_post = true;
>  
> @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev)
>  
>  	for_each_intel_encoder(dev, encoder) {
>  		bool enabled = false;
> -		bool active = false;
> -		enum pipe pipe, tracked_pipe;
> +		enum pipe pipe;
>  
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
>  			      encoder->base.base.id,
>  			      encoder->base.name);
>  
>  		for_each_intel_connector(dev, connector) {
> -			if (connector->base.encoder != &encoder->base)
> +			if (connector->base.state->best_encoder != &encoder->base)
>  				continue;
>  			enabled = true;
> -			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
> -				active = true;
>  
>  			I915_STATE_WARN(connector->base.state->crtc !=
>  					encoder->base.crtc,
> @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev)
>  		     "encoder's enabled state mismatch "
>  		     "(expected %i, found %i)\n",
>  		     !!encoder->base.crtc, enabled);
> -		I915_STATE_WARN(active && !encoder->base.crtc,
> -		     "active encoder with no crtc\n");
> -
> -		active = encoder->get_hw_state(encoder, &pipe);
>  
>  		if (!encoder->base.crtc) {
> -			I915_STATE_WARN(active,
> -			     "encoder detached but not turned off.\n");
> +			bool active;
>  
> -			continue;
> +			active = encoder->get_hw_state(encoder, &pipe);
> +			I915_STATE_WARN(active,
> +			     "encoder detached but still enabled on pipe %c.\n",
> +			     pipe_name(pipe));
>  		}
> -
> -		I915_STATE_WARN(active != encoder->base.crtc->state->active,
> -		     "encoder's hw state doesn't match sw tracking "
> -		     "(expected %i, found %i)\n",
> -		     encoder->base.crtc->state->active, active);
> -
> -
> -		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> -		I915_STATE_WARN(active && pipe != tracked_pipe,
> -		     "active encoder's pipe doesn't match"
> -		     "(expected %i, found %i)\n",
> -		     tracked_pipe, pipe);
> -
>  	}
>  }
>  
>  static void
> -check_crtc_state(struct drm_device *dev)
> +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
> -	struct intel_crtc_state pipe_config;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {

So now we only check the state of crtcs affected by the modeset. In the
unlikely case of a bug that changes the hw state of another crtc that
should have been unchanged, the old code might catch the issue but the
new one doesn't. Isn't it better to just check everything?

Ander

> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		struct intel_crtc_state *pipe_config, *sw_config;
>  		bool active;
>  
> -		memset(&pipe_config, 0, sizeof(pipe_config));
> +		if (!needs_modeset(crtc->state))
> +			continue;
>  
> -		DRM_DEBUG_KMS("[CRTC:%d]\n",
> -			      crtc->base.base.id);
> +		__drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
> +		pipe_config = to_intel_crtc_state(crtc_state);
> +		memset(pipe_config, 0, sizeof(*pipe_config));
> +		crtc_state->crtc = crtc;
> +		crtc_state->state = state;
>  
> -		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
> -		     "active crtc, but not enabled in sw tracking\n");
> +		DRM_DEBUG_KMS("[CRTC:%d]\n",
> +			      crtc->base.id);
>  
> -		active = dev_priv->display.get_pipe_config(crtc,
> -							   &pipe_config);
> +		active = dev_priv->display.get_pipe_config(intel_crtc,
> +							   pipe_config);
>  
>  		/* hw state is inconsistent with the pipe quirk */
> -		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> -		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> -			active = crtc->active;
> +		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> +		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> +			active = crtc->state->active;
>  
> -		for_each_intel_encoder(dev, encoder) {
> -			enum pipe pipe;
> -			if (encoder->base.crtc != &crtc->base)
> -				continue;
> -			if (encoder->get_hw_state(encoder, &pipe))
> -				encoder->get_config(encoder, &pipe_config);
> -		}
> -
> -		I915_STATE_WARN(crtc->active != active,
> +		I915_STATE_WARN(crtc->state->active != active,
>  		     "crtc active state doesn't match with hw state "
> -		     "(expected %i, found %i)\n", crtc->active, active);
> +		     "(expected %i, found %i)\n", crtc->state->active, active);
>  
> -		I915_STATE_WARN(crtc->active != crtc->base.state->active,
> +		I915_STATE_WARN(intel_crtc->active != crtc->state->active,
>  		     "transitional active state does not match atomic hw state "
> -		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
> +		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active);
> +
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			enum pipe pipe;
> +
> +			active = encoder->get_hw_state(encoder, &pipe);
> +			I915_STATE_WARN(active != crtc->state->active,
> +				"[ENCODER:%i] active %i with crtc active %i\n",
> +				encoder->base.base.id, active, crtc->state->active);
> +
> +			I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> +					"Encoder connected to wrong pipe %c\n",
> +					pipe_name(pipe));
> +
> +			if (active)
> +				encoder->get_config(encoder, pipe_config);
> +		}
>  
> -		if (!active)
> +		if (!crtc->state->active)
>  			continue;
>  
> -		if (!intel_pipe_config_compare(dev, crtc->config,
> -					       &pipe_config, false)) {
> +		sw_config = to_intel_crtc_state(crtc->state);
> +		if (!intel_pipe_config_compare(dev, sw_config,
> +					       pipe_config, false)) {
>  			I915_STATE_WARN(1, "pipe state doesn't match!\n");
> -			intel_dump_pipe_config(crtc, &pipe_config,
> +			intel_dump_pipe_config(intel_crtc, pipe_config,
>  					       "[hw state]");
> -			intel_dump_pipe_config(crtc, crtc->config,
> +			intel_dump_pipe_config(intel_crtc, sw_config,
>  					       "[sw state]");
>  		}
>  	}
> @@ -12884,7 +12878,7 @@ intel_modeset_check_state(struct drm_device *dev,
>  	check_wm_state(dev);
>  	check_connector_state(dev, state);
>  	check_encoder_state(dev);
> -	check_crtc_state(dev);
> +	check_crtc_state(dev, state);
>  	check_shared_dpll_state(dev);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx