Re: [Intel-gfx] [PATCH] drm/i915: Use vblank worker to unpin old legacy cursor fb safely

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

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@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@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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