[Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

Tue Mar 21 13:42:53 UTC 2017

On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote:
> On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> > Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> > > > Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> > > > > On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> > > > > > Op 17-02-17 om 16:01 schreef ville.syrjala at linux.intel.com:
> > > > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > 
> > > > > > > In order to make cursor updates actually safe wrt. watermark programming
> > > > > > > we have to clear the legacy_cursor_update flag in the atomic state. That
> > > > > > > will cause the regular atomic update path to do the necessary vblank
> > > > > > > wait after the plane update if needed, otherwise the vblank wait would
> > > > > > > be skipped and we'd feed the optimal watermarks to the hardware before
> > > > > > > the plane update has actually happened.
> > > > > > > 
> > > > > > > To make the slow vs. fast path determination in
> > > > > > > intel_legacy_cursor_update() a little simpler we can ignore the actual
> > > > > > > visibility of the plane (which can only get computed once we've already
> > > > > > > chosen out path) and instead we simply check whether the fb is being
> > > > > > > set or cleared by the user. This means a fully clipped but logically
> > > > > > > visible cursor will be considered visible as far as watermark
> > > > > > > programming is concerned. We can do that for the cursor since it's a
> > > > > > > fixed size plane and the clipped size doesn't play a role in the
> > > > > > > watermark computation.
> > > > > > > 
> > > > > > > This should fix underruns that can occur when the cursor gets
> > > > > > > enable/disabled or the size gets changed. Hopefully it's good enough
> > > > > > > that only pure cursor movement and flips go through unthrottled.
> > > > > > > 
> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > > Cc: Uwe Kleine-König <uwe at kleine-koenig.org>
> > > > > > > Reported-by: Uwe Kleine-König <uwe at kleine-koenig.org>
> > > > > > > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.")
> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c      | 20 ++++++++++++--------
> > > > > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > index b05d9c85384b..356ac04093e8 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device *dev,
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > >  	int ret = 0;
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * The intel_legacy_cursor_update() fast path takes care
> > > > > > > +	 * of avoiding the vblank waits for simple cursor
> > > > > > > +	 * movement and flips. For cursor on/off and size changes,
> > > > > > > +	 * we want to perform the vblank waits so that watermark
> > > > > > > +	 * updates happen during the correct frames. Gen9+ have
> > > > > > > +	 * double buffered watermarks and so shouldn't need this.
> > > > > > > +	 */
> > > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > > +		state->legacy_cursor_update = false;
> > > > > > 
> > > > > > Could we perhaps add a check in ilk_compute_pipe_wm which unsets the legacy_cursor_update flag so we keep things unsynced as much as possible?
> > > > > 
> > > > > I'd have to sprinkle that stuff everywhere but the SKL code
> > > > > eventually. Seems a little pointless when I can just plop it
> > > > > there.
> > > > 
> > > > Ah indeed. Lets hope it doesn't slow things down too much.
> > > > > > >  	ret = drm_atomic_helper_setup_commit(state, nonblock);
> > > > > > >  	if (ret)
> > > > > > >  		return ret;
> > > > > > > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > > > > >  	    old_plane_state->src_h != src_h ||
> > > > > > >  	    old_plane_state->crtc_w != crtc_w ||
> > > > > > >  	    old_plane_state->crtc_h != crtc_h ||
> > > > > > > -	    !old_plane_state->visible ||
> > > > > > > -	    old_plane_state->fb->modifier != fb->modifier)
> > > > > > > +	    !old_plane_state->fb != !fb)
> > > > > > >  		goto slow;
> > > > > > >  
> > > > > > >  	new_plane_state = intel_plane_duplicate_state(plane);
> > > > > > > @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > > > > >  	if (ret)
> > > > > > >  		goto out_free;
> > > > > > >  
> > > > > > > -	/* Visibility changed, must take slowpath. */
> > > > > > > -	if (!new_plane_state->visible)
> > > > > > > -		goto slow_free;
> > > > > > > -
> > > > > > >  	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> > > > > > >  	if (ret)
> > > > > > >  		goto out_free;
> > > > > > 
> > > > > > Those 2 hunks are needed. If you move the cursor out of the visible area or back you need to update.
> > > > > 
> > > > > No. I changed the wm code to consider a non-visible but logicall active
> > > > > cursor as needing proper watermarks. That avoids needing this fallback
> > > > > path here.
> > > > 
> > > > Ah indeed. But one thing you dropped is the fb modifier check.
> > > > I suppose there's no harm with no support for using prite planes as cursor plane yet, but might be nice to keep it in.
> > > 
> > > We'd have bigger problems than the modifier if we want to use a sprite
> > > plane as the cursor because for sprite planes the watermarks are
> > > computed based on the clipped size. So the wm code would need some
> > > surgery as well.
> > > 
> > > > Cc'ing Ristovski for testing the patch. :)
> > > 
> > > 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my
> > > manual tests, including vigorous_pointer_movement and
> > > spastic_window_repositioning, good enough for me!
> > > 
> > 
> > Fair enough.
> > 
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> 
> Pushed to dinq. Thanks for the review and testing.

This patch causes FIFO underruns on gen9 devices with kms_cursor_crc *-random
tests. The problem seems to happen when the cursor plane gets enabled after
being disabled because the cursor was completely offscreen. With the patch
reverted the enable goes through the slow path, which doesn't cause an underrun.

Ander