[PATCH v2] drm/i915/display: Limit SEL_FETCH clear to supported pipes

[PATCH v2] drm/i915/display: Limit SEL_FETCH clear to supported pipes

[Nemesa Garg]

Commit 7f1172a2ac0d ("drm/i915/display: Clear SEL_FETCH_PLANE_CTL
on plane disable") started clearing SEL_FETCH_PLANE_CTL and
SEL_FETCH_CUR_CTL on every plane and cursor disable.

Not all pipes implement these registers. Clearing them on a pipe
that does not have them leads to an unclaimed register access.

Skip the clear on pipes that do not implement the registers.

Fixes: 7f1172a2ac0d ("drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane disable")
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16876
Assisted-by: GitHub-Copilot:claude-opus-4.6
Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cursor.c   | 12 ++------
 drivers/gpu/drm/i915/display/intel_psr.c      | 30 +++++++++++++++++++
 .../drm/i915/display/skl_universal_plane.c    | 12 ++------
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index cce041e1da51..aa662b2cdf42 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -536,13 +536,7 @@ static void i9xx_cursor_disable_sel_fetch_arm(struct intel_dsb *dsb,
 	struct intel_display *display = to_intel_display(plane);
 	enum pipe pipe = plane->pipe;
 
-	/*
-	 * Clear this whenever the hardware has selective fetch, not just when
-	 * the current state uses it. The cursor may have been enabled with
-	 * selective fetch earlier and had its enable bit orphaned when the
-	 * feature was switched off.
-	 */
-	if (!HAS_PSR2_SEL_FETCH(display))
+	if (!crtc_state->enable_psr2_sel_fetch)
 		return;
 
 	intel_de_write_dsb(display, dsb, SEL_FETCH_CUR_CTL(pipe), 0);
@@ -592,7 +586,7 @@ static void i9xx_cursor_update_sel_fetch_arm(struct intel_dsb *dsb,
 		if (crtc_state->enable_psr2_su_region_et)
 			wa_16021440873(dsb, plane, crtc_state, plane_state);
 		else
-			i9xx_cursor_disable_sel_fetch_arm(dsb, plane);
+			i9xx_cursor_disable_sel_fetch_arm(dsb, plane, crtc_state);
 	}
 }
 
@@ -701,7 +695,7 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
 	if (plane_state)
 		i9xx_cursor_update_sel_fetch_arm(dsb, plane, crtc_state, plane_state);
 	else
-		i9xx_cursor_disable_sel_fetch_arm(dsb, plane);
+		i9xx_cursor_disable_sel_fetch_arm(dsb, plane, crtc_state);
 
 	if (plane->cursor.base != base ||
 	    plane->cursor.size != fbc_ctl ||
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index f490beb66629..702d4b6d62a4 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -58,6 +58,7 @@
 #include "intel_vdsc.h"
 #include "intel_vrr.h"
 #include "skl_universal_plane.h"
+#include "skl_universal_plane_regs.h"
 
 /**
  * DOC: Panel Self Refresh (PSR/SRD)
@@ -2162,6 +2163,33 @@ static bool psr_interrupt_error_check(struct intel_dp *intel_dp)
 	return true;
 }
 
+/*
+ * A plane disabled while selective fetch was off keeps its selective fetch
+ * enable bit set in hardware. The bit does nothing until selective fetch is
+ * turned back on, at which point the hardware would resume fetching for a
+ * plane that is no longer enabled and keep its DDB range reserved. Drop the
+ * bit for every inactive plane as selective fetch is enabled. The active
+ * planes are programmed by the selective fetch update that follows.
+ */
+static void psr2_sel_fetch_clear_inactive_planes(struct intel_display *display,
+						 const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	enum pipe pipe = crtc->pipe;
+	struct intel_plane *plane;
+
+	for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
+		if (crtc_state->active_planes & BIT(plane->id))
+			continue;
+
+		if (plane->id == PLANE_CURSOR)
+			intel_de_write(display, SEL_FETCH_CUR_CTL(pipe), 0);
+		else
+			intel_de_write(display,
+				       SEL_FETCH_PLANE_CTL(pipe, plane->id), 0);
+	}
+}
+
 static void intel_psr_enable_locked(struct intel_dp *intel_dp,
 				    const struct intel_crtc_state *crtc_state)
 {
@@ -2180,6 +2208,8 @@ static void intel_psr_enable_locked(struct intel_dp *intel_dp,
 	val = usecs_to_jiffies(intel_get_frame_time_us(crtc_state) * 6);
 	intel_dp->psr.dc3co_exit_delay = val;
 	intel_dp->psr.psr2_sel_fetch_enabled = crtc_state->enable_psr2_sel_fetch;
+	if (crtc_state->enable_psr2_sel_fetch)
+		psr2_sel_fetch_clear_inactive_planes(display, crtc_state);
 	intel_dp->psr.su_region_et_enabled = crtc_state->enable_psr2_su_region_et;
 	intel_dp->psr.psr2_sel_fetch_cff_enabled = false;
 	intel_dp->psr.req_psr2_sdp_prior_scanline =
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 5cda1ab90e40..a7e2f44ebfca 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -884,13 +884,7 @@ static void icl_plane_disable_sel_fetch_arm(struct intel_dsb *dsb,
 	struct intel_display *display = to_intel_display(plane);
 	enum pipe pipe = plane->pipe;
 
-	/*
-	 * Clear this whenever the hardware has selective fetch, not just when
-	 * the current state uses it. The plane may have been enabled with
-	 * selective fetch earlier and had its enable bit orphaned when the
-	 * feature was switched off.
-	 */
-	if (!HAS_PSR2_SEL_FETCH(display))
+	if (!crtc_state->enable_psr2_sel_fetch)
 		return;
 
 	intel_de_write_dsb(display, dsb, SEL_FETCH_PLANE_CTL(pipe, plane->id), 0);
@@ -926,7 +920,7 @@ icl_plane_disable_arm(struct intel_dsb *dsb,
 
 	skl_write_plane_wm(dsb, plane, crtc_state);
 
-	icl_plane_disable_sel_fetch_arm(dsb, plane);
+	icl_plane_disable_sel_fetch_arm(dsb, plane, crtc_state);
 
 	if (plane_has_normalizer(plane))
 		intel_de_write_dsb(display, dsb,
@@ -1646,7 +1640,7 @@ static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,
 		intel_de_write_dsb(display, dsb, SEL_FETCH_PLANE_CTL(pipe, plane->id),
 				   SEL_FETCH_PLANE_CTL_ENABLE);
 	else
-		icl_plane_disable_sel_fetch_arm(dsb, plane);
+		icl_plane_disable_sel_fetch_arm(dsb, plane, crtc_state);
 }
 
 static void
-- 
2.25.1

Re: [PATCH v2] drm/i915/display: Limit SEL_FETCH clear to supported pipes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces a build failure by using an undeclared variable `crtc_state` in function bodies without updating their signatures.
--

commit abf33fb81864e3a646619421c587c1bc7e6b1f3d
Author: Nemesa Garg <nemesa.garg@intel.com>

drm/i915/display: Limit SEL_FETCH clear to supported pipes

This commit limits the clearing of selective fetch registers to pipes that
actually support them. It avoids unclaimed register access warnings by
checking the selective fetch enable state before clearing.

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16876

> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index cce041e1da51d..aa662b2cdf42b 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -536,13 +536,7 @@ static void i9xx_cursor_disable_sel_fetch_arm(struct intel_dsb *dsb,
>  	struct intel_display *display = to_intel_display(plane);
>  	enum pipe pipe = plane->pipe;
>  
> -	/*
> -	 * Clear this whenever the hardware has selective fetch, not just when
> -	 * the current state uses it. The cursor may have been enabled with
> -	 * selective fetch earlier and had its enable bit orphaned when the
> -	 * feature was switched off.
> -	 */
> -	if (!HAS_PSR2_SEL_FETCH(display))
> +	if (!crtc_state->enable_psr2_sel_fetch)

[Severity: Low]
Does this cause a build failure? The variable crtc_state is used here but
isn't declared in the function signature for
i9xx_cursor_disable_sel_fetch_arm(). While the callers were updated to pass
crtc_state as a third argument, the function signature itself wasn't updated.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 5cda1ab90e40f..a7e2f44ebfca6 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -884,13 +884,7 @@ static void icl_plane_disable_sel_fetch_arm(struct intel_dsb *dsb,
>  	struct intel_display *display = to_intel_display(plane);
>  	enum pipe pipe = plane->pipe;
>  
> -	/*
> -	 * Clear this whenever the hardware has selective fetch, not just when
> -	 * the current state uses it. The plane may have been enabled with
> -	 * selective fetch earlier and had its enable bit orphaned when the
> -	 * feature was switched off.
> -	 */
> -	if (!HAS_PSR2_SEL_FETCH(display))
> +	if (!crtc_state->enable_psr2_sel_fetch)

[Severity: Low]
Does this also cause a build failure? Similar to the cursor code above, the
function signature for icl_plane_disable_sel_fetch_arm() is missing the
crtc_state parameter, leading to an undeclared identifier error here and a
"too many arguments" error at the callers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907042142.3114009-1-nemesa.garg@intel.com?part=1