[Intel-gfx] [PATCH] drm/i915: Reorder skl+ scaler vs. plane updates

* [Intel-gfx] [PATCH] drm/i915: Reorder skl+ scaler vs. plane updates
@ 2021-05-06  7:38 Ville Syrjala
  2021-05-06  8:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ville Syrjala @ 2021-05-06  7:38 UTC (permalink / raw)
  To: intel-gfx; +Cc: Cooper Chiou

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When scanning out NV12 if we at any time have the plane enabled
while the scaler is disabled we get a pretty catastrophics
underrun.

Let's reorder the operations so that we try to avoid that happening
even if our vblank evade fails and the scaler enable/disable and
the plane enable/disable get latched during two diffent frames.

This takes care of the most common cases. I suppose there is still
at least a theoretical possibility of hitting this if one plane
takes the scaler away from another plane before the second plane
had a chance to set up another scaler for its use. But that
is starting to get a bit complicated, especially since the plane
commit order already has to be carefully sequenced to avoid any
dbuf overlaps. So plugging this 100% may prove somewhat hard...

Cc: Cooper Chiou <cooper.chiou@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 30 ++++++++++++++-----
 .../drm/i915/display/skl_universal_plane.c    | 11 +++++--
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index fcd8123ede8e..0c8ca26156b1 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -9698,8 +9698,6 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
 
 	/* on skylake this is done by detaching scalers */
 	if (DISPLAY_VER(dev_priv) >= 9) {
-		skl_detach_scalers(new_crtc_state);
-
 		if (new_crtc_state->pch_pfit.enabled)
 			skl_pfit_enable(new_crtc_state);
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
@@ -9725,8 +9723,8 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
 		icl_set_pipe_chicken(crtc);
 }
 
-static void commit_pipe_config(struct intel_atomic_state *state,
-			       struct intel_crtc *crtc)
+static void commit_pipe_pre_planes(struct intel_atomic_state *state,
+				   struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	const struct intel_crtc_state *old_crtc_state =
@@ -9744,9 +9742,6 @@ static void commit_pipe_config(struct intel_atomic_state *state,
 		    new_crtc_state->update_pipe)
 			intel_color_commit(new_crtc_state);
 
-		if (DISPLAY_VER(dev_priv) >= 9)
-			skl_detach_scalers(new_crtc_state);
-
 		if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
 			bdw_set_pipemisc(new_crtc_state);
 
@@ -9760,6 +9755,23 @@ static void commit_pipe_config(struct intel_atomic_state *state,
 		dev_priv->display.atomic_update_watermarks(state, crtc);
 }
 
+static void commit_pipe_post_planes(struct intel_atomic_state *state,
+				    struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+
+	/*
+	 * Disable the scaler(s) after the plane(s) so that we don't
+	 * get a catastrophic underrun even if the two operations
+	 * end up happening in two different frames.
+	 */
+	if (DISPLAY_VER(dev_priv) >= 9 &&
+	    !intel_crtc_needs_modeset(new_crtc_state))
+		skl_detach_scalers(new_crtc_state);
+}
+
 static void intel_enable_crtc(struct intel_atomic_state *state,
 			      struct intel_crtc *crtc)
 {
@@ -9811,13 +9823,15 @@ static void intel_update_crtc(struct intel_atomic_state *state,
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(new_crtc_state);
 
-	commit_pipe_config(state, crtc);
+	commit_pipe_pre_planes(state, crtc);
 
 	if (DISPLAY_VER(dev_priv) >= 9)
 		skl_update_planes_on_crtc(state, crtc);
 	else
 		i9xx_update_planes_on_crtc(state, crtc);
 
+	commit_pipe_post_planes(state, crtc);
+
 	intel_pipe_update_end(new_crtc_state);
 
 	/*
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 0d34a5ad4e2b..6ad85d7cb219 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1032,6 +1032,14 @@ skl_program_plane(struct intel_plane *plane,
 	if (!drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
 		intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane);
 
+	/*
+	 * Enable the scaler before the plane so that we don't
+	 * get a catastrophic underrun even if the two operations
+	 * end up happening in two different frames.
+	 */
+	if (plane_state->scaler_id >= 0)
+		skl_program_plane_scaler(plane, crtc_state, plane_state);
+
 	/*
 	 * The control register self-arms if the plane was previously
 	 * disabled. Try to make the plane enable atomic by writing
@@ -1041,9 +1049,6 @@ skl_program_plane(struct intel_plane *plane,
 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
 			  intel_plane_ggtt_offset(plane_state) + surf_addr);
 
-	if (plane_state->scaler_id >= 0)
-		skl_program_plane_scaler(plane, crtc_state, plane_state);
-
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-- 
2.26.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread