[Intel-gfx] [PATCH] drm/i915: Use vblank worker to unpin old legacy cursor fb safely
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Sep 12 19:44:50 UTC 2023
Hey,
On 2023-09-01 12:56, Ville Syrjälä wrote:
> On Fri, Sep 01, 2023 at 12:16:21PM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>>
>> Den 2023-08-31 kl. 18:26, skrev Ville Syrjala:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> The cursor hardware only does sync updates, and thus the hardware
>>> will be scanning out from the old fb until the next start of vblank.
>>> So in order to make the legacy cursor fastpath actually safe we
>>> should not unpin the old fb until we're sure the hardware has
>>> ceased accessing it. The simplest approach is to just use a vblank
>>> work here to do the delayed unpin.
>>>
>>> Not 100% sure it's a good idea to put this onto the same high
>>> priority vblank worker as eg. our timing critical gamma updates.
>>> But let's keep it simple for now, and it we later discover that
>>> this is causing problems we can think about adding a lower
>>> priority worker for such things.
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_cursor.c | 25 +++++++++++++++++--
>>> .../drm/i915/display/intel_display_types.h | 3 +++
>>> 2 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> index b342fad180ca..2bd1a79c6955 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> @@ -603,6 +603,16 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
>>> return format == DRM_FORMAT_ARGB8888;
>>> }
>>>
>>> +static void intel_cursor_unpin_work(struct kthread_work *base)
>>> +{
>>> + struct drm_vblank_work *work = to_drm_vblank_work(base);
>>> + struct intel_plane_state *plane_state =
>>> + container_of(work, typeof(*plane_state), unpin_work);
>>> +
>>> + intel_plane_unpin_fb(plane_state);
>>> + intel_plane_destroy_state(plane_state->uapi.plane, &plane_state->uapi);
>>> +}
>>> +
>>> static int
>>> intel_legacy_cursor_update(struct drm_plane *_plane,
>>> struct drm_crtc *_crtc,
>>> @@ -730,14 +740,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>>
>>> local_irq_enable();
>>>
>>> - intel_plane_unpin_fb(old_plane_state);
>>> + if (old_plane_state->hw.fb != new_plane_state->hw.fb) {
>>> + drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
>>> + intel_cursor_unpin_work);
>>> +
>>> + drm_vblank_work_schedule(&old_plane_state->unpin_work,
>>> + drm_crtc_accurate_vblank_count(&crtc->base) + 1,
>>> + false);
>>> +
>>> + old_plane_state = NULL;
>>> + } else {
>>> + intel_plane_unpin_fb(old_plane_state);
>>> + }
>>
>> Maybe check if crtc is active and no modeset is happening?
>
> We wouldn't be on this codepath if that wasn't the case.
>
>> Similar to
>> how the vblank worker is used in other cases. That should hopefully fix
>> the cursor leak test.
>
> The leak is likely due to the vblank worker being a bit crazy
> and sometimes silently cancelling pending works. I fired a new
> series at trybot that tries to avoid that.
>
I saw the trybot series. Have you tried fixing it to fire off the vblank
work immediately before disabling?
~Maarten
More information about the Intel-gfx
mailing list