| Message ID | c42a235e126f273e9b8712a7154f813f757db507.1487938188.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote: > Detect on probe whether a PCI device is part of a Thunderbolt controller. > > Intel uses a Vendor-Specific Extended Capability (VSEC) with ID 0x1234 > on such devices. Detect presence of this VSEC and cache it in a newly > added is_thunderbolt bit in struct pci_dev. Add a helper to check > whether a given PCI device is situated on a Thunderbolt daisy chain. > > The necessity arises from the following: > > * To power off Thunderbolt controllers on Macs even if their BIOS is > older than 2015, their PCIe ports need to be whitelisted for runtime > PM. For this we need a way to recognize them. > > * Dual GPU MacBook Pros introduced 2011+ can no longer switch external > DisplayPort ports between GPUs. (They're no longer just used for DP > but have become combined DP/Thunderbolt ports.) The driver to switch > the ports, drivers/platform/x86/apple-gmux.c, needs to detect presence > of a Thunderbolt controller and, if found, keep external ports > permanently switched to the discrete GPU. > > * If an external Thunderbolt GPU is connected to a dual GPU laptop (Mac > or not), that GPU is currently registered with vga_switcheroo even > though it can neither drive the laptop's panel nor be powered off by > the platform. To vga_switcheroo it will appear as if two discrete > GPUs are present. As a result, when the external GPU is runtime > suspended, vga_switcheroo will cut power to the internal discrete GPU > which may not be runtime suspended at all at this moment. The > solution is to not register external GPUs with vga_switcheroo, which > necessitates a way to recognize if they're on a Thunderbolt daisy > chain. If I understand correctly, vga_switcheroo manages two GPUs that have a single output: either there's a mux that connects one GPU or the other to the output, or one GPU is permanently connected to the output and the other does offline rendering. To this non-GPU person, it sounds like the important question is whether two GPUs are related in this way (either they feed the same mux, or there's some special offline rendering connection between them). That sounds unrelated to the question of how the GPUs themselves are connected to the PCI hierarchy. From a pure PCI perspective, I assume it would be conceivable to have two Thunderbolt-connected GPUs feeding into a mux. Or to have a GPU (unrelated to the mux) in a non-Thunderbolt plugin slot or connected externally via a non-Thunderbolt switch and an iPass cable. If that's the case, and we're using Thunderbolt-connectedness as a hint that happens to work for the hardware we know about now, that's fine -- I'm just trying to understand what's a hint and what's intrisic to being connected via Thunderbolt. > Cc: Andreas Noever <andreas.noever@gmail.com> > Cc: Michael Jamet <michael.jamet@intel.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Amir Levy <amir.jer.levy@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pci.h | 2 ++ > drivers/pci/probe.c | 21 +++++++++++++++++++++ > include/linux/pci.h | 23 +++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index cb17db242f30..45c2b8144911 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -3,6 +3,8 @@ > > #define PCI_FIND_CAP_TTL 48 > > +#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ > + > extern const unsigned char pcie_link_speed[]; > > bool pcie_cap_has_lnkctl(const struct pci_dev *dev); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 204960e70333..7963ecc6d85f 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1208,6 +1208,24 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) > pdev->is_hotplug_bridge = 1; > } > > +static void set_pcie_thunderbolt(struct pci_dev *dev) > +{ > + int vsec = 0; > + u32 header; > + > + while ((vsec = pci_find_next_ext_capability(dev, vsec, > + PCI_EXT_CAP_ID_VNDR))) { > + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header); > + > + /* Is the device part of a Thunderbolt controller? */ > + if (dev->vendor == PCI_VENDOR_ID_INTEL && > + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) { > + dev->is_thunderbolt = 1; > + return; > + } > + } > +} > + > /** > * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? > * @dev: PCI device > @@ -1360,6 +1378,9 @@ int pci_setup_device(struct pci_dev *dev) > /* need to have dev->class ready */ > dev->cfg_size = pci_cfg_space_size(dev); > > + /* need to have dev->cfg_size ready */ > + set_pcie_thunderbolt(dev); > + > /* "Unknown power state" */ > dev->current_state = PCI_UNKNOWN; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2d1a124216a..36dfcfd946f4 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -358,6 +358,7 @@ struct pci_dev { > unsigned int is_virtfn:1; > unsigned int reset_fn:1; > unsigned int is_hotplug_bridge:1; > + unsigned int is_thunderbolt:1; /* Thunderbolt controller */ I'm not really keen on having this in the PCI core because the core doesn't need this or even know what it means. pci_find_next_ext_capability() is available to drivers, and if Thunderbolt-connectedness is useful information to apple-gmux or GPU drivers, it's fine with me if you want to use it there. I just don't see the benefit to having it in the core. > unsigned int __aer_firmware_first_valid:1; > unsigned int __aer_firmware_first:1; > unsigned int broken_intx_masking:1; > @@ -2171,6 +2172,28 @@ static inline bool pci_ari_enabled(struct pci_bus *bus) > return bus->self && bus->self->ari_enabled; > } > > +/** > + * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain > + * @pdev: PCI device to check > + * > + * Walk upwards from @pdev and check for each encountered bridge if it's > + * part of a Thunderbolt controller. Reaching the host bridge means @pdev > + * is soldered to the mainboard. The comment suggests this returns false only if @pdev is soldered to the mainboard. I don't think that's the case. This will return true for a Thunderbolt controller and all devices downstream from it. It will return false for all others, whether they're soldered down or not. > + */ > +static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev) > +{ > + struct pci_dev *parent = pdev; > + > + if (pdev->is_thunderbolt) > + return true; > + > + while ((parent = pci_upstream_bridge(parent))) > + if (parent->is_thunderbolt) > + return true; > + > + return false; > +} > + > /* provide the legacy pci_dma_* API */ > #include <linux/pci-dma-compat.h> > > -- > 2.11.0 >
On Fri, Feb 24, 2017 at 04:17:24PM -0600, Bjorn Helgaas wrote: > On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote: > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -358,6 +358,7 @@ struct pci_dev { > > unsigned int is_virtfn:1; > > unsigned int reset_fn:1; > > unsigned int is_hotplug_bridge:1; > > + unsigned int is_thunderbolt:1; /* Thunderbolt controller */ > > I'm not really keen on having this in the PCI core because the core > doesn't need this or even know what it means. > > pci_find_next_ext_capability() is available to drivers, and if > Thunderbolt-connectedness is useful information to apple-gmux or GPU > drivers, it's fine with me if you want to use it there. I just don't > see the benefit to having it in the core. The above contradicts your statement 3 days earlier: "Assuming we need it, having it in struct pci_dev is fine. There's no point in looking up the VSEC capability more than once." (http://www.spinics.net/lists/linux-pci/msg58532.html) Please explain. Thanks, Lukas
On Sat, Feb 25, 2017 at 08:40:03AM +0100, Lukas Wunner wrote: > On Fri, Feb 24, 2017 at 04:17:24PM -0600, Bjorn Helgaas wrote: > > On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote: > > > --- a/include/linux/pci.h > > > +++ b/include/linux/pci.h > > > @@ -358,6 +358,7 @@ struct pci_dev { > > > unsigned int is_virtfn:1; > > > unsigned int reset_fn:1; > > > unsigned int is_hotplug_bridge:1; > > > + unsigned int is_thunderbolt:1; /* Thunderbolt controller */ > > > > I'm not really keen on having this in the PCI core because the core > > doesn't need this or even know what it means. > > > > pci_find_next_ext_capability() is available to drivers, and if > > Thunderbolt-connectedness is useful information to apple-gmux or GPU > > drivers, it's fine with me if you want to use it there. I just don't > > see the benefit to having it in the core. > > The above contradicts your statement 3 days earlier: > > "Assuming we need it, having it in struct pci_dev is fine. > There's no point in looking up the VSEC capability more than once." > (http://www.spinics.net/lists/linux-pci/msg58532.html) It's clear that none of the proposed uses is related to Thunderbolt as a technology, which is why I would suggest we don't need the flag. But in the interest of moving on, if you remove the changelog part about whitelisting Thunderbolt for runtime PM (since that's not part of this series), you can add my: Acked-by: Bjorn Helgaas <bhelgaas@google.com>
On Fri, Feb 24, 2017 at 04:17:24PM -0600, Bjorn Helgaas wrote: > On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote: > > Detect on probe whether a PCI device is part of a Thunderbolt controller. [...] > > * If an external Thunderbolt GPU is connected to a dual GPU laptop (Mac > > or not), that GPU is currently registered with vga_switcheroo even > > though it can neither drive the laptop's panel nor be powered off by > > the platform. To vga_switcheroo it will appear as if two discrete > > GPUs are present. As a result, when the external GPU is runtime > > suspended, vga_switcheroo will cut power to the internal discrete GPU > > which may not be runtime suspended at all at this moment. The > > solution is to not register external GPUs with vga_switcheroo, which > > necessitates a way to recognize if they're on a Thunderbolt daisy > > chain. > > If I understand correctly, vga_switcheroo manages two GPUs that have a > single output: either there's a mux that connects one GPU or the other > to the output, or one GPU is permanently connected to the output and > the other does offline rendering. There are two aspects to hybrid graphics, switching the panel between GPUs and powering off the discrete GPU. (Some laptops can also switch external DP ports between GPUs and some, as you say, cannot switch the panel and only use the discrete GPU for render offloading.) > To this non-GPU person, it sounds like the important question is > whether two GPUs are related in this way (either they feed the same > mux, or there's some special offline rendering connection between > them). That sounds unrelated to the question of how the GPUs > themselves are connected to the PCI hierarchy. To the best of my knowledge there's no definite way to determine whether two GPUs are connected to the same panel via a mux. There is also no such thing as a special render offloading connection: Frames are computed on a discrete GPU, then copied over PCIe into the framebuffer of the integrated GPU. Whether that discrete GPU is on-board or externally connected is completely transparent and not discernible other than by looking at the PCI hierarchy. The same issue exists for HD Audio controllers integrated into many discrete GPUs: To the OS these look like separate PCI functions and in sound/pci/hda/hda_intel.c:get_bound_vga() we leverage the fact that the GPU is always function 0 and the HD Audio is function 1 to discover the GPU corresponding to a particular HD Audio controller. So again that relationship is deduced from the PCI hierarchy. > From a pure PCI perspective, I assume it would be conceivable to have > two Thunderbolt-connected GPUs feeding into a mux. Or to have a GPU > (unrelated to the mux) in a non-Thunderbolt plugin slot or connected > externally via a non-Thunderbolt switch and an iPass cable. Technically such products would be possible, but I believe they don't exist. (Unlike Thunderbolt eGPUs, which have been on the market for a few years.) Thanks, Lukas
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index cb17db242f30..45c2b8144911 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -3,6 +3,8 @@ #define PCI_FIND_CAP_TTL 48 +#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ + extern const unsigned char pcie_link_speed[]; bool pcie_cap_has_lnkctl(const struct pci_dev *dev); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 204960e70333..7963ecc6d85f 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1208,6 +1208,24 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) pdev->is_hotplug_bridge = 1; } +static void set_pcie_thunderbolt(struct pci_dev *dev) +{ + int vsec = 0; + u32 header; + + while ((vsec = pci_find_next_ext_capability(dev, vsec, + PCI_EXT_CAP_ID_VNDR))) { + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header); + + /* Is the device part of a Thunderbolt controller? */ + if (dev->vendor == PCI_VENDOR_ID_INTEL && + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) { + dev->is_thunderbolt = 1; + return; + } + } +} + /** * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? * @dev: PCI device @@ -1360,6 +1378,9 @@ int pci_setup_device(struct pci_dev *dev) /* need to have dev->class ready */ dev->cfg_size = pci_cfg_space_size(dev); + /* need to have dev->cfg_size ready */ + set_pcie_thunderbolt(dev); + /* "Unknown power state" */ dev->current_state = PCI_UNKNOWN; diff --git a/include/linux/pci.h b/include/linux/pci.h index e2d1a124216a..36dfcfd946f4 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -358,6 +358,7 @@ struct pci_dev { unsigned int is_virtfn:1; unsigned int reset_fn:1; unsigned int is_hotplug_bridge:1; + unsigned int is_thunderbolt:1; /* Thunderbolt controller */ unsigned int __aer_firmware_first_valid:1; unsigned int __aer_firmware_first:1; unsigned int broken_intx_masking:1; @@ -2171,6 +2172,28 @@ static inline bool pci_ari_enabled(struct pci_bus *bus) return bus->self && bus->self->ari_enabled; } +/** + * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain + * @pdev: PCI device to check + * + * Walk upwards from @pdev and check for each encountered bridge if it's + * part of a Thunderbolt controller. Reaching the host bridge means @pdev + * is soldered to the mainboard. + */ +static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev) +{ + struct pci_dev *parent = pdev; + + if (pdev->is_thunderbolt) + return true; + + while ((parent = pci_upstream_bridge(parent))) + if (parent->is_thunderbolt) + return true; + + return false; +} + /* provide the legacy pci_dma_* API */ #include <linux/pci-dma-compat.h>
Detect on probe whether a PCI device is part of a Thunderbolt controller. Intel uses a Vendor-Specific Extended Capability (VSEC) with ID 0x1234 on such devices. Detect presence of this VSEC and cache it in a newly added is_thunderbolt bit in struct pci_dev. Add a helper to check whether a given PCI device is situated on a Thunderbolt daisy chain. The necessity arises from the following: * To power off Thunderbolt controllers on Macs even if their BIOS is older than 2015, their PCIe ports need to be whitelisted for runtime PM. For this we need a way to recognize them. * Dual GPU MacBook Pros introduced 2011+ can no longer switch external DisplayPort ports between GPUs. (They're no longer just used for DP but have become combined DP/Thunderbolt ports.) The driver to switch the ports, drivers/platform/x86/apple-gmux.c, needs to detect presence of a Thunderbolt controller and, if found, keep external ports permanently switched to the discrete GPU. * If an external Thunderbolt GPU is connected to a dual GPU laptop (Mac or not), that GPU is currently registered with vga_switcheroo even though it can neither drive the laptop's panel nor be powered off by the platform. To vga_switcheroo it will appear as if two discrete GPUs are present. As a result, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU which may not be runtime suspended at all at this moment. The solution is to not register external GPUs with vga_switcheroo, which necessitates a way to recognize if they're on a Thunderbolt daisy chain. Cc: Andreas Noever <andreas.noever@gmail.com> Cc: Michael Jamet <michael.jamet@intel.com> Cc: Tomas Winkler <tomas.winkler@intel.com> Cc: Amir Levy <amir.jer.levy@intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/pci.h | 2 ++ drivers/pci/probe.c | 21 +++++++++++++++++++++ include/linux/pci.h | 23 +++++++++++++++++++++++ 3 files changed, 46 insertions(+)