Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Severe integer arithmetic precision loss in `get_switch_to_active` calculates a latency of 0 for UHBR20 links and incorrect values for other UHBR links. - [Medium] Missing bounds validation for the computed `switch_to_active` scanline value. --
--- Patch [14]: [PATCH v5 14/15] drm/i915/alpm: Compute and program switch to active latency --- commit c70c27fb353539a13b15cf08616e266e5b7146a1 Author: Animesh Manna <[email protected]> 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. For non-UHBR rates, the latency is zero. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 8c17da9883d6d..5d2ac75615670 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -270,6 +270,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 = 32 * ((396 + 3 + 64) / symbol_clock); [Severity: High] Does this calculation lose precision by performing the division before the multiplication? For a UHBR20 link where symbol_clock is 625 MHz, the integer division (396 + 3 + 64) / 625 will yield 0, resulting in a switch_to_active latency of 0. For UHBR10 (312 MHz) and UHBR13.5 (421 MHz), the division will yield 1, making the result 32 instead of the mathematically correct values of ~47 and ~35, respectively. Should this multiply before dividing, perhaps using DIV_ROUND_UP()? > + } else { > + switch_to_active = 0; > + } > + > + return switch_to_active; > +} > + > /* > * AUX-Less Wake Time = CEILING( ((PHY P2 to P0) + tLFPS_Period, Max+ > * tSilence, Max+ tPHY Establishment + tCDS) / tline) [ ... ] > @@ -329,7 +354,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: Medium] Is it possible for the computed scanline value to exceed the hardware limits? Earlier in _lnl_compute_aux_less_alpm_params(), values like aux_less_wake_lines and silence_period are checked against their hardware bounds, and the function returns false if they exceed limits to safely reject the configuration. However, switch_to_active is programmed into a 5-bit register field (ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY_MASK). If the converted scanline value exceeds 31, it will be silently truncated when passed to the macro in lnl_alpm_configure(). Should there be a bounds check for switch_to_active before applying it? > return true; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
