Re: [PATCH v4] drm/i915/display: Add quirk for interlane align bit on CH7511

[sashiko-bot@kernel.org]

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 <s.mavlenkov@mail.ru>

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/20261031174518.10004-1-s.mavlenkov@mail.ru?part=1