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
> <[email protected]>
> ---
> .../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
>
