Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Calculation of LFPS half cycle clocks incorrectly uses the link bit rate instead of the symbol clock, breaking UHBR configurations. - [High] Signed 32-bit integer overflow in `get_establishment_period()` causes ALPM wake times to be calculated as negative values. - [High] Premature integer division in `get_switch_to_active()` causes switch-to-active latency to incorrectly evaluate to 0 for UHBR20. -- --- Patch [15]: [PATCH v5 15/15] drm/i915/alpm: Program zero-based LFPS half cycle duration --- commit 6cd79e290a7ff8352dbda337c2c68d7456099444 Author: Animesh Manna <[email protected]> drm/i915/alpm: Program zero-based LFPS half cycle duration The LFPS Half Cycle Duration field, as well as the First and Last LFPS Half Cycle Duration fields, in PORT_ALPM_LFPS_CTL are zero-based: per Bspec the value programmed must be N_Symbol - 1. get_lfps_half_cycle_clocks() returned N_Symbol and programmed it directly into all three fields, making each half cycle one symbol clock too long. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index 5d2ac75615670..9768279f94ef5 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > @@ -123,8 +123,16 @@ static int get_lfps_cycle_count(const struct > intel_crtc_state *crtc_state) > > static int get_lfps_half_cycle_clocks(const struct intel_crtc_state > *crtc_state) > { > - return get_lfps_cycle_time(crtc_state) * crtc_state->port_clock / 1000 / > - 1000 / (2 * get_lfps_cycle_count(crtc_state)); > + int n_symbols = get_lfps_cycle_time(crtc_state) * > crtc_state->port_clock / > + 1000 / 1000 / (2 * get_lfps_cycle_count(crtc_state)); [Severity: High] This is a pre-existing issue, but does this correctly calculate the frequency multiplier for UHBR configurations? For UHBR encoding (128b/132b), the symbol clock in kHz is port_clock * 10 / 32, not port_clock. When used for UHBR20, using the inflated bit rate results in n_symbols - 1 = 59, which exceeds PORT_ALPM_LFPS_CTL_LAST_LFPS_HALF_CYCLE_DURATION_MASK (31). Could this cause validation to fail and completely disable ALPM on UHBR displays? > + > + /* > + * The LFPS half cycle duration fields (including the first and last > + * half cycle) are zero-based, i.e. the value programmed is N_Symbol - > 1. > + * > + * Bspec: 71632 > + */ > + return n_symbols - 1; > } [ ... ] > static int get_establishment_period(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state) > { [ ... ] > 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 / 10; [Severity: High] This isn't a bug introduced by this patch, but can this intermediate expression overflow? The sub-expression "1000 * 1000 * tps4" evaluates using signed 32-bit arithmetic. For non-UHBR links, tps4 is 2520, making the result 2,520,000,000. Since this exceeds INT_MAX (2,147,483,647), it wraps around to -1,774,967,296. Does this negative value propagate through to the final establishment_period, causing incorrect ALPM hardware programming? [ ... ] > static int get_switch_to_active(const struct intel_crtc_state *crtc_state) > { [ ... ] > 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] This is a pre-existing issue, but does this integer division evaluate to 0 for UHBR20 links? The expression "396 + 3 + 64" equals 463. For a UHBR20 link, symbol_clock is 625 MHz. The integer division 463 / 625 truncates to 0, resulting in an overall latency of 0 scanlines instead of the intended ~23 microseconds. Could this zero-latency configuration break the link state machine recovery for UHBR20 displays? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
