Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> When we decide that a plane is attached to the wrong pipe we try
> to turn off said plane. However we are passing around the crtc we
> think that the plane is supposed to be using rather than the crtc
> it is currently using. That doesn't work all that well because
> we may have to do vblank waits etc. and the other pipe might
> not even be enabled here. So let's pass the plane's current crtc to
> intel_plane_disable_noatomic() so that it can its job correctly.
> 
> To do that semi-cleanly we also have to change the plane readout
> to record the plane's visibility into the bitmasks of the crtc
> where the plane is currently enabled rather than to the crtc
> we want to use for the plane.
> 
> One caveat here is that our active_planes bitmask will get confused
> if both planes are enabled on the same pipe. Fortunately we can use
> plane_mask to reconstruct active_planes sufficiently since
> plane_mask still has the same meaning (is the plane visible?)
> during readout. We also have to do the same during the initial
> plane readout as the second plane could clear the active_planes
> bit the first plane had already set.

How often have we broken this :-/

Unfortunately I still don't have a good idea how to best CI this, since we
shut down everything on module unload. Maybe we should have a special mode
for module unload to leave the hw on, so that we can start testing various
fastboot scenarios ...

Some questions below.

> Cc: stable@vger.kernel.org # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> Cc: stable@vger.kernel.org
> Cc: Dennis <dennis.nezic@utoronto.ca>
> Tested-by: Dennis <dennis.nezic@utoronto.ca>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105637
> Fixes: b1e01595a66d ("drm/i915: Redo plane sanitation during readout")
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e018b37bed39..c72be8cd1f54 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15475,15 +15475,16 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	POSTING_READ(DPLL(pipe));
>  }
>  
> -static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
> -				   struct intel_plane *plane)
> +static void fixup_active_planes(struct intel_crtc *crtc)
>  {
> -	enum pipe pipe;
> -
> -	if (!plane->get_hw_state(plane, &pipe))
> -		return true;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_state *crtc_state =
> +		to_intel_crtc_state(crtc->base.state);
> +	struct drm_plane *plane;
>  
> -	return pipe == crtc->pipe;
> +	drm_for_each_plane_mask(plane, &dev_priv->drm,
> +				crtc_state->base.plane_mask)
> +		crtc_state->active_planes |= BIT(to_intel_plane(plane)->id);

I think we need to also update plane_mask here.

>  }
>  
>  static void
> @@ -15497,13 +15498,28 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		struct intel_plane *plane =
>  			to_intel_plane(crtc->base.primary);
> +		struct intel_crtc *plane_crtc;
> +		enum pipe pipe;
> +
> +		if (!plane->get_hw_state(plane, &pipe))
> +			continue;
>  
> -		if (intel_plane_mapping_ok(crtc, plane))
> +		if (pipe == crtc->pipe)
>  			continue;
>  
>  		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
>  			      plane->base.name);
> -		intel_plane_disable_noatomic(crtc, plane);
> +
> +		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +		intel_plane_disable_noatomic(plane_crtc, plane);
> +
> +		/*
> +		 * Our active_planes tracking will get confused here
> +		 * if both planes A and B are enabled on the same pipe
> +		 * (since both planes map to BIT(PLANE_PRIMARY)).
> +		 * Reconstruct active_planes after disabling the plane.
> +		 */

Hm, would be neat if we could retire intel_crtc_state->active_planes in
favour of drm_crtc_state->plane_mask. Except for that entire visible y/n
thing :-/

> +		fixup_active_planes(plane_crtc);

Bit a bikeshed, but what about throwing the plane state away and just
starting over, instead of trying to fix it up? We could then use that as a
consistency check, if the plane mappings are still wrong our code is
broken and we should bail out with a very loud warning.

But this here should work too, albeit a bit more fragile I think.

Cheers, Daniel
>  	}
>  }
>  
> @@ -15671,23 +15687,38 @@ void i915_redisable_vga(struct drm_i915_private *dev_priv)
>  }
>  
>  /* FIXME read out full plane state for all planes */
> -static void readout_plane_state(struct intel_crtc *crtc)
> +static void readout_plane_state(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_crtc_state *crtc_state =
> -		to_intel_crtc_state(crtc->base.state);
>  	struct intel_plane *plane;
> +	struct intel_crtc *crtc;
>  
> -	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> +	for_each_intel_plane(&dev_priv->drm, plane) {
>  		struct intel_plane_state *plane_state =
>  			to_intel_plane_state(plane->base.state);
> +		struct intel_crtc_state *crtc_state;
>  		enum pipe pipe;
>  		bool visible;
>  
>  		visible = plane->get_hw_state(plane, &pipe);
>  
> +		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +
>  		intel_set_plane_visible(crtc_state, plane_state, visible);
>  	}
> +
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		/*
> +		 * Our active_planes tracking may get confused here
> +		 * on gen2/3 if the first plane is enabled but the
> +		 * second one isn't but both indicate the same pipe.
> +		 * The second plane would clear the active_planes
> +		 * bit for the first plane (since both map to
> +		 * BIT(PLANE_PRIMARY). Reconstruct active_planes
> +		 * after plane readout is done.
> +		 */
> +		fixup_active_planes(crtc);
> +	}
>  }
>  
>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> @@ -15719,13 +15750,13 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		if (crtc_state->base.active)
>  			dev_priv->active_crtcs |= 1 << crtc->pipe;
>  
> -		readout_plane_state(crtc);
> -
>  		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
>  			      crtc->base.base.id, crtc->base.name,
>  			      enableddisabled(crtc_state->base.active));
>  	}
>  
> +	readout_plane_state(dev_priv);
> +
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>  
> -- 
> 2.16.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch