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

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

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.

-- 
Ville Syrjälä
Intel