Re: [PATCH] drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane disable

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

On Tue, 2026-08-18 at 15:21 +0530, Nemesa Garg wrote:
> icl_plane_disable_sel_fetch_arm() wrote SEL_FETCH_PLANE_CTL = 0 only
> when
> crtc_state->enable_psr2_sel_fetch was set. If a plane was disabled
> after
> selective fetch had been turned off, the guard fired early and left
> the
> register's enable bit set in hardware.
> 
> The bit is harmless until selective fetch is re-enabled.  When it is,
> the
> hardware resumes fetching for the now-disabled plane and keeps its
> old DDB
> range reserved.
> 
> i9xx_cursor_disable_sel_fetch_arm() has the same guard on
> SEL_FETCH_CUR_CTL
> and is fixed the same way.
> 
> v2: Add same check for cursor also. [sashiko]
> 
> Fixes: b1f5279b5981 ("drm/i915/psr: Move plane sel fetch
> configuration into plane source files")
> Closes:
> https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8739
> Assisted-by: GitHub-Copilot:claude-opus-4.6
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>

Good finding:

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, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 0673f16f6fd0..86bb96ac449b 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -530,13 +530,18 @@ 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,
> -					      const struct
> intel_crtc_state *crtc_state)
> +					      struct intel_plane
> *plane)
>  {
>  	struct intel_display *display = to_intel_display(plane);
>  	enum pipe pipe = plane->pipe;
>  
> -	if (!crtc_state->enable_psr2_sel_fetch)
> +	/*
> +	 * 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))
>  		return;
>  
>  	intel_de_write_dsb(display, dsb, SEL_FETCH_CUR_CTL(pipe),
> 0);
> @@ -586,7 +591,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, crtc_state);
> +			i9xx_cursor_disable_sel_fetch_arm(dsb,
> plane);
>  	}
>  }
>  
> @@ -695,7 +700,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,
> crtc_state);
> +		i9xx_cursor_disable_sel_fetch_arm(dsb, plane);
>  
>  	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 07a683293352..5cda1ab90e40 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -879,13 +879,18 @@ skl_plane_disable_arm(struct intel_dsb *dsb,
>  }
>  
>  static void icl_plane_disable_sel_fetch_arm(struct intel_dsb *dsb,
> -					    struct intel_plane
> *plane,
> -					    const struct
> intel_crtc_state *crtc_state)
> +					    struct intel_plane
> *plane)
>  {
>  	struct intel_display *display = to_intel_display(plane);
>  	enum pipe pipe = plane->pipe;
>  
> -	if (!crtc_state->enable_psr2_sel_fetch)
> +	/*
> +	 * 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))
>  		return;
>  
>  	intel_de_write_dsb(display, dsb, SEL_FETCH_PLANE_CTL(pipe,
> plane->id), 0);
> @@ -921,7 +926,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, crtc_state);
> +	icl_plane_disable_sel_fetch_arm(dsb, plane);
>  
>  	if (plane_has_normalizer(plane))
>  		intel_de_write_dsb(display, dsb,
> @@ -1641,7 +1646,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,
> crtc_state);
> +		icl_plane_disable_sel_fetch_arm(dsb, plane);
>  }
>  
>  static void