diff mbox series

drm/i915: Reorder skl+ scaler vs. plane updates

Message ID 20210506073836.14848-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Reorder skl+ scaler vs. plane updates | expand

Commit Message

Ville Syrjälä May 6, 2021, 7:38 a.m. UTC 
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(-)

Comments

Lisovskiy, Stanislav May 6, 2021, 8:47 a.m. UTC | #1 
On Thu, May 06, 2021 at 10:38:36AM +0300, Ville Syrjala wrote:
> 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. 

Just curious, how this is possible? Shouldn't the scaler be already 
marked "in_use" if another plane uses it, so we can't start using
it until it is detached?

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>


> 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
Ville Syrjälä May 6, 2021, 10:42 a.m. UTC | #2 
On Thu, May 06, 2021 at 11:47:08AM +0300, Lisovskiy, Stanislav wrote:
> On Thu, May 06, 2021 at 10:38:36AM +0300, Ville Syrjala wrote:
> > 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. 
> 
> Just curious, how this is possible? Shouldn't the scaler be already 
> marked "in_use" if another plane uses it, so we can't start using
> it until it is detached?

The commit is supposed to be atomic so we can shuffle the scalers
around as much as we want, so long as we don't exceed the number
of availalbe scalers.

So we could have a commit that does this for example:
 plane 1: disabled -> scaler 0
 plane 2: scaler 0 -> disabled
assuming the planes get committed in order, plane 1 grabs the
scaler from plane 2 before plane 2 gets turned off.

Simple cases like that could be handled by committing the
planes in a different order (assuming the dbuf constraints
allow it), but something like
 plane 1: scaler 0 -> scaler 1
 plane 2: scaler 1 -> scaler 0
would pose more of a challenge.

> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> 
> > 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
diff mbox series

Patch

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);
 }