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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 02, 2018 at 02:11:34PM +0200, Daniel Vetter wrote:
> 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 ...

Yeah, that might be nice. Though wouldn't directly help here since
we'd still have to move the plane to the other pipe. But we could
of course make the driver unload do that for us as well.

Oh and to hit this bug we'd also need to make sure cxsr is enabled
when we unload as that's what leads to the vblank wait. That's actually
the reason I didn't catch this bug originally. None of my machines
have a VBIOS that enables cxsr.

> 
> 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.

plane_mask will be correct since each plane has a unique bit there.
And in fact we use plane_mask to reconstruct active_planes.

What we could do is set active_planes=0 before the loop, as the loop
will populate it fully anyway.

> 
> >  }
> >  
> >  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 :-/

I'm a bit torn about this. active_planes is rather convenient for
watermark stuff and whatnot, but on the other hand it doesn't map
well to pre-g4x hardware, so in other ways it's not so great.

> 
> > +		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?

You mean just zeroing the plane masks in the crtc state and
doing the plane_readout again? That should be doable.

> 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.

Indeed. That seems like a half decent sanity check.

> 
> 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

-- 
Ville Syrj�l�
Intel