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

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

On 01-06-2026 17:17, Imre Deak wrote:
> On Mon, Jun 01, 2026 at 12:38:31PM +0300, Jani Nikula wrote:
>> On Mon, 01 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 both the write value and 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)
>>>
>>> BSpec: 49437,69115
>>> Signed-off-by: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>
>>> ---
>>>   .../i915/display/intel_display_power_well.c   | 29 +++++++++++++++----
>>>   1 file changed, 24 insertions(+), 5 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..4bc9e3ef738e 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>>> @@ -726,14 +726,33 @@ 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);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static void gen9_write_dc_state(struct intel_display *display,
>>>   				u32 state)
>> The caller already has the platform specific mask, please just pass that
>> in and use it.
> Imo it's better to read-back verify all the bits written to the
> DC_STATE_EN register, not only those that the caller changed. The only
> exception should be the bits that are expected to be changed by the
> firmware randomly (the read-only bits).
>
>> BR,
>> Jani.
>>
>>>   {
>>>   	int rewrites = 0;
>>>   	int rereads = 0;
>>>   	u32 v;
>>> +	u32 ro_mask = dc_state_ro_mask(display);
>>> +	u32 val = state;
>>> +
>>> +	/*
>>> +	 * Mask out RO status bits from both the write value and the read-back
>>> +	 * comparison. HW may set these bits independently, so exclude them
>>> +	 * to prevent the verify loop from retrying due to RO bits mismatch.
>>> +	 */
>>> +	val &= ~ro_mask;
>>>   
>>> -	intel_de_write(display, DC_STATE_EN, state);
>>> +	intel_de_write(display, DC_STATE_EN, val);

Thanks Jani and Imre for the review.

> I would not change what is written to the register, rather use the mask
> only for comparison.

Agreed, will use mask only for comparison.

>>>   
>>>   	/* It has been observed that disabling the dc6 state sometimes
>>>   	 * doesn't stick and dmc keeps returning old value. Make sure
>>> @@ -741,10 +760,10 @@ static void gen9_write_dc_state(struct intel_display *display,
>>>   	 * we are confident that state is exactly what we want.
>>>   	 */
>>>   	do  {
>>> -		v = intel_de_read(display, DC_STATE_EN);
>>> +		v = intel_de_read(display, DC_STATE_EN) & ~ro_mask;
>>>   
>>> -		if (v != state) {
> I.e. here instead of the above masking:
>
>    if (v & ~ro_mask != state & ~ro_mask)
>
>>> -			intel_de_write(display, DC_STATE_EN, state);
>>> +		if (v != val) {
>>> +			intel_de_write(display, DC_STATE_EN, val);
>>>   			rewrites++;
>>>   			rereads = 0;
>>>   		} else if (rereads++ > 5) {
>>> @@ -753,7 +772,7 @@ static void gen9_write_dc_state(struct intel_display *display,
>>>   
>>>   	} while (rewrites < 100);
>>>   
>>> -	if (v != state)
>>> +	if (v != val)
>>>   		drm_err(display->drm,
>>>   			"Writing dc state to 0x%x failed, now 0x%x\n",
> This could also print the ro_mask.

I will add ro_mask to print in the next version.

>>>   			state, v);
>> -- 
>> Jani Nikula, Intel