Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Source/sink protocol mismatch for PSR2 panels with AUX-less capability. - [High] State leakage of the `has_alpm` flag forces ALPM on unsupported PSR1 fallbacks. - [High] Inconsistent ALPM wake line validation causes unsafe late atomic state downgrades. --
--- Patch [13]: [PATCH v5 13/15] drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf --- commit dd0c0bc3be20db88d8488b6083be9663dc32cc6d Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf For DP2.1, ALPM is optional and there can be scenario where panel replay will be supported without ALPM. So decouple ALPM feature by has_alpm flag from related display features. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index f3f6f40f96244..8c17da9883d6d 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -575,8 +569,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp, > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 alpm_ctl, alpm_ctl2, lttpr_count; > > - if (DISPLAY_VER(display) < 20 || (!intel_psr_needs_alpm(intel_dp, > crtc_state) && > - !crtc_state->has_lobf)) > + if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm) > return; > > mutex_lock(&intel_dp->alpm.lock); > @@ -584,7 +577,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp, > * Panel Replay on eDP is always using ALPM aux less. I.e. no need to > * check panel support at this point. > */ > - if (intel_alpm_is_alpm_aux_less(intel_dp, crtc_state)) { > + if (intel_alpm_aux_less_wake_supported(intel_dp)) { > alpm_ctl = ALPM_CTL_ALPM_ENABLE | > ALPM_CTL_ALPM_AUX_LESS_ENABLE | > ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_50_SYMBOLS; [Severity: High] Does this create a protocol mismatch between the source and the sink? By changing this to rely on intel_alpm_aux_less_wake_supported(), the source is now programmed for AUX-less ALPM (ALPM_CTL_ALPM_AUX_LESS_ENABLE) for PSR2 panels that advertise DP_ALPM_AUX_LESS_CAP. However, intel_alpm_enable_sink() still requires Panel Replay or LOBF to enable AUX-less mode on the sink. Because neither is true for PSR2, the sink will be expecting fast-wake ALPM, which conflicts with how the source is now programmed here. [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 40e3d70959965..8c6aa4e948894 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c [ ... ] > @@ -1729,11 +1731,17 @@ static bool _panel_replay_compute_config(struct > intel_crtc_state *crtc_state, > crtc_state->link_off_after_as_sdp_when_pr_active = > compute_link_off_after_as_sdp_when_pr_active(connector); > crtc_state->disable_as_sdp_when_pr_active = > compute_disable_as_sdp_when_pr_active(connector); > > + if (intel_alpm_is_possible(intel_dp)) > + crtc_state->has_alpm = alpm_config_valid(intel_dp, crtc_state, > true, true, false); > + > if (!intel_dp_is_edp(intel_dp)) > return true; > > /* Remaining checks are for eDP only */ > > + if (!crtc_state->has_alpm) > + return false; > + > if (to_intel_crtc(crtc_state->uapi.crtc)->pipe != PIPE_A && > to_intel_crtc(crtc_state->uapi.crtc)->pipe != PIPE_B) > return false; [Severity: High] Can this leak the has_alpm state if the panel replay config is aborted? If alpm_config_valid() succeeds, crtc_state->has_alpm is set to true. However, if a subsequent check in _panel_replay_compute_config() returns false (such as the PIPE_A/B check or a later HDCP check), has_alpm remains set to true. When the driver then falls back to computing PSR1, has_alpm stays true and bypasses the previous protections that prevented ALPM from being enabled on PSR1. This forces ALPM on for PSR1 fallbacks where it is not supported. [ ... ] > @@ -4547,15 +4542,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 (intel_alpm_aux_less_wake_supported(intel_dp)) > + 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] Does this late selection of aux_less_wake_lines conflict with the early validation performed for PSR2? During early validation, intel_psr2_config_valid() hardcodes the aux_less parameter to false when calling alpm_config_valid(), so it validates against the smaller io_wake_lines. If intel_alpm_aux_less_wake_supported() evaluates to true here in late compute, it will dynamically select the larger aux_less_wake_lines instead. This mismatch can cause the subsequent _wake_lines_fit_into_vblank() check to fail, triggering a late teardown of has_sel_update after watermarks and DDB allocations have already been computed assuming PSR/SU was active. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
