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

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

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.


>  			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
>