Re: [RFC PATCH] thunderbolt: Tunnel only the first DP OUT of Apple Studio Displays

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] thunderbolt: tb_dp_out_is_spare_tile() breaks dual-SST 5K hosts
- [Low] thunderbolt: hardcoded DROM IDs and late check in
  tb_find_dp_out()
--

commit 56e4b7f54ea3c7e7cdb974a602ec3a77741d76e3
Author: Duff OMelia <duff@omelia.org>

thunderbolt: Tunnel only the first DP OUT of Apple Studio Displays

This commit skips tunneling any DisplayPort output adapter after the first
one on Apple Studio Displays (vendor 0x1, devices 0x801f and 0x8025) in
tb_find_dp_out().

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 57c11ba13459c..b015a71f4048d 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -1862,6 +1862,27 @@ static struct tb_port *tb_find_pcie_down(struct tb_switch *sw,
>  	return tb_find_unused_port(sw, TB_TYPE_PCIE_DOWN);
>  }
>  
> +/*
> + * A Studio Display has a DP OUT adapter for each half of its panel, but a
> + * host can drive the whole 5K panel over the first one alone. Tunneling the
> + * second only takes a DP IN adapter that another display could have used.
> + */
> +static bool tb_dp_out_is_spare_tile(struct tb_port *port)
> +{
> +	struct tb_switch *sw = port->sw;
> +	struct tb_port *p;
> +
> +	if (sw->vendor != 0x1 || (sw->device != 0x801f && sw->device != 0x8025))
> +		return false;
> +
> +	tb_switch_for_each_port(sw, p) {
> +		if (tb_port_is_dpout(p))
> +			return p != port;
> +	}
> +
> +	return false;
> +}

[Severity: High]
Does unconditionally skipping the second DisplayPort output adapter in
tb_dp_out_is_spare_tile() cause a regression on hosts or links that drive
the 5120x2880@60Hz panel as a 2x1 tiled display across two 2560x2880
streams?

Driving 5120x2880@60Hz over the first adapter alone requires over 21 Gbps
uncompressed, so it depends on Display Stream Compression and single-stream
mode selection in the host graphics driver.  On hosts without compression,
on Thunderbolt 3 links limited to 4-lane HBR2, or with drivers such as i915
that configure both tiles, skipping the second adapter even when free
DisplayPort input adapters exist prevents the second tunnel from being
created and leaves half the panel dark or limits the resolution.

>  static struct tb_port *tb_find_dp_out(struct tb *tb, struct tb_port *in)
>  {
>  	struct tb_port *host_port, *port;
> @@ -1879,6 +1900,11 @@ static struct tb_port *tb_find_dp_out(struct tb *tb, struct tb_port *in)
>  			continue;
>  		}
>  
> +		if (tb_dp_out_is_spare_tile(port)) {
> +			tb_port_dbg(port, "skipping spare DP OUT tile\n");
> +			continue;
> +		}

[Severity: Low]
This isn't a bug, but device-specific workarounds matching sw->vendor and
sw->device belong in tb_quirks[] in drivers/thunderbolt/quirks.c rather than
hardcoded in tb_dp_out_is_spare_tile().  Also, checking
tb_dp_out_is_spare_tile(port) after tb_port_is_enabled(port) performs an
unnecessary hardware config space read on the skipped port on every
tb_find_dp_out() pass.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106154445.517940-1-duff@omelia.org?part=1