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 4:36 PM
> To: Garg, Nemesa <[email protected]>; [email protected];
> [email protected]
> Subject: RE: [PATCH] drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane
> disable
> 
> On Wed, 12 Aug 2026, "Garg, Nemesa" <[email protected]> wrote:
> >> -----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
> 
> None of that answers my question.
> 
> Where does crtc_state->enable_psr2_sel_fetch get set to false without
> disabling the bit in hardware? Why? What's the scenario that actually leads us
> to the case at hand?
> 
> 
We have intel_sel_update_config_valid() where we check when to disable selective fetch.
In our case its pipe_crc so whenever crc is enabled we need to disable the selective fetch and hence
we set the flag crtc_state->enable_psr2_sel_fetch as false.

After this when we land in icl_plane_update_sel_fetch_arm() we do early return becuase of the check
if(!crtc_state->enable_psr2_sel_fetch)
  retrun
without clearing the bit in hardware.

Scenario leading to DBUF overlap:
1. PR + selective update active.  Plane 1A is enabled and SEL_FETCH_PLANE_CTL has its enable bit set.
2. Pipe CRC gets enabled so enable_psr2_sel_fetch becomes false. Plane 1A is still visible,
   icl_plane_update_sel_fetch_arm() returns early, and the enable bit stays set.
3. Later plane 1A disabled. icl_plane_disable_arm() -> icl_plane_disable_sel_fetch_arm() takes
   the same early return, so the last chance to clear it is missed.
4. Pipe CRC gets disabled. Selective fetch is enabled again.
5. icl_plane_update_sel_fetch_arm() only runs for planes that are visible and plane 1A is not,
   so nothing rewrites its bit. 
6. Later the hardware still treats disabled plane as a selective fetch participant and the DBuf
   overlap detector then flags the collision against the live plane's allocation starting at the
   same block.

Thanks and Regards,
Nemesa

> BR,
> Jani.
> 
> 
> >
> > 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/87
> >> > 39
> >>
> >> 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
> 
> --
> Jani Nikula, Intel