Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/i915/display: intel_init_dpcd_quirks() never clears quirks.mask - [Low] drm/i915/display: misleading QUIRK_IGNORE_INTERLANE_ALIGN_BIT name and logic --
commit f2b8ff46914625ae07bb1d14ecb3a249c916384a Author: bitey <[email protected]> drm/i915/display: Add quirk for interlane align bit on CH7511 This commit adds a DPCD quirk matching the Chrontel CH7511 bridge on any platform and forces the per-lane and interlane alignment link status bits in intel_dp_needs_link_retrain() to prevent an endless retraining loop. > 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 ac1aff6edf052..5888f57422be2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c [ ... ] > @@ -2521,6 +2522,22 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp) > if (link_recovery_autoretrain_pending(link_training)) > return true; > > + /* > + * The CH7511 bridge incorrectly clears all link status bits > + * (INTERLANE_ALIGN_DONE, per-lane CR/CEQ/SYMLOCK) when disabling > + * the training pattern. Force them to indicate successful training. > + */ > + if (intel_has_dpcd_quirk(intel_dp, QUIRK_IGNORE_INTERLANE_ALIGN_BIT)) { > + u8 lane_eq = DP_LANE_CR_DONE | DP_LANE_CHANNEL_EQ_DONE | > + DP_LANE_SYMBOL_LOCKED; > + int i; > + > + for (i = 0; i < (intel_dp->lane_count + 1) / 2; i++) > + link_status[i] |= lane_eq | (lane_eq << 4); > + link_status[DP_LANE_ALIGN_STATUS_UPDATED - DP_LANE0_1_STATUS] |= > + DP_INTERLANE_ALIGN_DONE; > + } [Severity: Low] This isn't a bug, but since v4 forces all of the 8b/10b link status bits (DP_LANE_CR_DONE, DP_LANE_CHANNEL_EQ_DONE, DP_LANE_SYMBOL_LOCKED, and DP_INTERLANE_ALIGN_DONE) rather than just the interlane align bit, are QUIRK_IGNORE_INTERLANE_ALIGN_BIT, quirk_ignore_interlane_align_bit(), the log message, and the commit subject now misleadingly named? Also, instead of synthesizing every bit checked by drm_dp_channel_eq_ok() into link_status right before calling intel_dp_link_ok(), would it be clearer to return false early in intel_dp_needs_link_retrain() or check the quirk inside intel_dp_link_ok()? > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c > b/drivers/gpu/drm/i915/display/intel_quirks.c > index 7d7db774d8c7b..359bca48c6f66 100644 > --- a/drivers/gpu/drm/i915/display/intel_quirks.c > +++ b/drivers/gpu/drm/i915/display/intel_quirks.c > @@ -94,6 +94,14 @@ static void quirk_disable_edp_panel_replay(struct intel_dp > *intel_dp) > drm_info(display->drm, "Applying disable Panel Replay quirk\n"); > } > > +static void quirk_ignore_interlane_align_bit(struct intel_dp *intel_dp) > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + > + intel_set_dpcd_quirk(intel_dp, QUIRK_IGNORE_INTERLANE_ALIGN_BIT); > + drm_info(display->drm, "Applying ignore interlane align bit quirk\n"); > +} > + > static void quirk_disable_psr2(struct intel_display *display) > { > intel_set_quirk(display, QUIRK_DISABLE_PSR2); > @@ -289,6 +297,15 @@ static const struct intel_dpcd_quirk intel_dpcd_quirks[] > = { > .sink_oui = SINK_OUI(0x00, 0x22, 0xb9), > .hook = quirk_disable_edp_panel_replay, > }, > + /* Chrontel CH7511 DP-to-LVDS bridge */ > + { > + .device = DEVICE_ID_ANY, > + .subsystem_vendor = PCI_ANY_ID, > + .subsystem_device = PCI_ANY_ID, > + .sink_oui = SINK_OUI(0x2b, 0x02, 0xf0), > + .sink_device_id = SINK_DEVICE_ID('C', 'H', '7', '5', '1', '1'), > + .hook = quirk_ignore_interlane_align_bit, > + }, > }; [Severity: Medium] Unlike existing entries in intel_dpcd_quirks[] which match internal eDP panels on specific laptop PCI subsystem IDs, this entry uses DEVICE_ID_ANY and PCI_ANY_ID and matches external hotpluggable CH7511 DP-to-HDMI/LVDS adapters. On non-eDP connectors, intel_dp_detect() -> intel_dp_detect_dpcd() -> intel_dp_get_dpcd() runs on every connector detection: drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_get_dpcd() { ... if (!intel_dp_is_edp(intel_dp)) { u8 dprx; drm_dp_read_desc(&intel_dp->aux, &intel_dp->desc, drm_dp_is_branch(intel_dp->dpcd)); intel_init_dpcd_quirks(intel_dp, &intel_dp->desc.ident); ... } And intel_init_dpcd_quirks() never clears intel_dp->quirks.mask before scanning intel_dpcd_quirks[]: drivers/gpu/drm/i915/display/intel_quirks.c:intel_init_dpcd_quirks() { struct intel_display *display = to_intel_display(intel_dp); struct pci_dev *d = to_pci_dev(display->drm->dev); int i; for (i = 0; i < ARRAY_SIZE(intel_dpcd_quirks); i++) { ... q->hook(intel_dp); } } If a user unplugs a CH7511 adapter from an external DisplayPort connector and plugs in a normal DisplayPort monitor, won't QUIRK_IGNORE_INTERLANE_ALIGN_BIT remain permanently set in intel_dp->quirks.mask for that port (especially if drm_dp_read_desc() fails on the new sink without clearing intel_dp->desc), disabling 8b/10b link failure detection and retraining for any monitor connected afterwards? Also, while a CH7511 adapter remains connected to a non-eDP port, won't every connector probe re-run quirk_ignore_interlane_align_bit() and print "Applying ignore interlane align bit quirk" via drm_info()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
