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 <[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.
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
