Re: [PATCH] drm/i915/xe3p: add W/A to disable DC5/DC6 in certain scaler conditions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 09, 2025 at 02:54:42PM +0200, Luca Coelho wrote:
> In NVL-A0, a workaround is needed to prevent scaling problems when
> using scaler coefficients with DC5 and DC6.  In order to avoid this,
> the workaround needs to prevent the device from entering DC5 or DC6
> when programmable scaler coefficients are used.
> 
> Check for these conditions and hold a DC_OFF power domain wakeref to
> prevent entering DC5 and DC6 in these situations.
> 
> This patch implements Wa_16026694205.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 10 ++++++
>  .../drm/i915/display/intel_display_types.h    |  7 ++++
>  .../gpu/drm/i915/display/intel_display_wa.c   |  4 +++
>  .../gpu/drm/i915/display/intel_display_wa.h   |  1 +
>  drivers/gpu/drm/i915/display/skl_scaler.c     | 35 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/skl_scaler.h     |  5 +++
>  6 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 9c6d3ecdb589..c3b19dd2ac56 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7299,6 +7299,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  	struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	unsigned int size = new_crtc_state->plane_color_changed ? 8192 : 1024;
> +	u32 ps_ctrl;
> +	int i;
>  
>  	if (!new_crtc_state->use_flipq &&
>  	    !new_crtc_state->use_dsb &&
> @@ -7384,6 +7386,14 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  	}
>  
>  	intel_dsb_finish(new_crtc_state->dsb_commit);
> +
> +	/* Wa_16026694205: check and re-enable DC5 if needed */
> +	for (i = 0; i < crtc->num_scalers; i++) {
> +		ps_ctrl = intel_de_read(display, SKL_PS_CTRL(crtc->pipe, i));
> +		if (intel_display_wa(display, 16026694205))
> +			wa_no_dc5_if_ps_filter_programmed(display, crtc,
> +							  ps_ctrl, false);
> +	}
>  }
>  
>  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 6ff53cd58052..8d4dbe46fa26 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1545,6 +1545,13 @@ struct intel_crtc {
>  	/* scalers available on this crtc */
>  	int num_scalers;
>  
> +	/*
> +	 * wakeref for Wa_16026694205 where we need to prevent DC5/DC6
> +	 * when using scaler coefficients (PS_CTRL_FILTER_SELECT is
> +	 * programmed).
> +	 */
> +	struct ref_tracker *wa_no_dc5_wakeref;
> +
>  	/* for loading single buffered registers during vblank */
>  	struct pm_qos_request vblank_pm_qos;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c b/drivers/gpu/drm/i915/display/intel_display_wa.c
> index a00af39f7538..9e4de69f4d58 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
> @@ -10,6 +10,7 @@
>  #include "intel_display_core.h"
>  #include "intel_display_regs.h"
>  #include "intel_display_wa.h"
> +#include "intel_step.h"
>  
>  static void gen11_display_wa_apply(struct intel_display *display)
>  {
> @@ -74,6 +75,9 @@ bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa,
>  		return display->platform.battlemage;
>  	case INTEL_DISPLAY_WA_14025769978:
>  		return DISPLAY_VER(display) == 35;
> +	case INTEL_DISPLAY_WA_16026694205:
> +		return (DISPLAY_VER(display) == 35 &&
> +			IS_DISPLAY_STEP(display, STEP_A0, STEP_A0));

This looks like a lot of  code to deal with a single broken
pre-production stepping. Assuming this is going to get fixed in
the hardware later (which seems to be the case according to the
HSD), then IMO we should simply not use the programmed coefficients
on that broken stepping.

That's assuming that we even care about this. The HSD fails to explain
what the lack of the retention will do to the sign bit. If it's simply
going to get reset to 0 then it'll be fine since we never used negative
coefficients anyway.

-- 
Ville Syrjälä
Intel