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