On Wed, Jun 03, 2026 at 11:30:04AM +0300, Jani Nikula wrote:
> On Tue, 02 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 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)
> >
> > Changes in v3:
> > - Limit ro mask to read-back comparison.
> >
> > BSpec: 49437,69115
> > Signed-off-by: Dibin Moolakadan Subrahmanian
> > <[email protected]>
> > ---
> > .../i915/display/intel_display_power_well.c | 24 +++++++++++++++----
> > 1 file changed, 20 insertions(+), 4 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..ab0200701a73 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > @@ -726,12 +726,28 @@ 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);
>
> Register contents need to be defined next to the register definition.
>
> But even so, the caller already has the mask we want to change, I
> already suggested passing that in. What's wrong with that?
The mask used by the caller only contains the bits which the driver
changes. However I would like to know that writing all the other bits in
the register also take their effect, reflected by the read-back after the
write.
> BR,
> Jani.
>
>
> > +
> > + return 0;
> > +}
> > +
> > static void gen9_write_dc_state(struct intel_display *display,
> > u32 state)
> > {
> > int rewrites = 0;
> > int rereads = 0;
> > u32 v;
> > + /*
> > + * Mask out RO status bits from read-back comparison.
> > + * HW may set these bits independently, so exclude them
> > + * to prevent the verify loop from retrying due to RO bits mismatch.
> > + */
> > + u32 ro_mask = dc_state_ro_mask(display);
> >
> > intel_de_write(display, DC_STATE_EN, state);
> >
> > @@ -743,7 +759,7 @@ static void gen9_write_dc_state(struct intel_display
> > *display,
> > do {
> > v = intel_de_read(display, DC_STATE_EN);
> >
> > - if (v != state) {
> > + if ((v & ~ro_mask) != (state & ~ro_mask)) {
> > intel_de_write(display, DC_STATE_EN, state);
> > rewrites++;
> > rereads = 0;
> > @@ -753,10 +769,10 @@ static void gen9_write_dc_state(struct intel_display
> > *display,
> >
> > } while (rewrites < 100);
> >
> > - if (v != state)
> > + if ((v & ~ro_mask) != (state & ~ro_mask))
> > drm_err(display->drm,
> > - "Writing dc state to 0x%x failed, now 0x%x\n",
> > - state, v);
> > + "Writing dc state to 0x%x failed, now 0x%x
> > (ro_mask=0x%x)\n",
> > + state, v, ro_mask);
> >
> > /* Most of the times we need one retry, avoid spam */
> > if (rewrites > 1)
>
> --
> Jani Nikula, Intel
