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

[Alex Deucher]

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).
>
> 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.