Re: [PATCH] drm/i915/display: guard intel_alpm_get_error() against non-ALPM sinks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] intel_alpm_get_error() clears the hardware error state on read, causing a secondary caller within the same short pulse interrupt to miss the error.
--

commit 7e6bdc8ef5d560933201e9367240c8dcc3b248fe
Author: Stephen J. Fuhry <fuhrysteve@gmail.com>

drm/i915/display: guard intel_alpm_get_error() against non-ALPM sinks

intel_alpm_get_error() unconditionally reads DP_RECEIVER_ALPM_STATUS via
AUX from any DP connection that receives a short-pulse interrupt. However,
alpm_dpcd is only populated by intel_edp_init_dpcd(), so it is zero for
all external (non-eDP) DP and DP-MST connections. This patch adds a check
to return early when ALPM is not supported.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index c6963ea420cc9..07c1a96ddc6e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -629,6 +629,9 @@ bool intel_alpm_get_error(struct intel_dp *intel_dp)
>  	u8 val;
>  	int r;
>  
> +	if (!intel_dp->alpm_dpcd)
> +		return false;
> +
>  	r = drm_dp_dpcd_readb(aux, DP_RECEIVER_ALPM_STATUS, &val);
>  	if (r != 1) {
>  		drm_err(display->drm, "Error reading ALPM status\n");
>  		return true;
>  	}
>  
>  	if (val & DP_ALPM_LOCK_TIMEOUT_ERROR) {
>  		drm_dbg_kms(display->drm, "ALPM lock timeout error\n");
>  
>  		/* Clearing error */
>  		drm_dp_dpcd_writeb(aux, DP_RECEIVER_ALPM_STATUS, val);

[Severity: High]
This is a pre-existing issue, but does clearing the hardware error state here
cause subsequent checks to miss the error?

During an HPD short pulse, intel_dp_short_pulse() first calls
intel_psr_short_pulse(), which calls psr_alpm_check(). This invokes
intel_alpm_get_error() which reads and clears the DP_ALPM_LOCK_TIMEOUT_ERROR
bit:

drivers/gpu/drm/i915/display/intel_psr.c:psr_alpm_check() {
    ...
    if (intel_alpm_get_error(intel_dp)) {
        intel_psr_disable_locked(intel_dp);
        psr->sink_not_reliable = true;
    }
    ...
}

Later in the same sequence, intel_dp_short_pulse() directly calls
intel_alpm_get_error() a second time:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_short_pulse() {
    ...
    intel_psr_short_pulse(intel_dp);

    if (intel_alpm_get_error(intel_dp)) {
        intel_alpm_disable(intel_dp);
        intel_dp->alpm.sink_alpm_error = true;
    }
    ...
}

Because the DPCD error bit was already cleared by the first call, the second
call will return false.

While psr_alpm_check() disables PSR via intel_psr_disable_locked(), that
function only disables ALPM/LOBF for Panel Replay, and psr_alpm_check() only
runs when Panel Replay is NOT enabled.

For standard PSR-enabled eDP panels, does the second check failing mean we
skip calling intel_alpm_disable(intel_dp), leaving LOBF improperly enabled
despite the ALPM timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611125408.676626-1-fuhrysteve@gmail.com?part=1