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 <[email protected]> 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/[email protected]?part=1
