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

> -----Original Message-----
> From: Jani Nikula <[email protected]>
> Sent: Wednesday, August 12, 2026 3:47 PM
> To: Garg, Nemesa <[email protected]>; [email protected];
> [email protected]
> Cc: Garg, Nemesa <[email protected]>
> Subject: Re: [PATCH] drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane
> disable
> 
> On Wed, 12 Aug 2026, Nemesa Garg <[email protected]> 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.
> 
> Why doesn't the selective fetch disable change the bit?
> 
During disable path icl_plane_disable_sel_fetch_arm is called and here we have a condition :
if (!crtc_state->enable_psr2_sel_fetch)
                return;
		
So if the plane is disabled after selective fetch has already been turned off, this returns early and the enable bit is left set in hardware.

Turning selective fetch back on does not repair it: icl_plane_update_sel_fetch_arm() only runs for planes that are in the update mask and visible. A plane disabled during the off window is neither, so its stale bit is never rewritten, it just becomes live again.

From logs: plane 1A after being disabled:
 plane 1 ddb (0 - 0) CTL=0x00000000 SURF=0x00000000 SURFLIVE=0x00000000
                      SEL_FETCH_PLANE_CTL=0x80000000

Thanks and Regards,
Nemesa

> > 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.
> >
> > 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
> 
> Missing space after :.
> 
> > Assisted-by: GitHub-Copilot:claude-opus-4.6
> > Signed-off-by: Nemesa Garg <[email protected]>
> > ---
> >  drivers/gpu/drm/i915/display/skl_universal_plane.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 07a683293352..afc1df1b84a4 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -885,7 +885,13 @@ 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;
> >
> > -	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);
> 
> --
> Jani Nikula, Intel