Lukas Wunner
2023-Nov-06 18:10 UTC
[Nouveau] [PATCH v2 8/9] PCI: Exclude PCIe ports used for tunneling in pcie_bandwidth_available()
On Fri, Nov 03, 2023 at 02:07:57PM -0500, Mario Limonciello wrote:> The USB4 spec specifies that PCIe ports that are used for tunneling > PCIe traffic over USB4 fabric will be hardcoded to advertise 2.5GT/s and > behave as a PCIe Gen1 device. The actual performance of these ports is > controlled by the fabric implementation. > > Downstream drivers such as amdgpu which utilize pcie_bandwidth_available() > to program the device will always find the PCIe ports used for > tunneling as a limiting factor potentially leading to incorrect > performance decisions. > > To prevent problems in downstream drivers check explicitly for ports > being used for PCIe tunneling and skip them when looking for bandwidth > limitations of the hierarchy. If the only device connected is a root port > used for tunneling then report that device.I think a better approach would be to define three new bandwidths for Thunderbolt in enum pci_bus_speed and add appropriate descriptions in pci_speed_string(). Those three bandwidths would be 10 GBit/s for Thunderbolt 1, 20 GBit/s for Thunderbolt 2, 40 GBit/s for Thunderbolt 3 and 4. Code to determine the Thunderbolt generation from the PCI ID already exists in tb_switch_get_generation(). This will not only address the amdgpu issue you're trying to solve, but also emit an accurate speed from __pcie_print_link_status(). The speed you're reporting with your approach is not necessarily accurate because the next non-tunneled device in the hierarchy might be connected with a far higher PCIe speed than what the Thunderbolt fabric allows. Thanks, Lukas
Mario Limonciello
2023-Nov-06 18:44 UTC
[Nouveau] [PATCH v2 8/9] PCI: Exclude PCIe ports used for tunneling in pcie_bandwidth_available()
On 11/6/2023 12:10, Lukas Wunner wrote:> On Fri, Nov 03, 2023 at 02:07:57PM -0500, Mario Limonciello wrote: >> The USB4 spec specifies that PCIe ports that are used for tunneling >> PCIe traffic over USB4 fabric will be hardcoded to advertise 2.5GT/s and >> behave as a PCIe Gen1 device. The actual performance of these ports is >> controlled by the fabric implementation. >> >> Downstream drivers such as amdgpu which utilize pcie_bandwidth_available() >> to program the device will always find the PCIe ports used for >> tunneling as a limiting factor potentially leading to incorrect >> performance decisions. >> >> To prevent problems in downstream drivers check explicitly for ports >> being used for PCIe tunneling and skip them when looking for bandwidth >> limitations of the hierarchy. If the only device connected is a root port >> used for tunneling then report that device. > > I think a better approach would be to define three new bandwidths for > Thunderbolt in enum pci_bus_speed and add appropriate descriptions in > pci_speed_string(). Those three bandwidths would be 10 GBit/s for > Thunderbolt 1, 20 GBit/s for Thunderbolt 2, 40 GBit/s for Thunderbolt 3 > and 4.It's an interesting idea, but there's a few short comings I can think of. 1) The USB4 specification doesn't actually require 40GB/s support, this is only a Thunderbolt 4 requirement. https://www.tomsguide.com/features/thunderbolt-4-vs-usb4-whats-the-difference The TBT/USB4 link speed can be discovered, but it's not a property of the *switch* not of the PCIe tunneling port. Tangentially related; the link speed is currently symmetric but there are two sysfs files. Mika left a comment in drivers/thunderbolt/switch.c it may be asymmetric in the future. So we may need to keep that in mind on any design that builds on top of them. On an AMD Phoenix system connected to a TBT3 Alpine Ridge based eGPU enclosure I can see: $ cat /sys/bus/thunderbolt/devices/1-0/generation 4 $ cat /sys/bus/thunderbolt/devices/1-2/generation 3 $ cat /sys/bus/thunderbolt/devices/1-2/tx_speed 20.0 Gb/s $ cat /sys/bus/thunderbolt/devices/1-2/rx_speed 20.0 Gb/s 2) This works until you end up with USB4v2 which supports 80GBit/s. So this might mean an extra 80GB/s enum and porting some variation of usb4_switch_version() outside of the thunderbolt driver so that PCI core can use it too.> > Code to determine the Thunderbolt generation from the PCI ID already exists > in tb_switch_get_generation().As 'thunderbolt' can be a module or built in, we need to bring code into PCI core so that it works in early boot before it loads. On the presumption that no more "new" TBT3 devices will be released to the market I suppose that *a lot* of that code could come over to PCI core especially if we can bring some variation of usb4_switch_version() over too. The other option is that we can stop allowing thunderbolt as a module and require it to be built-in. If we do this we can export symbols from it and use some of them in PCI core too.> > This will not only address the amdgpu issue you're trying to solve, > but also emit an accurate speed from __pcie_print_link_status(). > > The speed you're reporting with your approach is not necessarily > accurate because the next non-tunneled device in the hierarchy might > be connected with a far higher PCIe speed than what the Thunderbolt > fabric allows. > > Thanks, > > Lukas