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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 12 Aug 2026, "Garg, Nemesa" <nemesa.garg@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Wednesday, August 12, 2026 3:47 PM
>> To: Garg, Nemesa <nemesa.garg@intel.com>; intel-gfx@lists.freedesktop.org;
>> intel-xe@lists.freedesktop.org
>> Cc: Garg, Nemesa <nemesa.garg@intel.com>
>> Subject: Re: [PATCH] drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane
>> disable
>> 
>> On Wed, 12 Aug 2026, Nemesa Garg <nemesa.garg@intel.com> 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?


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/8739
>> 
>> Missing space after :.
>> 
>> > Assisted-by: GitHub-Copilot:claude-opus-4.6
>> > Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
>> > ---
>> >  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