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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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ä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > 
> > > > 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: stable@xxxxxxxxxxxxxxx # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Cc: Dennis <dennis.nezic@xxxxxxxxxxx>
> > > > Tested-by: Dennis <dennis.nezic@xxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  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
> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > 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

-- 
Ville Syrjälä
Intel



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux