[2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

Commit Message

Ville Syrjala Oct. 1, 2018, 2:31 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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.

Cc: [e-mail] # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
Cc: [e-mail]
Cc: Dennis <[e-mail]>
Tested-by: Dennis <[e-mail]>
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ä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 16 deletions(-)

Comments

Daniel Vetter Oct. 2, 2018, 12:11 p.m. UTC | #1

On Mon, Oct 01, 2018 at 05:31:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> 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: [e-mail] # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> Cc: [e-mail]
> Cc: Dennis <[e-mail]>
> Tested-by: Dennis <[e-mail]>
> 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ä <[e-mail]>
> ---
>  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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjala Oct. 2, 2018, 2:21 p.m. UTC | #2

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ä <[e-mail]>
> > 
> > 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: [e-mail] # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> > Cc: [e-mail]
> > Cc: Dennis <[e-mail]>
> > Tested-by: Dennis <[e-mail]>
> > 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ä <[e-mail]>
> > ---
> >  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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter Oct. 3, 2018, 8:53 a.m. UTC | #3

On Tue, Oct 02, 2018 at 05:21:36PM +0300, Ville Syrjälä wrote:
> 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ä <[e-mail]>
> > > 
> > > 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.

Well that should be easy, as long as i915.ko enables cxsr ...

Just pondered this since with Hans' work fedora is now using fastbook, so
constantly breaking this stuff is kinda no longer a good option. But
definitely future work.

> > Some questions below.
> > 
> > > Cc: [e-mail] # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> > > Cc: [e-mail]
> > > Cc: Dennis <[e-mail]>
> > > Tested-by: Dennis <[e-mail]>
> > > 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ä <[e-mail]>
> > > ---
> > >  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.

That + comment explaining why we don't need to reconstruct plane_mask
would be good. Since I completely missed that. Something like:

	/* active_planes aliases if mutliple "primary" planes have been
	 * used on the same (or wrong) pipe. plane_mask uses unique ids,
	 * hence we can use that to reconstruct active_planes. */

Hm, maybe we want to remove the active_planes updating from
intel_set_plane_visible then, since it's just garbage anyway? And with the
3rd caller removed, we always follow up with a call to this fixup function
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 :-/
> 
> 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.

Yeah. Maybe a for_each_visible_plane_on_crtc iterator could help, which is
essentially:
	
	for_each_plane_on_crtc()
		for_each_if(plane_state->visible)
	
We'd still need to frob the intel_plane->plane out for our wm code. But
the macro might be generally useful in other drivers too.

Anyway, all just an aside.
> 
> > 
> > > +		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.

Nah strike that, on second thought I think the pattern of first doing
intel_set_plane_visible (but without the ->active_plane stuff), then
fixup_active_planes() sounds clear enough for me. If it works. I think
this was just me not entirely understanding the problem (and some
redundant code that threw me off the rails).

Cheers, Daniel

> > 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
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Oct. 3, 2018, 2:32 p.m. UTC | #4

On Wed, Oct 03, 2018 at 10:53:11AM +0200, Daniel Vetter wrote:
> On Tue, Oct 02, 2018 at 05:21:36PM +0300, Ville Syrjälä wrote:
> > 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ä <[e-mail]>
> > > > 
> > > > 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.
> 
> Well that should be easy, as long as i915.ko enables cxsr ...
> 
> Just pondered this since with Hans' work fedora is now using fastbook, so
> constantly breaking this stuff is kinda no longer a good option. But
> definitely future work.
> 
> > > Some questions below.
> > > 
> > > > Cc: [e-mail] # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> > > > Cc: [e-mail]
> > > > Cc: Dennis <[e-mail]>
> > > > Tested-by: Dennis <[e-mail]>
> > > > 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ä <[e-mail]>
> > > > ---
> > > >  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.
> 
> That + comment explaining why we don't need to reconstruct plane_mask
> would be good. Since I completely missed that. Something like:
> 
> 	/* active_planes aliases if mutliple "primary" planes have been
> 	 * used on the same (or wrong) pipe. plane_mask uses unique ids,
> 	 * hence we can use that to reconstruct active_planes. */

Sure.

> 
> Hm, maybe we want to remove the active_planes updating from
> intel_set_plane_visible then, since it's just garbage anyway? And with the
> 3rd caller removed, we always follow up with a call to this fixup function
> here.

There's still intel_plane_disable_noatomic(). Though I guess we could
just call the fixup from there as well.

> 
> > > 
> > > >  }
> > > >  
> > > >  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.
> 
> Yeah. Maybe a for_each_visible_plane_on_crtc iterator could help, which is
> essentially:
> 	
> 	for_each_plane_on_crtc()
> 		for_each_if(plane_state->visible)
> 	
> We'd still need to frob the intel_plane->plane out for our wm code. But
> the macro might be generally useful in other drivers too.
> 
> Anyway, all just an aside.
> > 
> > > 
> > > > +		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.
> 
> Nah strike that, on second thought I think the pattern of first doing
> intel_set_plane_visible (but without the ->active_plane stuff), then
> fixup_active_planes() sounds clear enough for me. If it works. I think
> this was just me not entirely understanding the problem (and some
> redundant code that threw me off the rails).
> 
> Cheers, Daniel
> 
> > > 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
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch

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);
 }
 
 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.
+		 */
+		fixup_active_planes(plane_crtc);
 	}
 }
 
@@ -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];