Re: [PATCH] drm/i915/display: Use rmw in gen9_write_dc_state() to preserve non-DC bits

[Imre Deak <imre.deak@intel.com>]

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 <dibin.moolakadan.subrahmanian@intel.com>
> > > ---
> > >   .../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
> > >