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

[Juha-Pekka Heikkila]

I didn't spot anything to nag about. Just hope that warning doesn't 
become excessively noisy. These two patches are

Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>

On 4.9.2023 7.16, Ville Syrjala wrote:
> 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.
> 
> v2: wait for cursor unpins before turning off the vblank irq
> 
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c   | 36 +++++++++++++++++--
>   drivers/gpu/drm/i915/display/intel_cursor.h   |  2 ++
>   drivers/gpu/drm/i915/display/intel_display.c  |  3 ++
>   .../drm/i915/display/intel_display_types.h    |  4 +++
>   4 files changed, 43 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..625540fd1dab 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -603,6 +603,26 @@ 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);
> +
> +	if (atomic_dec_and_test(&plane->cursor.pending_unpins))
> +		wake_up_var(&plane->cursor.pending_unpins);
> +}
> +
> +void intel_cursor_wait_unpin_works(struct intel_plane *plane)
> +{
> +	wait_var_event(&plane->cursor.pending_unpins,
> +		       !atomic_read(&plane->cursor.pending_unpins));
> +}
> +
>   static int
>   intel_legacy_cursor_update(struct drm_plane *_plane,
>   			   struct drm_crtc *_crtc,
> @@ -730,14 +750,26 @@ 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);
> +
> +		atomic_inc(&plane->cursor.pending_unpins);
> +		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_cursor.h b/drivers/gpu/drm/i915/display/intel_cursor.h
> index ce333bf4c2d5..e778aff77129 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.h
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.h
> @@ -14,4 +14,6 @@ struct intel_plane *
>   intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>   			  enum pipe pipe);
>   
> +void intel_cursor_wait_unpin_works(struct intel_plane *plane);
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f6397462e4c2..90c1ed61ba0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -63,6 +63,7 @@
>   #include "intel_crt.h"
>   #include "intel_crtc.h"
>   #include "intel_crtc_state_dump.h"
> +#include "intel_cursor.h"
>   #include "intel_ddi.h"
>   #include "intel_de.h"
>   #include "intel_display_driver.h"
> @@ -6618,6 +6619,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>   
>   		intel_pre_plane_update(state, crtc);
>   		intel_crtc_disable_planes(state, crtc);
> +
> +		intel_cursor_wait_unpin_works(to_intel_plane(crtc->base.cursor));
>   	}
>   
>   	/* 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 c21064794f32..f6ca86f1d834 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -702,6 +702,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;
>   
> @@ -1515,6 +1518,7 @@ struct intel_plane {
>   
>   	struct {
>   		u32 base, cntl, size;
> +		atomic_t pending_unpins;
>   	} cursor;
>   
>   	struct intel_fbc *fbc;