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

Tue Mar 21 14:43:12 UTC 2017

On Tue, Mar 21, 2017 at 03:42:53PM +0200, Ander Conselvan De Oliveira wrote:
> 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.

https://patchwork.freedesktop.org/series/21226/#rev1

-- 
Ville Syrjälä
Intel OTC