Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/i915/alpm: ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY bounds check missing - [High] drm/i915/alpm: switch_to_active math error inflates latency 32x --
commit 6e4638c7fe4ea7d79d2000aac2b17f2fc3edc381 Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: Compute and program switch to active latency This commit programs the ALPM switch to active latency into ALPM_CTL2. For UHBR link rates, the latency is derived from the link symbol clock and converted to scanlines, while for non-UHBR rates it is set to zero. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 236341aa3c8d8..84777c8d31bb4 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -301,6 +301,31 @@ static int get_establishment_period(struct intel_dp > *intel_dp, > return establishment_period; > } > > +static int get_switch_to_active(const struct intel_crtc_state *crtc_state) > +{ > + int port_clock = crtc_state->port_clock; > + int switch_to_active; > + > + if (intel_dp_is_uhbr(crtc_state)) { > + /* symbol_clock (fLink) in MHz */ > + int symbol_clock = port_clock / > intel_dp_link_symbol_size(port_clock) / 100; > + > + /* > + * tSwitch to Active = 32 * (ML_PHY_LOCK Length + 3 + 64) / > fLink > + * > + * The "+ 3" term is the trailing zero padding after the > + * POST_LT_SCRAMBLER_RESET, the "+ 64" term represents the MTP > + * time slots. The result is in microseconds. > + */ > + switch_to_active = get_dp_link_symbol_size(crtc_state) * > + (ML_PHY_LOCK_LEN_UHBR + 3 + 64) / > symbol_clock; [Severity: High] Does this logic incorrectly substitute the symbol rate for the bit rate, inflating the latency by 32x? In the DP spec formula, fLink is the link rate in Mbps, and multiplying the symbol count by 32 correctly calculates bits divided by bit-rate. However, symbol_clock is calculated here as port_clock / 32, meaning the denominator is 32x smaller than intended. This math error calculates a latency of ~47.4us instead of 1.48us, causing excessively long ALPM wake sequences. Additionally, standard integer division here drops fractional microseconds, so if corrected, the wait time might be shorter than the physical hardware minimum on high-clock displays. > + } else { > + switch_to_active = 0; > + } > + > + return switch_to_active; > +} > + [ ... ] > @@ -360,7 +385,8 @@ _lnl_compute_aux_less_alpm_params(struct intel_dp > *intel_dp, > crtc_state->alpm_state.aux_less_wake_lines = aux_less_wake_lines; > crtc_state->alpm_state.silence_period_sym_clocks = silence_period; > crtc_state->alpm_state.lfps_half_cycle_num_of_syms = lfps_half_cycle; > - > + crtc_state->alpm_state.switch_to_active = > intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, > + > get_switch_to_active(crtc_state)); [Severity: High] Can the computed scanline count overflow the 5-bit register field it is later written to? Unlike the other parameters configured in _lnl_compute_aux_less_alpm_params(), there is no bounds check here against the maximum allowed value before assigning it to switch_to_active. With the 32x inflation bug above, on high-refresh-rate displays (like 4K @ 360Hz), ~47us translates to > 31 scanlines. > return true; > } > [ ... ] > @@ -772,6 +798,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp, > lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps); > > alpm_ctl2 = ALPM_CTL2_NUMBER_OF_LTTPR(lttpr_count); > + alpm_ctl2 |= > ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY(crtc_state->alpm_state.switch_to_active); [Severity: High] Does this silently mask and truncate the unbounded scanline count? ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY uses REG_FIELD_PREP with a 5-bit mask. Any value > 31 is silently truncated. For example, 32 scanlines truncates to 0, completely removing the required wake latency. This causes the hardware to exit ALPM before the PHY has locked, leading to link training failures or display corruption. > > intel_de_write(display, ALPM_CTL2(display, cpu_transcoder), alpm_ctl2); > intel_de_write(display, ALPM_CTL(display, cpu_transcoder), alpm_ctl); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
