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@xxxxxxxxxxxxxxx:
>>>>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxxxxxxx>
>>>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>>> Cc: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx>
>>>>> Reported-by: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx>
>>>>> 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@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
