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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The fix for orphaned selective fetch enable bits is incomplete, missing the identical flaw in the cursor plane disable logic (`i9xx_cursor_disable_sel_fetch_arm()`).
--

--- Patch [1]: [PATCH] drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane disable ---
commit e0077c037f1d1d0137a05dcfc36800e4add084cd
Author: Nemesa Garg <[email protected]>
drm/i915/display: Clear SEL_FETCH_PLANE_CTL on plane disable

This commit fixes an issue where disabling a plane after selective fetch
was turned off would leave the SEL_FETCH_PLANE_CTL enable bit set in
hardware. It changes the early return condition to check for hardware
support rather than the current state.

Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8739

> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 07a6832933521..afc1df1b84a4a 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);

[Severity: High]
This is a pre-existing issue, but does this fix need to be applied to the
cursor plane logic as well? 

Looking at drivers/gpu/drm/i915/display/intel_cursor.c, the function
i9xx_cursor_disable_sel_fetch_arm() still uses the old state-based early
return:

	if (!crtc_state->enable_psr2_sel_fetch)
		return;

	intel_de_write_dsb(display, dsb, SEL_FETCH_CUR_CTL(pipe), 0);

If selective fetch is dynamically disabled while a cursor plane is active,
and then the cursor is disabled, this early return will leave the
SEL_FETCH_CUR_CTL bit set in hardware. 

If selective fetch is later re-enabled, could the hardware mistakenly resume
fetching for the disabled cursor and keep its old Display Data Buffer range
reserved, potentially starving other planes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1