Re: [PATCH 1/2] Revert "drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane disable"

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2026-09-09 at 11:55 +0530, Nemesa Garg wrote:
> This reverts commit 7f1172a2ac0d7e50850785e2e65789c8aac8411a.
> 
> This commit replaced the crtc_state->enable_psr2_sel_fetch guard in
> icl_plane_disable_sel_fetch_arm() and
> i9xx_cursor_disable_sel_fetch_arm()
> with HAS_PSR2_SEL_FETCH(). This is a display version check and
> says nothing about the pipe, so every plane and cursor disable on a
> display 12+ platform started writing SEL_FETCH_PLANE_CTL() /
> SEL_FETCH_CUR_CTL(), including on pipes that do not implement them.
> It shows up as an unclaimed register access on pipes driving HDMI
> where
> selective fetch was never enabled.
> 
> The stale selective fetch enable bit that commit addressed is handled
> in the next patch.
> 
> Fixes: 7f1172a2ac0d ("drm/i915/display: Clear SEL_FETCH_PLANE_CTL on
> plane disable")
> Closes:
> https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16876
> Cc: stable@vger.kernel.org
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>

Reviewed-by: Jouni Högander <jouni.hogander@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c       | 15 +++++--------
> --
>  .../gpu/drm/i915/display/skl_universal_plane.c    | 15 +++++--------
> --
>  2 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index cce041e1da51..90173040d825 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -531,18 +531,13 @@ static int i9xx_check_cursor(struct
> intel_crtc_state *crtc_state,
>  }
>  
>  static void i9xx_cursor_disable_sel_fetch_arm(struct intel_dsb *dsb,
> -					      struct intel_plane
> *plane)
> +					      struct intel_plane
> *plane,
> +					      const struct
> intel_crtc_state *crtc_state)
>  {
>  	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 +587,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 +696,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/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 5cda1ab90e40..07a683293352 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -879,18 +879,13 @@ skl_plane_disable_arm(struct intel_dsb *dsb,
>  }
>  
>  static void icl_plane_disable_sel_fetch_arm(struct intel_dsb *dsb,
> -					    struct intel_plane
> *plane)
> +					    struct intel_plane
> *plane,
> +					    const struct
> intel_crtc_state *crtc_state)
>  {
>  	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 +921,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 +1641,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