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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx