Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Severe 32-bit signed integer overflow in `tml_phy_lock` calculation results in negative time values. - [Medium] Missing error check on `drm_dp_lttpr_count()` return value allows negative error codes to corrupt ALPM time calculations. - [Medium] The bounds check for `aux_less_wake_lines` does not use the expanded Xe3LPD mask, artificially limiting the maximum wake lines and potentially disabling ALPM on newer hardware. --
--- Patch [6]: [PATCH v5 06/15] drm/i915/alpm: Auxless wake time calculation for Xe3p --- commit d691d0d2ce1f7d50c7d92a8c206ccf76b71ba2ce Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: Auxless wake time calculation for Xe3p This commit adds support for auxless waketime calculation for DP2.1 ALPM, updating dependent parameters. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 4b15d96d0ad79..af2733805505e 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > @@ -105,21 +105,50 @@ static int get_lfps_half_cycle_clocks(const struct > intel_crtc_state *crtc_state) > 1000 / (2 * LFPS_CYCLE_COUNT); > } > > +#define ML_PHY_LOCK_LEN 252 > +#define ML_PHY_LOCK_LEN_UHBR 396 > + > static int get_tphy2_p2_to_p0(const struct intel_crtc_state *crtc_state) > { > - return 12 * 1000; > + struct intel_display *display = to_intel_display(crtc_state); > + > + return DISPLAY_VER(display) >= 35 ? (20 * 1000) : (12 * 1000); > } > > -static int get_establishment_period(const struct intel_crtc_state > *crtc_state) > +static int get_establishment_period(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > { > int t1 = 50 * 1000; > - int tps4 = 252; > + int tps4 = intel_dp_is_uhbr(crtc_state) ? (ML_PHY_LOCK_LEN_UHBR * 32) : > + (ML_PHY_LOCK_LEN * 10); > /* port_clock is link rate in 10kbit/s units */ > - int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock; > + int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock / 10; [Severity: High] Does 1000 * 1000 * tps4 overflow the 32-bit signed integer before division? For DP 2.1 UHBR, tps4 evaluates to 396 * 32 = 12672, yielding a product of 12,672,000,000. This exceeds the maximum 32-bit signed integer (2,147,483,647) and results in a negative intermediate value. This negative value will propagate through the division and subsequent time formulas, corrupting the wake times programmed into the hardware. > + int lttpr_count = 0; > int tcds, establishment_period; > > - tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * tml_phy_lock; > - establishment_period = (SILENCE_PERIOD_TIME + t1 + tcds); > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) { > + tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * > tml_phy_lock; > + } else { > + tcds = 7 * tml_phy_lock; > + lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps); > + } > + > + if (lttpr_count) { [Severity: Medium] Can drm_dp_lttpr_count() return a negative error code here? If drm_dp_lttpr_count() returns a negative error code (e.g., -EINVAL or -ERANGE), lttpr_count will evaluate to true in this check. The code will then proceed into this block and use the negative value in arithmetic like (lttpr_count - 1), causing a massive underflow in the calculation of tdcs, tds, and establishment_period. Should this explicitly check if (lttpr_count > 0)? > + int tlw = 13000; > + int tcs = 10000; > + int tlfps_period = get_lfps_cycle_time(crtc_state); > + int tdcs = (SILENCE_PERIOD_TIME + t1 + tcs + > + (lttpr_count - 1) * (tlw + tlfps_period)); > + int tacds = 70000; > + int tds = (lttpr_count - 1) * 7 * tml_phy_lock; > + > + /* tdrl is same as tcds*/ > + establishment_period = tlw + tlfps_period + tdcs + tacds + tds > + tcds; > + } else { > + /* TODO: Add a check for data realign by DPCD 0x116[3] */ > + > + establishment_period = (SILENCE_PERIOD_TIME + t1 + tcds); > + } > > return establishment_period; > } [ ... ] > @@ -161,7 +194,7 @@ _lnl_compute_aux_less_alpm_params(struct intel_dp > *intel_dp, > lfps_half_cycle; > > aux_less_wake_time = > - _lnl_compute_aux_less_wake_time(crtc_state); > + _lnl_compute_aux_less_wake_time(intel_dp, crtc_state); > aux_less_wake_lines = > intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, > aux_less_wake_time); > silence_period = get_silence_period_symbols(crtc_state); [Severity: Medium] Does the bounds check further down in this function still use the legacy mask? Looking at the rest of _lnl_compute_aux_less_alpm_params(): if (aux_less_wake_lines > ALPM_CTL_AUX_LESS_WAKE_TIME_MASK || ... return false; if (display->params.psr_safest_params) aux_less_wake_lines = ALPM_CTL_AUX_LESS_WAKE_TIME_MASK; Since this patch introduces ALPM_CTL_AUX_LESS_WAKE_TIME_MASK_XE3LPD for platforms with DISPLAY_VER >= 35, will these checks artificially limit the maximum wake lines to the older 6-bit mask (63) and potentially disable ALPM on newer hardware? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
