On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> wrote: > > 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. The tricky part is that there were AMD and NVIDIA specific proprietary _PR3-like ACPI methods (plus whatever Apple did) prior to GPU power control standardizing on _PR3. Currently those methods are handled in the drivers directly, sort of tangled up with vga_switcheroo. I think ideally that logic would move to the ACPI core and be handled the same way as _PR3, but I'm not sure how well that would work because of the various bios date checks around _PR3 and the lack of general _PR3 support in those older platforms. So I think we still need some sort of "is this soldered in" check. Alex > > 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.
