Hi Mario, On Wed, Feb 16, 2022 at 10:50:31AM -0600, Limonciello, Mario wrote: > On 2/16/2022 08:44, Alex Deucher wrote: > > 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). > > Correct. It might be possible to drop the patch for the integrated case > (patch 3) because I do think that by Microsoft having the _DSD for > "ExternalFacingPort" it's very likely that most implementations will have > used it for the appropriate PCIe root ports. If something shows up in the > wild that this isn't the case it could be revisited. If it's found > pre-production presumably the OEM can still fix it and if it's post > production and there are problems we can dust it off then. Yeah, that's most likely the case. > The discrete USB4 controller I would be more concerned that this isn't > populated, and that (patch 4) should be more important to let the driver > core set it removable. Agreed. [I actually only now noticed that the PCI core actually already marks devices connected to external facing ports as "removable" in pci_set_removable().] > > > 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. > > Considering that limitation if `dev->external_facing` already exists in PCI > core may as well use it for this instead of `is_thunderbolt`. Indeed. > > 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. > > Hopefully that is what the patch series does right now as of v4. As I It does yes. I think the detection of internal and discrete tunneled ports can be dropped from this series for now to make this leaner. We can add those later when needed.
