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