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 03:58:55PM +0200, Luca Coelho wrote:
> On Tue, 2025-12-09 at 15:20 +0200, Ville Syrjälä wrote:
> > 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 was my original thought too, I thought we wouldn't even need to
> upstream this and, only if really needed, we would add it to our
> internal tree.  But I was advised otherwise.

I think if we really need this for some reason then it should be done
at a fairly high level. Something like:
- scaler state compute sets some flag in crtc_state if we need to
  disable dc states
- enable/disable dc states from the {pre,post}_plane_update()
  when the flag changes

But I have a feeling it might all just be dead code we carry around
for a bit and then remove. And not using the programmable coefficients
would be a lot simpler (basically just don't expose the filter props).

> 
> 
> > 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.
> 
> Okay, so any recommendation on how to proceed?

I'd try to find out what happens to the sign bit. If it always
comes back as zero then I don't think we need to do anything.
But if it can be corrupted in the other direction then we
need to deal with it somehow.

-- 
Ville Syrjälä
Intel