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

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Thursday, November 9, 2023 9:05 PM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Use vblank worker to unpin old legacy
> cursor fb safely
> 
> From: Ville Syrjälä <ville.syrjala at 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.
> 
> This patch is slightly reworked by Maarten
> 
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c   | 37 ++++++++++++++++---
>  drivers/gpu/drm/i915/display/intel_display.c  |  3 ++
>  .../drm/i915/display/intel_display_types.h    |  3 ++
>  3 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index b342fad180ca5..1728ecd5cc782 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -603,6 +603,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,
> @@ -688,9 +699,14 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  	if (ret)
>  		goto out_free;
> 
> -	ret = intel_plane_pin_fb(new_plane_state);
> -	if (ret)
> -		goto out_free;
> +	if (new_plane_state->uapi.fb != old_plane_state->uapi.fb) {
> +		ret = intel_plane_pin_fb(new_plane_state);
> +		if (ret)
> +			goto out_free;
> +	} else {
> +		/* magic trick */

Please add some comment on the rationale for change

> +		swap(new_plane_state->ggtt_vma, old_plane_state->ggtt_vma);
> +	}
> 
>  	intel_frontbuffer_flush(to_intel_frontbuffer(new_plane_state->hw.fb),
>  				ORIGIN_CURSOR_UPDATE);
> @@ -730,14 +746,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> 
>  	local_irq_enable();
> 
> -	intel_plane_unpin_fb(old_plane_state);
> +	if (old_plane_state->uapi.fb != new_plane_state->uapi.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);

Could it not cause an issue with reference counts, here we are invoking the
unpin_fb but its respective pin call is replaced with a gggtt_vma swap operation.
intel_unpin_fb_vma will get called which will try to lower the ref count.

> +	}
> 
>  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 3effafcbb411a..9b43810f9a934 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -64,6 +64,7 @@
>  #include "intel_crt.h"
>  #include "intel_crtc.h"
>  #include "intel_crtc_state_dump.h"
> +#include "intel_cursor.h"

This can be dropped.

>  #include "intel_ddi.h"
>  #include "intel_de.h"
>  #include "intel_display_driver.h"
> @@ -6883,6 +6884,8 @@ static void intel_commit_modeset_disables(struct
> intel_atomic_state *state)
> 
>  		intel_pre_plane_update(state, crtc);
>  		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 926bf9c1a3ede..6d27cfb4f08e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -709,6 +709,9 @@ struct intel_plane_state {
> 
>  	struct intel_fb_view view;
> 
> +	/* for legacy cursor fb unpin */
> +	struct drm_vblank_work unpin_work;
> +
>  	/* Plane pxp decryption state */
>  	bool decrypt;
> 
> --
> 2.39.2