From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/i915/dp: Fix link retrain loop on pre-gen8 platforms with DP adapters Date: Tue, 05 May 2026 10:51:25 +1000 Message-ID: In-Reply-To: <20260430030846.29373-1-s.mavlenkov@mail.ru> References: <20260430030846.29373-1-s.mavlenkov@mail.ru> <20260430030846.29373-1-s.mavlenkov@mail.ru> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review #### Build error: `display` variable declared in wrong function The first hunk adds `display` to `intel_dp_read_link_status()`: ```c @@ -5676,6 +5676,7 @@ static int intel_dp_read_link_status(struct intel_dp *intel_dp, u8 link_status[DP_LINK_STATUS_SIZE]) { + struct intel_display *display =3D to_intel_display(intel_dp); int err; ``` But the second hunk uses `display` inside `intel_dp_needs_link_retrain()`, = which is a **separate function** starting at line 5675: ```c @@ -5712,7 +5713,8 @@ static bool intel_dp_needs_link_retrain(struct intel_= dp *intel_dp) if (intel_dp->link.retrain_disabled) return false; =20 - if (intel_dp->link.seq_train_failures) + if (DISPLAY_VER(display) >=3D 8 && + intel_dp->link.seq_train_failures) return true; ``` This will cause two compilation errors: 1. **Unused variable warning/error** in `intel_dp_read_link_status()` (wher= e `display` is declared but never referenced). 2. **Undeclared identifier error** in `intel_dp_needs_link_retrain()` (wher= e `display` is used but never declared). The `display` variable declaration should be moved from `intel_dp_read_link= _status()` to `intel_dp_needs_link_retrain()`. #### Design concern: `intel_dp_short_pulse()` change disables link checking= too broadly ```c - esi[3] |=3D LINK_STATUS_CHANGED; + if (DISPLAY_VER(display) >=3D 8) + esi[3] |=3D LINK_STATUS_CHANGED; ``` The existing code force-sets `LINK_STATUS_CHANGED` specifically to cover si= nks with `DPCD_REV < 1.2` that don't report this bit themselves. The existi= ng TODO comment right above this line says: ```c * TODO: let the link status check depend on LINK_STATUS_CHANGED * or intel_dp->link.force_retrain for DPCD_REV >=3D 1.2 ``` Gating this on `DISPLAY_VER >=3D 8` means pre-gen8 platforms with DPCD 1.1 = sinks will never have their link status checked on short HPD =E2=80=94 even= for genuine link degradation, not just the Chrontel adapter problem. A mor= e targeted guard would be to check `intel_dp->dpcd[DP_DPCD_REV] >=3D DP_DPC= D_REV_12` (which is what the TODO itself suggests), or to detect/quirk the = specific misbehaving adapter via its OUI. #### Design concern: `intel_dp_needs_link_retrain()` change ```c - if (intel_dp->link.seq_train_failures) + if (DISPLAY_VER(display) >=3D 8 && + intel_dp->link.seq_train_failures) return true; ``` On pre-gen8, `seq_train_failures > 0` will no longer trigger a retrain. The= code will fall through to the `intel_dp_link_ok()` check, which reads DPCD= link status. If the adapter reports non-standard DPCD (as noted in the com= mit message), this may still trigger a retrain via that path. Whether this = fully breaks the loop depends on what the Chrontel adapter reports =E2=80= =94 worth verifying in testing. #### `intel_dp_stop_link_train()` change looks reasonable ```c - if (!display->hotplug.ignore_long_hpd && + if (DISPLAY_VER(display) >=3D 8 && + !display->hotplug.ignore_long_hpd && intel_dp->link.seq_train_failures < MAX_SEQ_TRAIN_FAILURES) { ``` This is the most reasonable of the three changes. The post-training link ch= eck queue was added for modern platforms and is the primary entry point int= o the retrain loop. Skipping it on pre-gen8 platforms is a sensible and low= -risk change. The `display` variable is already declared in this function, = so no scoping issue here. #### Signed-off-by issue ``` Signed-off-by: bitey ``` The kernel DCO requires a legal name in the `Signed-off-by` line, not a han= dle/nickname. #### Summary The patch must be resubmitted with at minimum: 1. The `display` variable declaration moved to the correct function (`intel= _dp_needs_link_retrain`, not `intel_dp_read_link_status`). 2. A real name in the `Signed-off-by` tag. Ideally, the `intel_dp_short_pulse()` change should be reconsidered =E2=80= =94 either guarding on `DPCD_REV` instead of `DISPLAY_VER`, or quirking the= specific adapter, to avoid disabling legitimate link checks on all pre-gen= 8 platforms. --- Generated by Claude Code Patch Reviewer