On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- > drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_vga.c | 4 +- > drivers/gpu/drm/radeon/radeon_device.c | 4 +- > drivers/gpu/drm/radeon/radeon_kms.c | 2 +- > drivers/pci/hotplug/pciehp_hpc.c | 6 +- > drivers/pci/pci-acpi.c | 15 ++++- > drivers/pci/pci.c | 17 +++-- > drivers/pci/probe.c | 52 ++++++++++++++- > drivers/pci/quirks.c | 84 +++++++++++++++++++++++++ > drivers/platform/x86/apple-gmux.c | 2 +- > drivers/thunderbolt/nhi.h | 2 - > include/linux/pci.h | 25 +------- > include/linux/pci_ids.h | 3 + > 14 files changed, 173 insertions(+), 47 deletions(-)That's an awful lot of additional LoC for what is primarily a refactoring job with the intent to simplify things. Honestly this looks like an attempt to fix something that isn't broken. Specifically, the is_thunderbolt bit apparently can't be removed without adding new bits to struct pci_dev. Not sure if that can be called progress. Thanks, Lukas
Limonciello, Mario
2022-Feb-15 19:07 UTC
[Nouveau] [PATCH v4 00/10] Overhaul `is_thunderbolt`
On 2/15/2022 01:29, Lukas Wunner wrote:> On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote: >> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- >> drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 2 +- >> drivers/gpu/drm/nouveau/nouveau_vga.c | 4 +- >> drivers/gpu/drm/radeon/radeon_device.c | 4 +- >> drivers/gpu/drm/radeon/radeon_kms.c | 2 +- >> drivers/pci/hotplug/pciehp_hpc.c | 6 +- >> drivers/pci/pci-acpi.c | 15 ++++- >> drivers/pci/pci.c | 17 +++-- >> drivers/pci/probe.c | 52 ++++++++++++++- >> drivers/pci/quirks.c | 84 +++++++++++++++++++++++++ >> drivers/platform/x86/apple-gmux.c | 2 +- >> drivers/thunderbolt/nhi.h | 2 - >> include/linux/pci.h | 25 +------- >> include/linux/pci_ids.h | 3 + >> 14 files changed, 173 insertions(+), 47 deletions(-) > > That's an awful lot of additional LoC for what is primarily > a refactoring job with the intent to simplify things.You may recall the first version of this series was just for adding USB4 matches to the existing code paths, and that's when it was noted that is_thunderbolt is a bit overloaded.> > Honestly this looks like an attempt to fix something that > isn't broken. Specifically, the is_thunderbolt bit apparently > can't be removed without adding new bits to struct pci_dev. > Not sure if that can be called progress. > > Thanks, > > LukasWithin this series there are two new material patches; setting up root ports for both integrated and discrete USB4 controllers to behave well with all the existing drivers that rely upon a hint of how they're connected to configure devices differently. If y'all collectively prefer this direction to not refactor is_thunderbolt and push into quirks, a simpler version of this series would be to leave all the quirks in place, just drop dev->is_thunderbolt, and set dev->external_facing on all 3 cases: * Intel TBT controller * USB4 integrated PCIe tunneling root port/XHCI tunneling root port * USB4 disctete PCIe tunneling root port/XHCI tunneling root port All the other drivers and symbols can stay the same then.
Mika Westerberg
2022-Feb-16 14:34 UTC
[Nouveau] [PATCH v4 00/10] Overhaul `is_thunderbolt`
Hi all, On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:> On 2/15/2022 01:29, Lukas Wunner wrote: > > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote: > > > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- > > > drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 2 +- > > > drivers/gpu/drm/nouveau/nouveau_vga.c | 4 +- > > > drivers/gpu/drm/radeon/radeon_device.c | 4 +- > > > drivers/gpu/drm/radeon/radeon_kms.c | 2 +- > > > drivers/pci/hotplug/pciehp_hpc.c | 6 +- > > > drivers/pci/pci-acpi.c | 15 ++++- > > > drivers/pci/pci.c | 17 +++-- > > > drivers/pci/probe.c | 52 ++++++++++++++- > > > drivers/pci/quirks.c | 84 +++++++++++++++++++++++++ > > > drivers/platform/x86/apple-gmux.c | 2 +- > > > drivers/thunderbolt/nhi.h | 2 - > > > include/linux/pci.h | 25 +------- > > > include/linux/pci_ids.h | 3 + > > > 14 files changed, 173 insertions(+), 47 deletions(-) > > > > That's an awful lot of additional LoC for what is primarily > > a refactoring job with the intent to simplify things. > > You may recall the first version of this series was just for adding > USB4 matches to the existing code paths, and that's when it was noted > that is_thunderbolt is a bit overloaded. > > > > > Honestly this looks like an attempt to fix something that > > isn't broken. Specifically, the is_thunderbolt bit apparently > > can't be removed without adding new bits to struct pci_dev. > > Not sure if that can be called progress. > > > Thanks, > > > > Lukas > > Within this series there are two new material patches; setting up root ports > for both integrated and discrete USB4 controllers to behave well with all > the existing drivers that rely upon a hint of how they're connected to > configure devices differently. > > If y'all collectively prefer this direction to not refactor is_thunderbolt > and push into quirks, a simpler version of this series would be to leave all > the quirks in place, just drop dev->is_thunderbolt, and set > dev->external_facing on all 3 cases: > > * Intel TBT controller > * USB4 integrated PCIe tunneling root port/XHCI tunneling root port > * USB4 disctete PCIe tunneling root port/XHCI tunneling root port > > All the other drivers and symbols can stay the same then.If I understand correctly the original intention of this patch series is to be able to differentiate whether the device is "permanently" connected to the motherboard, or it is connected over some hot-pluggable bus (PCIe, USB, USB4 for example but I'm sure there are other buses that fit into this picture too). Specifically this is needed for discrete GPUs because of power management differences or so (please correct me if I'm mistaken). If we set the is_thunderbolt debate aside and concentrate on that issue, I think the way to do this is to check whether the root port the GPU is connected to has an ACPI power resource (returned from _PR3() method). IF it is present then most likely the platform has provided all the necessary wiring to move the GPU into D3cold (and the BIOS knows this). If it is not present then the device cannot even go into D3cold as there is not means to power of the device in PCIe spec. Perhaps we can simply use pci_pr3_present() here as nouveau is already doing? Granted it is not too elegant solution either but better than using is_thunderbolt IMHO. Since this seem to be common for many GPUs, perhaps we can have a helper in DRM core that handles this. Then going back to is_thunderbolt debate :) I really don't think the drivers should care whether they are connected over a tunnel or not. They should work regardless of the underlying transport of the native protocol. They should also be prepared for the fact that the hardware can vanish under them at any point (e.g user unplugs the device). For this reason I don't really like to see is_thunderbolt to be used more and prefer to get rid if it completely if possible at all. If there is still need to differentiate whether the device can be hot-removed or not, I think "removable" in the driver core is the way to go. That is not dependent on any single transport.