> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Maarten
> Lankhorst
> Sent: Wednesday, May 22, 2024 11:04 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>;
> Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Subject: [v6 2/3] drm/i915: Use vblank worker to unpin old legacy cursor fb safely
>
> 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.
>
> This patch is slightly reworked by Maarten
Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> 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
