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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx