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

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

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@linux.intel.com:
> >>>>> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> >>>>> Reported-by: Uwe Kleine-König <uwe@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@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@linux.intel.com>

Pushed to dinq. Thanks for the review and testing.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx