On Mon, Jun 01, 2026 at 12:38:31PM +0300, Jani Nikula wrote:
> On Mon, 01 Jun 2026, Dibin Moolakadan Subrahmanian
> <[email protected]> wrote:
> > The DC_STATE_EN register has read-only status bits that are set by
> > hardware on some platforms. These bits may cause the read-back
> > verification loop in gen9_write_dc_state() to spuriously retry.
> >
> > Mask the RO bits from both the write value and the read-back comparison
> > to prevent unnecessary retries.
> >
> > Changes in v2:
> > - Rename patch from
> > "drm/i915/display: Use rmw in gen9_write_dc_state() to preserve non-DC
> > bits"
> > to
> > "drm/i915/display: Mask RO bits in gen9_write_dc_state()"
> > - Mask only RO bits rather than masking all non DC state bits
> > in DC_STATE_EN. As the register has also some clear-on-write flags,
> > like 'Display DC*CO State Status DSI'(Imre Deak)
> >
> > BSpec: 49437,69115
> > Signed-off-by: Dibin Moolakadan Subrahmanian
> > <[email protected]>
> > ---
> > .../i915/display/intel_display_power_well.c | 29 +++++++++++++++----
> > 1 file changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > index 04bd0dde5bed..4bc9e3ef738e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > @@ -726,14 +726,33 @@ static void assert_can_disable_dc9(struct
> > intel_display *display)
> > */
> > }
> >
> > +static u32 dc_state_ro_mask(struct intel_display *display)
> > +{
> > + if (DISPLAY_VER(display) >= 20)
> > + return BIT(10) | BIT(11);
> > + else if (DISPLAY_VER(display) >= 13 && !display->platform.dg2)
> > + return BIT(10);
> > +
> > + return 0;
> > +}
> > +
> > static void gen9_write_dc_state(struct intel_display *display,
> > u32 state)
>
> The caller already has the platform specific mask, please just pass that
> in and use it.
Imo it's better to read-back verify all the bits written to the
DC_STATE_EN register, not only those that the caller changed. The only
exception should be the bits that are expected to be changed by the
firmware randomly (the read-only bits).
> BR,
> Jani.
>
> > {
> > int rewrites = 0;
> > int rereads = 0;
> > u32 v;
> > + u32 ro_mask = dc_state_ro_mask(display);
> > + u32 val = state;
> > +
> > + /*
> > + * Mask out RO status bits from both the write value and the read-back
> > + * comparison. HW may set these bits independently, so exclude them
> > + * to prevent the verify loop from retrying due to RO bits mismatch.
> > + */
> > + val &= ~ro_mask;
> >
> > - intel_de_write(display, DC_STATE_EN, state);
> > + intel_de_write(display, DC_STATE_EN, val);
I would not change what is written to the register, rather use the mask
only for comparison.
> >
> > /* It has been observed that disabling the dc6 state sometimes
> > * doesn't stick and dmc keeps returning old value. Make sure
> > @@ -741,10 +760,10 @@ static void gen9_write_dc_state(struct intel_display
> > *display,
> > * we are confident that state is exactly what we want.
> > */
> > do {
> > - v = intel_de_read(display, DC_STATE_EN);
> > + v = intel_de_read(display, DC_STATE_EN) & ~ro_mask;
> >
> > - if (v != state) {
I.e. here instead of the above masking:
if (v & ~ro_mask != state & ~ro_mask)
> > - intel_de_write(display, DC_STATE_EN, state);
> > + if (v != val) {
> > + intel_de_write(display, DC_STATE_EN, val);
> > rewrites++;
> > rereads = 0;
> > } else if (rereads++ > 5) {
> > @@ -753,7 +772,7 @@ static void gen9_write_dc_state(struct intel_display
> > *display,
> >
> > } while (rewrites < 100);
> >
> > - if (v != state)
> > + if (v != val)
> > drm_err(display->drm,
> > "Writing dc state to 0x%x failed, now 0x%x\n",
This could also print the ro_mask.
> > state, v);
>
> --
> Jani Nikula, Intel
