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 <[email protected]> > > --- > > 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. > 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? -- Cheers, Luca.
