On Thu, May 07, 2026 at 11:17:36AM +0530, Dibin Moolakadan Subrahmanian wrote:
>
> On 06-05-2026 19:05, Imre Deak wrote:
> > On Wed, May 06, 2026 at 06:33:21PM +0530, Dibin Moolakadan Subrahmanian
> > wrote:
> > > gen9_write_dc_state() verifies DC_STATE_EN by reading it back, but it
> > > was comparing the full register value instead of only the DC state bits.
> > > That could trigger false failure messages and unnecessary retries when
> > > unrelated bits differed.
> > >
> > > Use intel_de_rmw() to update only the DC state bits and compare only
> > > the masked DC state bits in the read-back check and retry logic.
> > >
> > > BSpec: 49437,69115
> > > Signed-off-by: Dibin Moolakadan Subrahmanian
> > > <[email protected]>
> > > ---
> > > .../i915/display/intel_display_power_well.c | 21 ++++++++-----------
> > > 1 file changed, 9 insertions(+), 12 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 6fbfd46461b0..75471898e323 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > @@ -727,13 +727,13 @@ static void assert_can_disable_dc9(struct
> > > intel_display *display)
> > > }
> > > static void gen9_write_dc_state(struct intel_display *display,
> > > - u32 state)
> > > + u32 state, u32 mask)
> > > {
> > > int rewrites = 0;
> > > int rereads = 0;
> > > u32 v;
> > > - intel_de_write(display, DC_STATE_EN, state);
> > > + intel_de_rmw(display, DC_STATE_EN, mask, state);
> > There is no need to change this to an RMW, since gen9_set_dc_state()
> > computed the state passed to this function by the equivalent
> >
> > (intel_de_read(display, DC_STATE_EN) & ~mask) | state
> >
> > > /* It has been observed that disabling the dc6 state sometimes
> > > * doesn't stick and dmc keeps returning old value. Make sure
> > > @@ -742,9 +742,8 @@ static void gen9_write_dc_state(struct intel_display
> > > *display,
> > > */
> > > do {
> > > v = intel_de_read(display, DC_STATE_EN);
> > > -
> > > - if (v != state) {
> > > - intel_de_write(display, DC_STATE_EN, state);
> > > + if ((v & mask) != (state & mask)) {
> > > + intel_de_rmw(display, DC_STATE_EN, mask, state);
> >
> > Could you provide the flags in the register causing an unexpected
> > mismatch? I can only see bits that should preserve their state as
> > written by the driver. The register has also some clear-on-write flags,
> > like 'Display DC*CO State Status DSI', but not sure how even those can
> > lead to a mismatch.
> >
> Thanks for the review. I can see some RO bits status getting changed
> while read back. In one case, I can see register read back value as
> 0x400 while writing DC state 0. In this case I can see DC state is
> 0,but other bit have changed.
I think there is no good reason to ignore spurious changes to any of the
non-RO flags. So I'd only mask out RO flags here before the comparison.
> > > rewrites++;
> > > rereads = 0;
> > > } else if (rereads++ > 5) {
> > > @@ -753,16 +752,16 @@ static void gen9_write_dc_state(struct
> > > intel_display *display,
> > > } while (rewrites < 100);
> > > - if (v != state)
> > > + if ((v & mask) != (state & mask))
> > > drm_err(display->drm,
> > > "Writing dc state to 0x%x failed, now 0x%x\n",
> > > - state, v);
> > > + state & mask, v & mask);
> > > /* Most of the times we need one retry, avoid spam */
> > > if (rewrites > 1)
> > > drm_dbg_kms(display->drm,
> > > "Rewrote dc state to 0x%x %d times\n",
> > > - state, rewrites);
> > > + state & mask, rewrites);
> > > }
> > > static u32 gen9_dc_mask(struct intel_display *display)
> > > @@ -855,15 +854,13 @@ void gen9_set_dc_state(struct intel_display
> > > *display, u32 state)
> > > if (!dc6_was_enabled && enable_dc6)
> > > intel_dmc_update_dc6_allowed_count(display, true);
> > > - val &= ~mask;
> > > - val |= state;
> > > - gen9_write_dc_state(display, val);
> > > + gen9_write_dc_state(display, state, mask);
> > > if (!enable_dc6 && dc6_was_enabled)
> > > intel_dmc_update_dc6_allowed_count(display, false);
> > > - power_domains->dc_state = val & mask;
> > > + power_domains->dc_state = state & mask;
> > > }
> > > static void tgl_enable_dc3co(struct intel_display *display)
> > > --
> > > 2.43.0
> > >
