Thunderbolt GPU fixes

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo.

In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

Cc Ben Skeggs <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_vga.c +0 -0

index eef22c6b9665..c2a7fd606c2e 100644

drivers/gpu/drm/nouveau/nouveau_vga.c +6 -0

+
        if (nouveau_runtime_pm == 1)
                runtime = true;
        if ((nouveau_runtime_pm == -1) && (nouveau_is_optimus() || nouveau_is_v1_dsm()))

@@ -111,6 +115,11 @@ nouveau_vga_fini(struct nouveau_drm *drm)
        struct drm_device *dev = drm->dev;
        bool runtime = false;
 
+       vga_client_register(dev->pdev, NULL, NULL, NULL);
+
+       if (pci_is_thunderbolt_attached(dev->pdev))
+               return;
+
        if (nouveau_runtime_pm == 1)
                runtime = true;
        if ((nouveau_runtime_pm == -1) && (nouveau_is_optimus() || nouveau_is_v1_dsm()))

drivers/gpu/drm/nouveau/nouveau_vga.c +0 -1

 }
 
 
--

2.11.0

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.

From a pure PCI perspective, I assume it would be conceivable to have

Cc Andreas Noever <[email protected]>

Cc Michael Jamet <[email protected]>

Cc Tomas Winkler <[email protected]>

Cc Amir Levy <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/pci/pci.h   |  2 ++
 drivers/pci/probe.c | 21 +++++++++++++++++++++
 include/linux/pci.h | 23 +++++++++++++++++++++++
 3 files changed, 46 insertions(+)

drivers/pci/pci.h +2 -0

index cb17db242f30..45c2b8144911 100644
@@ -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);

drivers/pci/probe.c +21 -0

index 204960e70333..7963ecc6d85f 100644
@@ -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;
 

include/linux/pci.h +0 -0

index e2d1a124216a..36dfcfd946f4 100644

include/linux/pci.h +0 -0

 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
 	unsigned int	broken_intx_masking:1;

include/linux/pci.h +15 -0

+ */
+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>
 

Signature · 1 line

2.11.0

include/linux/pci.h struct pci_dev { · line 361

Quoted 7 lines · Lukas Wunner

include/linux/pci.h +1 -0

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

Quoted 12 lines · Bjorn Helgaas

include/linux/pci.h struct pci_dev { · line 361

Quoted 7 lines · Lukas Wunner

include/linux/pci.h +1 -0

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

Quoted 4 lines

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)

Quoted 3 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo.

Quoted 3 lines

In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

Why does suspending the external GPU cause vga_switcheroo to cut power to the internal GPU? No doubt this would be obvious to a GPU person, which I definitely am not.

Quoted 6 lines

drivers/gpu/drm/nouveau/nouveau_vga.c +0 -0

@@ -95,6 +95,10 @@ nouveau_vga_init(struct nouveau_drm *drm)
 
        vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
 

drivers/gpu/drm/nouveau/nouveau_vga.c nouveau_vga_init(struct nouveau_drm *drm) · line 100

Quoted 3 lines

drivers/gpu/drm/nouveau/nouveau_vga.c +3 -0

+       /* don't register Thunderbolt eGPU with vga_switcheroo */
+       if (pci_is_thunderbolt_attached(dev->pdev))
+               return;

I guess there's no way to move this inside vga_switcheroo_register_client() instead of putting the test in all the drivers?

Quoted 13 lines

drivers/gpu/drm/nouveau/nouveau_vga.c +5 -0

@@ -111,6 +115,11 @@ nouveau_vga_fini(struct nouveau_drm *drm)
        struct drm_device *dev = drm->dev;
        bool runtime = false;
 
+       vga_client_register(dev->pdev, NULL, NULL, NULL);
+
+       if (pci_is_thunderbolt_attached(dev->pdev))
+               return;
+
        if (nouveau_runtime_pm == 1)
                runtime = true;
        if ((nouveau_runtime_pm == -1) && (nouveau_is_optimus() || nouveau_is_v1_dsm()))

drivers/gpu/drm/nouveau/nouveau_vga.c nouveau_vga_fini(struct nouveau_drm *drm) · line 122

Quoted 4 lines

drivers/gpu/drm/nouveau/nouveau_vga.c +0 -1

@@ -119,7 +128,6 @@ nouveau_vga_fini(struct nouveau_drm *drm)
        vga_switcheroo_unregister_client(dev->pdev);
        if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
                vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
-       vga_client_register(dev->pdev, NULL, NULL, NULL);

The amd & radeon patches look like this:

(quoted diff) +2 -1

-       vga_switcheroo_unregister_client(rdev->pdev);
+       if (!pci_is_thunderbolt_attached(adev->pdev))
+               vga_switcheroo_unregister_client(adev->pdev);

This nouveau patch looks suspiciously different. But again, I'm not a GPU guy so all I can really say is "hmm, I wonder why it's different?"

Quoted 1 line · Lukas Wunner

Detect on probe whether a PCI device is part of a Thunderbolt controller.

Quoted 10 lines

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

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

Cc Alex Deucher <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/radeon/radeon_device.c | 7 +++++--
 drivers/gpu/drm/radeon/radeon_kms.c    | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

drivers/gpu/drm/radeon/radeon_device.c +5 -2

index 4b0c388be3f5..27be17f0b227 100644
@@ -1471,7 +1471,9 @@ int radeon_device_init(struct radeon_device *rdev,
 
        if (rdev->flags & RADEON_IS_PX)
                runtime = true;
-       vga_switcheroo_register_client(rdev->pdev, &radeon_switcheroo_ops, runtime);
+       if (!pci_is_thunderbolt_attached(rdev->pdev))
+               vga_switcheroo_register_client(rdev->pdev,
+                                              &radeon_switcheroo_ops, runtime);
        if (runtime)
                vga_switcheroo_init_domain_pm_ops(rdev->dev, &rdev->vga_pm_domain);
 

@@ -1564,7 +1566,8 @@ void radeon_device_fini(struct radeon_device *rdev)
        /* evict vram memory */
        radeon_bo_evict_vram(rdev);
        radeon_fini(rdev);
-       vga_switcheroo_unregister_client(rdev->pdev);
+       if (!pci_is_thunderbolt_attached(rdev->pdev))
+               vga_switcheroo_unregister_client(rdev->pdev);
        if (rdev->flags & RADEON_IS_PX)
                vga_switcheroo_fini_domain_pm_ops(rdev->dev);
        vga_client_register(rdev->pdev, NULL, NULL, NULL);

drivers/gpu/drm/radeon/radeon_kms.c +2 -2

index 56f35c06742c..e95ceec1c97a 100644
@@ -115,7 +115,8 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags)
 
        if ((radeon_runtime_pm != 0) &&
            radeon_has_atpx() &&
-           ((flags & RADEON_IS_IGP) == 0))
+           ((flags & RADEON_IS_IGP) == 0) &&
+           !pci_is_thunderbolt_attached(rdev->pdev))
                flags |= RADEON_IS_PX;
 
        /* radeon_device_init should report only fatal error
--

2.11.0

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I'm not necessarily opposed to this, but I'd prefer something more generic. E.g., what happens if someone uses another dGPU in a docking station or some other sort of PCIe bridge?

I think on AMD platforms
at least we should be able to determine what devices are the switcheroo devices based on information in the ATIF and ATPX ACPI methods. In that case, we can be explicit in which devices we register with vga_switcheroo.

Quoted 10 lines · Alex Deucher

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I'm not necessarily opposed to this, but I'd prefer something more generic. E.g., what happens if someone uses another dGPU in a docking station or some other sort of PCIe bridge?

Such a dGPU is only relevant to vga_switcheroo if it can either drive the panel or be powered off by the platform. Does such a product exist?

I've never heard of one, and think that's because such a product doesn't make sense: A docking staton is not power-constrained, it's stationary and connected to a wall outlet, so there's no need to power the dGPU off when it's not in use.

And at a docking station you're usually connected to a larger monitor, so having the dGPU drive the laptop's smaller panel isn't necessary either. In the rare cases where there's no larger monitor, you just use the dGPU for render offloading, just as you would for contemporary ATPX laptops.

OTOH, dGPUs in Thunderbolt enclosures *do* exist and connecting them to an ATPX machine causes failure, as explained in the commit message.

Quoted 5 lines

I think on AMD platforms
at least we should be able to determine what devices are the switcheroo devices based on information in the ATIF and ATPX ACPI methods. In that case, we can be explicit in which devices we register with vga_switcheroo.

Is there public documentation on these methods somewhere?

Quoted 14 lines · Lukas Wunner

Quoted 12 lines · Alex Deucher

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I think on AMD platforms
at least we should be able to determine what devices are the switcheroo devices based on information in the ATIF and ATPX ACPI methods. In that case, we can be explicit in which devices we register with vga_switcheroo.

Quoted 1 line

Is there public documentation on these methods somewhere?

The ACPI interface is documented in drivers/gpu/drm/amd/include/amd_acpi.h while drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c contains some glue for ACPI and the amdgpu driver (similar code exists for radeon).

Quoted 10 lines · Alex Deucher

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I'm not necessarily opposed to this, but I'd prefer something more generic. E.g., what happens if someone uses another dGPU in a docking station or some other sort of PCIe bridge?

Such a dGPU is only relevant to vga_switcheroo if it can either drive the panel or be powered off by the platform. Does such a product exist?

I've never heard of one, and think that's because such a product doesn't make sense: A docking staton is not power-constrained, it's stationary and connected to a wall outlet, so there's no need to power the dGPU off when it's not in use.

And at a docking station you're usually connected to a larger monitor, so having the dGPU drive the laptop's smaller panel isn't necessary either. In the rare cases where there's no larger monitor, you just use the dGPU for render offloading, just as you would for contemporary ATPX laptops.

OTOH, dGPUs in Thunderbolt enclosures *do* exist and connecting them to an ATPX machine causes failure, as explained in the commit message.

Quoted 7 lines

I think on AMD platforms
at least we should be able to determine what devices are the switcheroo devices based on information in the ATIF and ATPX ACPI methods. In that case, we can be explicit in which devices we register with vga_switcheroo.

Is there public documentation on these methods somewhere?

Thanks,

Lukas

Quoted 20 lines · Lukas Wunner

Quoted 10 lines · Alex Deucher

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I'm not necessarily opposed to this, but I'd prefer something more generic. E.g., what happens if someone uses another dGPU in a docking station or some other sort of PCIe bridge?

Such a dGPU is only relevant to vga_switcheroo if it can either drive the panel or be powered off by the platform. Does such a product exist?

I've never heard of one, and think that's because such a product doesn't make sense: A docking staton is not power-constrained, it's stationary and connected to a wall outlet, so there's no need to power the dGPU off when it's not in use.

Quoted 7 lines

And at a docking station you're usually connected to a larger monitor, so having the dGPU drive the laptop's smaller panel isn't necessary either. In the rare cases where there's no larger monitor, you just use the dGPU for render offloading, just as you would for contemporary ATPX laptops.

OTOH, dGPUs in Thunderbolt enclosures *do* exist and connecting them to an ATPX machine causes failure, as explained in the commit message.

Whether you introduce additional dGPUs via thunderbolt or some proprietary interface or a pci bridge in a docking station the result is still the same. You end up with the potential scenario you described in this commit message that there may be confusion as to which GPU is controlled via ACPI power controls.

I talked to the windows team. They special case thunderbolt as well,

so the patch is probably fine.

For pcie ports in a docking station, I
suspect there may not actually be any docking stations supported by PX laptops where this could be an issue.

For non-thunderbolt detachable
graphics there is a new ATIF function to query the bus number of the supported device. I'll send a patch out for that in a bit.

Thinking about this more, long term we should probably only register with vga_switcheroo if we support display muxing which is a legacy feature these days. Most systems are mux-less so we just need to handle dgpu power control via runtime pm.

Quoted 29 lines · Alex Deucher

Quoted 20 lines · Lukas Wunner

Quoted 10 lines · Alex Deucher

Quoted 5 lines · Lukas Wunner

An external Thunderbolt GPU can neither drive the laptop's panel nor be powered off by the platform, so there's no point in registering it with vga_switcheroo. In fact, when the external GPU is runtime suspended, vga_switcheroo will cut power to the internal discrete GPU, resulting in a lockup.

I'm not necessarily opposed to this, but I'd prefer something more generic. E.g., what happens if someone uses another dGPU in a docking station or some other sort of PCIe bridge?

Such a dGPU is only relevant to vga_switcheroo if it can either drive the panel or be powered off by the platform. Does such a product exist?

I've never heard of one, and think that's because such a product doesn't make sense: A docking staton is not power-constrained, it's stationary and connected to a wall outlet, so there's no need to power the dGPU off when it's not in use.

Quoted 7 lines

And at a docking station you're usually connected to a larger monitor, so having the dGPU drive the laptop's smaller panel isn't necessary either. In the rare cases where there's no larger monitor, you just use the dGPU for render offloading, just as you would for contemporary ATPX laptops.

OTOH, dGPUs in Thunderbolt enclosures *do* exist and connecting them to an ATPX machine causes failure, as explained in the commit message.

Quoted 7 lines

Whether you introduce additional dGPUs via thunderbolt or some proprietary interface or a pci bridge in a docking station the result is still the same. You end up with the potential scenario you described in this commit message that there may be confusion as to which GPU is controlled via ACPI power controls.

I talked to the windows team. They special case thunderbolt as well,

Very interesting, thanks for reaching out to them. I've already heard that Windows 10 supports Thunderbolt eGPUs, but only with Thunderbolt 3. I think it's desirable that we achieve feature parity with them (without the unnecessary restriction to Thunderbolt 3). Older Windows versions as well as macOS apparently require all sorts of awful hacks for eGPUs.

Quoted 1 line

so the patch is probably fine.

Is that an ack or are there any remaining concerns?

Quoted 5 lines

For pcie ports in a docking station, I
suspect there may not actually be any docking stations supported by PX laptops where this could be an issue.

If/when such products do become available, they can probably be identified via specific ACPI methods or if all else fails, DMI quirks.

Quoted 3 lines

For non-thunderbolt detachable
graphics there is a new ATIF function to query the bus number of the supported device. I'll send a patch out for that in a bit.

Great, thanks.

Quoted 4 lines

Thinking about this more, long term we should probably only register with vga_switcheroo if we support display muxing which is a legacy feature these days. Most systems are mux-less so we just need to handle dgpu power control via runtime pm.

Right now registering with vga_switcheroo is still necessary even for muxless systems primarily because DRM drivers call vga_switcheroo_set_dynamic_switch() to pause the HDA driver and update the power state stored internally by vga_switcheroo.

I plan to address the former by reworking vga_switcheroo audio handling using functional device dependencies (a new PM mechanism that appeared in v4.10, see documentation in aad800403a87), and I think the latter will then become obsolete as well. I've got a concept in my head and am pumped to code it up, just a little time-constrained at the moment. :-)

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Fix Thunderbolt-related issues in apple-gmux and vga_switcheroo:

Patch [1/5] ("Recognize Thunderbolt devices") has already been subjected to a fair amount of scrutiny over at linux-pci@, I've submitted it 5 times total since May 2016. With luck it may be in ack-able shape now.

Patch [2/5] amends apple-gmux to handle combined DP/Thunderbolt ports properly on newer MacBook Pros.

Patches [3/5] to [5/5] avoid registering external Thunderbolt GPUs with
vga_switcheroo: Dave Airlie designed vga_switcheroo to register GPUs unconditionally. So if a desktop box has multiple GPUs, vga_switcheroo will see more than one discrete GPU but that's not a problem because on desktop boxes no handler is registered and thus vga_switcheroo_enable() is never called. Hybrid graphics laptops on the other hand do register a handler, but are assumed to never register more than one discrete GPU. However once a Thunderbolt eGPU is attached to a hybrid graphics laptop, that assumption is no longer true and things go south when vga_switcheroo runtime suspends the external discrete GPU and then calls the handler to cut power to the internal discrete GPU. The driver for the internal GPU will sit there puzzled and typically cause a lockup.

This series is just a first step towards proper handling of eGPUs, another will be support for surprise removal: DRM drivers need to cease MMIO and PCI config space access once a device is gone to avoid delaying device teardown or accessing a newly attached replacement device. Also, MMIO reads to removed devices return "all ones", which results in an infinite loop e.g. in nouveau's nvkm_nsec().

The question is how to recognize device removal. One common method is to read the vendor register with pci_device_is_present(), but this reports a false positive if the device is present but in D3cold. A better method is to let the PCIe hotplug driver recognize and cache device removal. Keith Busch has developed patches which do exactly that, they're now at v6 and fully reviewed by Christoph Hellwig but alas were not included in the 4.11 PCI pull for some reason:
http://www.spinics.net/lists/linux-pci/msg58123.html

I've pushed the present series to GitHub in case anyone prefers reviewing it in a GUI:
https://github.com/l1k/linux/commits/thunderbolt_gpu_v1

Thanks,

Lukas

Lukas Wunner (5):
PCI: Recognize Thunderbolt devices
apple-gmux: Don't switch external DP port on 2011+ MacBook Pros drm/nouveau: Don't register Thunderbolt eGPU with vga_switcheroo drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo drm/amdgpu: Don't register Thunderbolt eGPU with vga_switcheroo

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  7 +++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  3 ++-
 drivers/gpu/drm/nouveau/nouveau_vga.c      | 10 +++++++++-
 drivers/gpu/drm/radeon/radeon_device.c     |  7 +++++--
 drivers/gpu/drm/radeon/radeon_kms.c        |  3 ++-
 drivers/pci/pci.h                          |  2 ++
 drivers/pci/probe.c                        | 21 ++++++++++++++++++++
 drivers/platform/x86/apple-gmux.c          | 31 +++++++++++++++++++++++++++++-
 include/linux/pci.h                        | 23 ++++++++++++++++++++++
 9 files changed, 99 insertions(+), 8 deletions(-)

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 21 lines · Lukas Wunner

Fix Thunderbolt-related issues in apple-gmux and vga_switcheroo:

Patch [1/5] ("Recognize Thunderbolt devices") has already been subjected to a fair amount of scrutiny over at linux-pci@, I've submitted it 5 times total since May 2016. With luck it may be in ack-able shape now.

Patch [2/5] amends apple-gmux to handle combined DP/Thunderbolt ports properly on newer MacBook Pros.

Patches [3/5] to [5/5] avoid registering external Thunderbolt GPUs with
vga_switcheroo: Dave Airlie designed vga_switcheroo to register GPUs unconditionally. So if a desktop box has multiple GPUs, vga_switcheroo will see more than one discrete GPU but that's not a problem because on desktop boxes no handler is registered and thus vga_switcheroo_enable() is never called. Hybrid graphics laptops on the other hand do register a handler, but are assumed to never register more than one discrete GPU. However once a Thunderbolt eGPU is attached to a hybrid graphics laptop, that assumption is no longer true and things go south when vga_switcheroo runtime suspends the external discrete GPU and then calls the handler to cut power to the internal discrete GPU. The driver for the internal GPU will sit there puzzled and typically cause a lockup.

Quoted 3 lines

I've pushed the present series to GitHub in case anyone prefers reviewing it in a GUI:
https://github.com/l1k/linux/commits/thunderbolt_gpu_v1

For merging, should I smash this all into drm-misc? The only thing outside is the apple-gmux driver ...

Quoted 22 lines · Daniel Vetter

Quoted 21 lines · Lukas Wunner

Fix Thunderbolt-related issues in apple-gmux and vga_switcheroo:

Patch [1/5] ("Recognize Thunderbolt devices") has already been subjected to a fair amount of scrutiny over at linux-pci@, I've submitted it 5 times total since May 2016. With luck it may be in ack-able shape now.

Patch [2/5] amends apple-gmux to handle combined DP/Thunderbolt ports properly on newer MacBook Pros.

Patches [3/5] to [5/5] avoid registering external Thunderbolt GPUs with
vga_switcheroo: Dave Airlie designed vga_switcheroo to register GPUs unconditionally. So if a desktop box has multiple GPUs, vga_switcheroo will see more than one discrete GPU but that's not a problem because on desktop boxes no handler is registered and thus vga_switcheroo_enable() is never called. Hybrid graphics laptops on the other hand do register a handler, but are assumed to never register more than one discrete GPU. However once a Thunderbolt eGPU is attached to a hybrid graphics laptop, that assumption is no longer true and things go south when vga_switcheroo runtime suspends the external discrete GPU and then calls the handler to cut power to the internal discrete GPU. The driver for the internal GPU will sit there puzzled and typically cause a lockup.

[snip]

Quoted 6 lines

Quoted 3 lines

I've pushed the present series to GitHub in case anyone prefers reviewing it in a GUI:
https://github.com/l1k/linux/commits/thunderbolt_gpu_v1

For merging, should I smash this all into drm-misc? The only thing outside is the apple-gmux driver ...

Merging through drm-misc would be lovely. However I've prepared a v2 of patch [1/5] to address Bjorn's comments (amended the commit message and a code comment). I'll respin the series this evening and include the acks I've collected so far.

@Darren & Andy:
Please ack patch [5/5] of this series, barring any objections.

I'll move the apple-gmux patch to the end of the series, so merging that one can be postponed until Darren and Andy find the time to look at it.