Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/i915/alpm: wrong transcoder ALPM_CTL access in intel_alpm_disable() --
commit 16555494a05a6a62d73ca6ad3e0999793ea5b8b4 Author: Animesh Manna <[email protected]> drm/i915/alpm: alpm_init() for DP2.1 This commit initializes ALPM for DP 2.1 on display version 35 and newer, separating the ALPM mutex initialization from DPCD capability initialization and checking source support instead of sink capabilities in intel_alpm_disable(). > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index cb1d68fef1986..b6307862b7560 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -750,7 +763,11 @@ void intel_alpm_disable(struct intel_dp *intel_dp) > struct intel_display *display = to_intel_display(intel_dp); > enum transcoder cpu_transcoder = intel_dp->alpm.transcoder; > > - if (DISPLAY_VER(display) < 20 || !intel_dp->alpm_dpcd) > + /* > + * Don't check sink ALPM caps here, they are reset on disconnect > + * before the CRTC is disabled. > + */ > + if (DISPLAY_VER(display) < 20 || !intel_alpm_source_supported(intel_dp)) > return; [Severity: High] Can replacing the intel_dp->alpm_dpcd check with intel_alpm_source_supported(intel_dp) here cause intel_alpm_disable() to modify ALPM_CTL on the wrong transcoder when ALPM was never configured on intel_dp? Because struct intel_digital_port is zero-allocated, intel_dp->alpm.transcoder starts at 0 (TRANSCODER_A, rather than INVALID_TRANSCODER), and intel_alpm_init() does not initialize it. Across the driver, intel_dp->alpm.transcoder is only set in intel_alpm_configure(): drivers/gpu/drm/i915/display/intel_alpm.c:intel_alpm_configure() { lnl_alpm_configure(intel_dp, crtc_state); intel_dp->alpm.transcoder = crtc_state->cpu_transcoder; } which is only called from intel_psr_enable_source() when crtc_state->has_psr is true or from intel_alpm_lobf_enable() when new_crtc_state->has_lobf is true. However, intel_ddi_disable_dp() unconditionally calls intel_alpm_disable(intel_dp) whenever any DP or eDP encoder is disabled: drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_disable_dp() { ... intel_panel_unprepare(old_conn_state); intel_psr_disable(intel_dp, old_crtc_state); intel_alpm_disable(intel_dp); ... } And intel_dp_short_pulse() also calls intel_alpm_disable(intel_dp) if reading DP_RECEIVER_ALPM_STATUS fails or reports an error: drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_short_pulse() { ... if (intel_alpm_get_error(intel_dp)) { intel_alpm_disable(intel_dp); intel_dp->alpm.sink_alpm_error = true; } ... } Previously, !intel_dp->alpm_dpcd caused intel_alpm_disable() to return early for external DP connectors and eDP panels without ALPM support. With this change, if an external DP connector on DISPLAY_VER(display) >= 35 (or an eDP connector on DISPLAY_VER(display) >= 20) is enabled on TRANSCODER_B/C/D without PSR, Panel Replay, or LOBF, intel_alpm_configure() is never called and intel_dp->alpm.transcoder remains TRANSCODER_A (or a stale transcoder from a previous modeset). When that connector is later disabled or hits the short HPD pulse error path, intel_alpm_disable() will lock that connector's intel_dp->alpm.lock and clear ALPM_CTL_ALPM_ENABLE | ALPM_CTL_LOBF_ENABLE in ALPM_CTL(display, TRANSCODER_A). If TRANSCODER_A is active on another connector (such as eDP on Pipe A), this races with Pipe A and disables its active ALPM/LOBF state; if TRANSCODER_A's power well is off, it triggers an unclaimed MMIO register access. Should intel_dp->alpm.transcoder be initialized to INVALID_TRANSCODER in intel_alpm_init(), reset to INVALID_TRANSCODER in intel_alpm_disable(), and checked for INVALID_TRANSCODER before accessing ALPM_CTL()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
