Re: [PATCH] drm/i915/display: Run the selective fetch update after the crtc checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/display: sel fetch loop misapplied to intel_async_flip_check_uapi()
--

commit 95230b3cd094aeb1ee4f35af24d1f7d8002a6446
Author: Mika Kahola <mika.kahola@intel.com>

drm/i915/display: Run the selective fetch update after the crtc checks

This commit moves the call to intel_psr2_sel_fetch_update() out of
intel_crtc_atomic_check() into a separate loop so that selective fetch is
computed after all joined pipes have had their scaler states updated.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d5d4453f283bb..e3831260ca4a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4444,10 +4444,6 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  
>  	}
>  
> -	ret = intel_psr2_sel_fetch_update(state, crtc);
> -	if (ret)
> -		return ret;
> -
>  	return 0;
>  }
>  
> @@ -6188,6 +6184,19 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
>  		}
>  	}
>  
> +	/* Selective fetch needs the scaler state of all the joined pipes. */
> +	for_each_new_intel_crtc_in_state(state, crtc, crtc_state) {
> +		int ret;
> +
> +		ret = intel_psr2_sel_fetch_update(state, crtc);
> +		if (ret) {
> +			drm_dbg_atomic(display->drm,
> +				       "[CRTC:%d:%s] selective fetch check failed\n",
> +				       crtc->base.base.id, crtc->base.name);
> +			return ret;
> +		}
> +	}

[Severity: High]
Was this hunk misapplied to intel_async_flip_check_uapi() instead of
intel_atomic_check_crtcs()?

In drivers/gpu/drm/i915/display/intel_display.c, intel_async_flip_check_uapi()
declares new_crtc_state rather than crtc_state, so expanding
for_each_new_intel_crtc_in_state(state, crtc, crtc_state) fails to compile
with an undeclared identifier error:

drivers/gpu/drm/i915/display/intel_display.c:intel_async_flip_check_uapi() {
	struct intel_display *display = to_intel_display(state);
	const struct intel_crtc_state *new_crtc_state =
		intel_atomic_get_new_crtc_state(state, crtc);
	...
	if (!new_crtc_state->uapi.async_flip)
		return 0;
	...
	if (new_crtc_state->joiner_pipes) {
		drm_dbg_kms(display->drm,
			    "[CRTC:%d:%s] async flip disallowed with joiner\n",
			    crtc->base.base.id, crtc->base.name);
		return -EINVAL;
	}
}

In addition, intel_async_flip_check_uapi() returns early for synchronous
commits and rejects joined pipes, and runs in intel_atomic_check() before
intel_atomic_check_crtcs() configures the scalers. With the call removed
from intel_crtc_atomic_check(), normal atomic commits never run
intel_psr2_sel_fetch_update().

Should this loop be placed at the end of intel_atomic_check_crtcs()
instead, as described in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102080705.2027571-1-mika.kahola@intel.com?part=1