Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The patch activates a dormant logic bug that causes Clock Recovery to instantly fail if the DP sink requests maximum pre-emphasis. - [High] The clock recovery loop counter `cr_loop` accumulates across different requested levels instead of tracking consecutive identical requests. -- commit 2ce38552c8226840ef5d2e4d8187e099b547ef5a Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Fix PE/VS value shift mismatch during link training This commit fixes a missing bit-shift for voltage swing and pre-emphasis values during DP link training. It shifts the raw values returned by the drm_dp_get_adjust_request_* functions so they can be properly used with the DPCD_*_SET macros. Link: https://lore.kernel.org/all/CAAMcf8D-d+5n=h44kekbsqwy42m+o32w+mo-r15vqwnyyhj...@mail.gmail.com/ > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 8cf6b73bceac0..5006ac181b2d7 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c [ ... ] > @@ -355,7 +357,9 @@ static int analogix_dp_process_clock_recovery(struct > analogix_dp_device *dp) > for (lane = 0; lane < lane_count; lane++) { > training_lane = analogix_dp_get_lane_link_training(dp, lane); > voltage_swing = drm_dp_get_adjust_request_voltage(link_status, > lane); > + voltage_swing >>= DP_TRAIN_VOLTAGE_SWING_SHIFT; > pre_emphasis = > drm_dp_get_adjust_request_pre_emphasis(link_status, lane); > + pre_emphasis >>= DP_TRAIN_PRE_EMPHASIS_SHIFT; > > if (DPCD_VOLTAGE_SWING_GET(training_lane) == voltage_swing && > DPCD_PRE_EMPHASIS_GET(training_lane) == pre_emphasis) [Severity: High] This is a pre-existing issue, but does this lack of reset cause premature link training failures? In analogix_dp_process_clock_recovery(), cr_loop[lane] is incremented when the requested levels match the applied levels. However, if the DP sink requests a new level, the counter is not reset to 0. It appears to accumulate counts across different levels during the entire clock recovery phase. If the sink steps through multiple levels and hits the MAX_CR_LOOP limit in total (e.g., 2 loops at level 1, 2 loops at level 2, 1 loop at level 3), could it incorrectly trigger the abort condition below? [Severity: High] This isn't a bug introduced by this patch, but does fixing the shift logic activate a dormant bug right below this check? Immediately after the snippet above in analogix_dp_process_clock_recovery(), the code checks: if (dp->link_train.cr_loop[lane] == MAX_CR_LOOP || voltage_swing == VOLTAGE_LEVEL_3 || pre_emphasis == PRE_EMPHASIS_LEVEL_3) { dev_err(dp->dev, "CR Max reached (%d,%d,%d)\n", ...); analogix_dp_reduce_link_rate(dp); return -EIO; } Before this patch, pre_emphasis evaluated to 24, which bypassed this abort. Now that the value is properly shifted, if the sink legitimately requests PRE_EMPHASIS_LEVEL_3 (which evaluates to 3), won't the driver immediately return -EIO and abort clock recovery instead of applying the maximum level and setting the MAX_PRE_EMPHASIS_REACHED bit? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
