[Nouveau] [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Mika Westerberg]

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 at linux.intel.com> 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.