Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/i915/alpm: invalid DPCD capability checks in intel_alpm_is_possible() - [High] drm/i915/alpm: ALPM_CTL left enabled on hot-unplug due to dynamic capability checks --
commit f95070b21da14e9f299aa7eaa97682597723ecc9 Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: Replace is_edp() with alpm_is_possible() This commit introduces a separate intel_alpm_is_possible() function to check for ALPM capability from DPCD information, replacing earlier checks that relied solely on the panel being eDP. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 06c1a59aa2c51..29a93a252fe9b 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -69,6 +69,13 @@ void intel_alpm_init(struct intel_dp *intel_dp) > mutex_init(&intel_dp->alpm.lock); > } > > +bool intel_alpm_is_possible(struct intel_dp *intel_dp) > +{ > + return (intel_dp->alpm_dpcd || > + intel_dp->lttpr_common_caps[DP_LTTPR_ALPM_CAPABILITIES - > + > DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV]); > +} [Severity: High] Does this logic correctly enforce DP requirements for ALPM? It looks like this uses a logical OR instead of requiring both the sink and LTTPR to support ALPM. Additionally, this seems to access the raw capability byte without masking the specific ALPM capability bit, which could mean reserved bits will be evaluated as true. Can this break standard DP link training invariants when connecting a display with LTTPRs that support ALPM while the sink does not, or when a sink has reserved bits set but no ALPM support? [ ... ] > @@ -832,7 +839,7 @@ void intel_alpm_lobf_disable(const struct > intel_crtc_state *new_crtc_state) > > intel_dp = enc_to_intel_dp(encoder); > > - if (!intel_dp_is_edp(intel_dp)) > + if (!intel_alpm_is_possible(intel_dp)) > continue; > > mutex_lock(&intel_dp->alpm.lock); [Severity: High] Could this dynamic capability check cause ALPM_CTL to be left enabled during a hot-unplug event? During a hot-unplug event, the cached DPCD capabilities like alpm_dpcd can be cleared before the teardown hooks execute. Because this teardown function checks these dynamic capabilities via intel_alpm_is_possible() instead of the old hardware state, it might return early and fail to clear the ALPM_CTL hardware register. Would this leave the transcoder with ALPM enabled when it is supposed to be disabled, potentially causing state mismatches on subsequent modesets? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
