diff mbox series

[v6,2/3] drm/i915: Use vblank worker to unpin old legacy cursor fb safely

Message ID 20240522053341.137592-3-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Fix cursor FB unpinning. | expand

Commit Message

Maarten Lankhorst May 22, 2024, 5:33 a.m. UTC
From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 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(-)

Comments

Shankar, Uma May 28, 2024, 7:51 a.m. UTC | #1
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Wednesday, May 22, 2024 11:04 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org; Ville Syrjälä <ville.syrjala@linux.intel.com>;
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Subject: [v6 2/3] drm/i915: Use vblank worker to unpin old legacy cursor fb safely
> 
> From: Ville Syrjälä <ville.syrjala@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

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  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
diff mbox series

Patch

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;