> -----Original Message----- > From: Intel-xe <[email protected]> On Behalf Of Maarten > Lankhorst > Sent: Wednesday, May 22, 2024 11:04 AM > To: [email protected] > Cc: [email protected]; Ville Syrjälä > <[email protected]>; > Maarten Lankhorst <[email protected]> > Subject: [v6 2/3] drm/i915: Use vblank worker to unpin old legacy cursor fb > safely > > From: Ville Syrjälä <[email protected]> > > 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. > > This patch is slightly reworked by Maarten
Looks Good to me. Reviewed-by: Uma Shankar <[email protected]> > Cc: Maarten Lankhorst <[email protected]> > Signed-off-by: Ville Syrjälä <[email protected]> > Signed-off-by: Maarten Lankhorst <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_cursor.c | 26 +++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_display.c | 3 +++ > .../drm/i915/display/intel_display_types.h | 3 +++ > 3 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > b/drivers/gpu/drm/i915/display/intel_cursor.c > index c780ce146131..36e2dcbe3614 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > @@ -710,6 +710,17 @@ 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); > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > + > + intel_plane_unpin_fb(plane_state); > + intel_plane_destroy_state(&plane->base, &plane_state->uapi); } > + > static int > intel_legacy_cursor_update(struct drm_plane *_plane, > struct drm_crtc *_crtc, > @@ -853,14 +864,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > intel_psr_unlock(crtc_state); > > - intel_plane_unpin_fb(old_plane_state); > + if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) { > + 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); > + } > > out_free: > if (new_crtc_state) > intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi); > if (ret) > intel_plane_destroy_state(&plane->base, &new_plane_state- > >uapi); > - else > + else if (old_plane_state) > intel_plane_destroy_state(&plane->base, &old_plane_state- > >uapi); > return ret; > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index cce1420fb541..715672c142b4 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -66,6 +66,7 @@ > #include "intel_crtc.h" > #include "intel_crtc_state_dump.h" > #include "intel_cursor_regs.h" > +#include "intel_cursor.h" > #include "intel_ddi.h" > #include "intel_de.h" > #include "intel_display_driver.h" > @@ -6925,6 +6926,8 @@ static void intel_commit_modeset_disables(struct > intel_atomic_state *state) > continue; > > intel_crtc_disable_planes(state, crtc); > + > + drm_vblank_work_flush_all(&crtc->base); > } > > /* Only disable port sync and MST slaves */ diff --git > a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index 9678c2b157f6..51fa73a2a161 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -735,6 +735,9 @@ struct intel_plane_state { > struct intel_fb_view view; > u32 phys_dma_addr; /* for cursor_needs_physical */ > > + /* for legacy cursor fb unpin */ > + struct drm_vblank_work unpin_work; > + > /* Plane pxp decryption state */ > bool decrypt; > > -- > 2.43.0
