Re: [PATCH v3] drm/i915/display: Mask RO bits in gen9_write_dc_state()

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

On Wed, Jun 03, 2026 at 11:30:04AM +0300, Jani Nikula wrote:
> On Tue, 02 Jun 2026, Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com> 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 <dibin.moolakadan.subrahmanian@intel.com>
> > ---
> >  .../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