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

[Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>]

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.

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