Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Logical inversion in the early return condition causes `intel_dp_update_lttpr_count` to unconditionally return for all display types, rendering the new feature completely inactive. - [Medium] Mismatch between comment and code for LTTPR revision check. --
--- Patch [10]: [PATCH v5 10/15] drm/i915/alpm: Program LTTPR count for DP 2.1 ALPM --- commit 2590778ff6f9e6755fe0231710bcb90018b982c4 Author: Animesh Manna <[email protected]> drm/i915/alpm: Program LTTPR count for DP 2.1 ALPM This commit adds an AUX write transaction to DP_TOTAL_LTTPR_CNT (0xf000a) with the total number of LTTPRs before link training for DP 2.1 ALPM. > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index cb92cff906146..1d5873cb64f55 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c [ ... ] > @@ -2265,6 +2265,20 @@ intel_dp_128b132b_link_train(struct intel_dp *intel_dp, > return passed; > } > > +static void intel_dp_update_lttpr_count(struct intel_dp *intel_dp, > + int lttpr_count) > +{ > + /* > + * Program only for DP2.1 and return if LTTPR revison is less than 0x20 > + * where 0:3 represents minor rev and 4:7 represent major rev > + */ [Severity: Medium] Does this comment contradict the logic implemented below? The comment states the function should return if the revision is "less than 0x20", while the code correctly checks for less than or equal to 0x20 to filter out DP 2.0. > + if (!intel_dp_is_edp(intel_dp) || > + intel_dp->lttpr_common_caps[0] <= 0x20) > + return; [Severity: High] Does this logic inadvertently cause an unconditional early return for all display types? For external DP displays where LTTPRs are used, !intel_dp_is_edp(intel_dp) evaluates to true, triggering the early return immediately. For eDP displays, !intel_dp_is_edp(intel_dp) evaluates to false, but since eDP bypasses LTTPR initialization in intel_dp_init_lttpr_and_dprx_caps(), intel_dp->lttpr_common_caps remains zero-initialized. This causes intel_dp->lttpr_common_caps[0] <= 0x20 to evaluate to true (since 0 is less than 0x20), which also triggers the early return. Was the intention to check intel_dp_is_edp(intel_dp) without the logical inversion to properly bypass eDP? > + > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TOTAL_LTTPR_CNT, lttpr_count); > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
