Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/i915/alpm: Unconditional overwrite of has_alpm breaks Panel Replay without ALPM - [High] drm/i915/alpm: Retained eDP check in intel_psr_needs_alpm_aux_less prevents AS SDP for external DP - [High] drm/i915/alpm: has_alpm state leak when PSR/PR features are downgraded late - [Medium] drm/i915/alpm: Early return in intel_alpm_port_configure prevents clearing stale ALPM state - [Medium] drm/i915/alpm: Missing has_alpm check causes incorrect PR_ALPM_CTL programming --
commit 4dcfef3fd89e0ed18372dabec785892c009d57eb Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf This commit introduces a single crtc_state->has_alpm flag to track ALPM enablement, decoupling it from implicit feature tracking like eDP Panel Replay, PSR2, or LOBF. It updates various ALPM configuration and port paths to use this new flag as the single source of truth. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 29a93a252fe9b..236341aa3c8d8 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -719,7 +720,7 @@ void intel_alpm_pr_as_sdp_update(const struct > intel_crtc_state *crtc_state) > intel_dp = enc_to_intel_dp(encoder); > > if (!intel_dp->as_sdp_supported || > - !intel_alpm_is_alpm_aux_less(intel_dp, crtc_state)) > + !intel_alpm_is_aux_less(intel_dp, crtc_state)) > continue; > > mutex_lock(&intel_dp->alpm.lock); [Severity: Medium] Does this missing check for crtc_state->has_alpm cause incorrect PR_ALPM_CTL programming? The intel_alpm_is_aux_less() function evaluates to true for Panel Replay regardless of the actual has_alpm state. Without an explicit check verifying that ALPM is actually active, if Panel Replay is explicitly falling back to a "without ALPM" configuration, this loop will incorrectly write AS SDP parameters to the ALPM controller, which contradicts the fallback state. [ ... ] > @@ -792,10 +792,10 @@ void intel_alpm_port_configure(struct intel_dp > *intel_dp, > enum port port = dp_to_dig_port(intel_dp)->base.port; > u32 alpm_ctl_val = 0, lfps_ctl_val = 0; > > - if (DISPLAY_VER(display) < 20) > + if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm) > return; [Severity: Medium] Will this early return prevent clearing stale ALPM state in the hardware? Previously, this function would proceed and write the zeroed alpm_ctl_val and lfps_ctl_val to the hardware when ALPM was disabled. By returning early when crtc_state->has_alpm is false, any transition from an ALPM-enabled mode to an ALPM-disabled mode will skip the clearing writes. This leaves the hardware with an active ALPM configuration while the driver state considers it disabled. > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 872e253db1786..1f14dbe57512f 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1648,6 +1648,8 @@ static bool intel_sel_update_config_valid(struct > intel_crtc_state *crtc_state, > crtc_state->enable_psr2_su_region_et = > psr2_su_region_et_valid(connector, > > crtc_state->has_panel_replay); > > + crtc_state->has_alpm = intel_alpm_is_possible(intel_dp); > + [Severity: High] Does this unconditional assignment overwrite the validated has_alpm state? The Panel Replay configuration path sets has_alpm based on whether ALPM timing actually fits into the vblank period. When Panel Replay is validated but ALPM timing fails, has_alpm is correctly set to false to allow Panel Replay without ALPM. This line in intel_sel_update_config_valid() unconditionally overwrites that calculated state with a simple capability check, forcing it back to true. This destroys the fallback state, causing the late compute config to evaluate ALPM wake lines again, fail, and fully disable Panel Replay on external DP. [ ... ] > @@ -4555,16 +4569,6 @@ void intel_psr_connector_debugfs_add(struct > intel_connector *connector) > connector, &i915_psr_status_fops); > } > > -bool intel_psr_needs_alpm(struct intel_dp *intel_dp, const struct > intel_crtc_state *crtc_state) > -{ > - /* > - * eDP Panel Replay uses always ALPM > - * PSR2 uses ALPM but PSR1 doesn't > - */ > - return intel_dp_is_edp(intel_dp) && (crtc_state->has_sel_update || > - crtc_state->has_panel_replay); > -} > - > bool intel_psr_needs_alpm_aux_less(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state) > { [Severity: High] Does retaining the eDP-only check in intel_psr_needs_alpm_aux_less() prevent AS SDP configuration for external DP Panel Replay? While intel_alpm_is_aux_less() was updated to support external DP, intel_psr_needs_alpm_aux_less() still restricts true return values to eDP only. Any external DP configuration enabling Panel Replay will fail this check when evaluated by intel_dp_pr_with_as_sdp_enabled() in the link training paths. This leads to silently missing the DP_FIXED_VTOTAL_AS_SDP_EN_IN_PR_ACTIVE configuration during link training, causing a mismatch between source programming and the sink DPCD configuration. > @@ -4578,15 +4582,17 @@ void intel_psr_compute_config_late(struct intel_dp > *intel_dp, > int vblank = intel_crtc_vblank_length(crtc_state); > int wake_lines; > > - if (intel_psr_needs_alpm_aux_less(intel_dp, crtc_state)) > - wake_lines = crtc_state->alpm_state.aux_less_wake_lines; > - else if (intel_psr_needs_alpm(intel_dp, crtc_state)) > - wake_lines = DISPLAY_VER(display) < 20 ? > - > psr2_block_count_lines(crtc_state->alpm_state.io_wake_lines, > - > crtc_state->alpm_state.fast_wake_lines) : > - crtc_state->alpm_state.io_wake_lines; > - else > + if (crtc_state->has_alpm) { > + if (crtc_state->has_panel_replay) > + wake_lines = crtc_state->alpm_state.aux_less_wake_lines; > + else > + wake_lines = DISPLAY_VER(display) < 20 ? > + > psr2_block_count_lines(crtc_state->alpm_state.io_wake_lines, > + > crtc_state->alpm_state.fast_wake_lines) : > + crtc_state->alpm_state.io_wake_lines; > + } else { > wake_lines = 0; > + } > > /* > * Disable the PSR features if wake lines exceed the available vblank. [Severity: High] Can has_alpm leak as true if PSR or Panel Replay features are disabled late in this function? When falling back to PSR1 or fully disabling PSR due to vblank timing limits or hardware workarounds later in intel_psr_compute_config_late(): if (crtc_state->has_panel_replay) { crtc_state->has_panel_replay = false; crtc_state->has_psr = false; } crtc_state->has_sel_update = false; The corresponding feature flags are cleared, but has_alpm is left as true. Since ALPM programming paths (like intel_alpm_port_configure) now rely directly on has_alpm, this leaked state can cause unintended ALPM hardware programming on unsupported modes. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
