[PATCH 0/5] Thunderbolt GPU fixes

[PATCH 0/5] Thunderbolt GPU fixes

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

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

-- 
2.11.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 0/5] Thunderbolt GPU fixes

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

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

-- 
2.11.0

[PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

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

Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_device.c | 7 +++++--
 drivers/gpu/drm/radeon/radeon_kms.c    | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 4b0c388be3f5..27be17f0b227 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -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);
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 56f35c06742c..e95ceec1c97a 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Alex Deucher]

On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> 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.

Alex

>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/radeon/radeon_device.c | 7 +++++--
>  drivers/gpu/drm/radeon/radeon_kms.c    | 3 ++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 4b0c388be3f5..27be17f0b227 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -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);
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 56f35c06742c..e95ceec1c97a 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -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
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Lukas Wunner]

On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
> On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > 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.


> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Peter Wu]

On Wed, Mar 08, 2017 at 06:01:54AM +0100, Lukas Wunner wrote:
> On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
> > On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > 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.
> 
> 
> > 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?

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).
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Lukas Wunner]

On Wed, Mar 08, 2017 at 11:46:33AM +0100, Peter Wu wrote:
> On Wed, Mar 08, 2017 at 06:01:54AM +0100, Lukas Wunner wrote:
> > On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
> > > On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > 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.
> > 
> > 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).

Ah, thanks Peter.

Unfortunately this method will not work on Macs.  So I guess on those
we're again dependent on deducing whether a dGPU is internal or external
by looking at the PCI hierarchy.

However on non-Macs it seems that ATIF_FUNCTION_GET_GRAPHICS_DEVICE_TYPES
should return the type for each built-in GPU.

@Alex:
How reliable is this, e.g. is it possible that vendors may have forgotten
to set these bits in the BIOS?  If we depend on ATIF to determine the
type of a dGPU and the information returned is incorrect, we risk not
registering a device when we actually should, thus causing a regression.

Also, could you explain which of these types should be registered with
vga_switcheroo and which shouldn't?  Likewise, which of these can be
powered down by the platform and should thus use the vga_switcheroo
power domain?

	ATIF_PX_REMOVABLE_GRAPHICS_DEVICE
	ATIF_XGP_PORT
	ATIF_VGA_ENABLED_GRAPHICS_DEVICE
	ATIF_XGP_PORT_IN_DOCK

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Alex Deucher]

On Wed, Mar 8, 2017 at 12:01 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
>> On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> > 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.

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.

Alex

>
>
>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Lukas Wunner]

On Wed, Mar 08, 2017 at 03:34:47PM -0500, Alex Deucher wrote:
> On Wed, Mar 8, 2017 at 12:01 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
> >> On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> > 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.
> 
> 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.


> so the patch is probably fine.

Is that an ack or are there any remaining concerns?


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


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


> 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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/radeon: Don't register Thunderbolt eGPU with vga_switcheroo

[Alex Deucher]

On Thu, Mar 9, 2017 at 5:55 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Wed, Mar 08, 2017 at 03:34:47PM -0500, Alex Deucher wrote:
>> On Wed, Mar 8, 2017 at 12:01 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > On Tue, Mar 07, 2017 at 03:30:30PM -0500, Alex Deucher wrote:
>> >> On Fri, Feb 24, 2017 at 2:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> >> > 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.
>>
>> 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.
>
>
>> so the patch is probably fine.
>
> Is that an ack or are there any remaining concerns?

Series is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
>
>> 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.
>
>
>> 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.
>
>
>> 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 for looking into it.

Alex

>
> Thanks,
>
> Lukas
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/nouveau: Don't register Thunderbolt eGPU with vga_switcheroo

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

Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index eef22c6b9665..c2a7fd606c2e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -95,6 +95,10 @@ nouveau_vga_init(struct nouveau_drm *drm)
 
 	vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
 
+	/* don't register Thunderbolt eGPU with vga_switcheroo */
+	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()))
@@ -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()))
@@ -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);
 }
 
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/nouveau: Don't register Thunderbolt eGPU with vga_switcheroo

[Bjorn Helgaas]

On Fri, Feb 24, 2017 at 1:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> 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.

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.

> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index eef22c6b9665..c2a7fd606c2e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -95,6 +95,10 @@ nouveau_vga_init(struct nouveau_drm *drm)
>
>         vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
>
> +       /* 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?

> +
>         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()))
> @@ -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:

-       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?"

>  }
>
>
> --
> 2.11.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/nouveau: Don't register Thunderbolt eGPU with vga_switcheroo

[Lukas Wunner]

On Fri, Feb 24, 2017 at 02:59:44PM -0600, Bjorn Helgaas wrote:
> On Fri, Feb 24, 2017 at 1:19 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > 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.
> 
> 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.

vga_switcheroo_init_domain_pm_ops() is currently called both for an
internal discrete GPU as well as an external one attached with Thunderbolt.

That function overrides the ->runtime_suspend hook to cut power to the
internal discrete GPU after runtime suspending whichever GPU the
->runtime_suspend hook was called for.  So the external GPU runtime
suspends, the hook cuts power to the internal GPU, boom.

The function should only be called for the internal GPU, which is the
objective of this patch.


> > --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> > @@ -95,6 +95,10 @@ nouveau_vga_init(struct nouveau_drm *drm)
> >
> >         vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
> >
> > +       /* 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?

It's only needed for a subset of the drivers, in particular not for i915.

The preferred approach in the kernel seems to be to avoid midlayers which
bundle stuff that is not applicable to every driver interfacing with it,
and instead put the functionality in library functions which drivers can
opt in to if necessary.  In this case that library function would be
pci_is_thunderbolt_attached().

A link list on midlayers vs. libraries is here:
http://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html


> > @@ -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()))
> > @@ -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:
> 
> -       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?"

Functionally it's the same, it only looks differently because the
DRM drivers aren't structured identically.  In particular, radeon
and amdgpu call vga_switcheroo_init_domain_pm_ops() only if the flag
RADEON_IS_PX / AMD_IS_PX (is PowerXpress) has been set, so a call
to pci_is_thunderbolt_attached() is added when setting that flag,
whereas nouveau doesn't have that indirection.

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] apple-gmux: Don't switch external DP port on 2011+ MacBook Pros

[Lukas Wunner]

On MacBook Pros introduced 2011 and onward, external DP ports are
combined DP/Thunderbolt ports that are no longer fully switchable
between GPUs, they can only be driven by the discrete GPU.

More specifically, the Main Link pins (which transport the actual video
and audio streams) are soldered to the discrete GPU, whereas the AUX
Channel pins are switchable. Because the integrated GPU is missing the
Main Link, external displays appear to it as phantoms which fail to
link-train.

Force the AUX channel to the discrete GPU on these models to avoid any
confusion. Document the switching policy implemented by this commit.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index a66be137324c..623d322447a2 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -60,6 +60,7 @@ struct apple_gmux_data {
 	/* switcheroo data */
 	acpi_handle dhandle;
 	int gpe;
+	bool external_switchable;
 	enum vga_switcheroo_client_id switch_state_display;
 	enum vga_switcheroo_client_id switch_state_ddc;
 	enum vga_switcheroo_client_id switch_state_external;
@@ -358,6 +359,19 @@ static const struct backlight_ops gmux_bl_ops = {
  * ports while the discrete GPU is asleep, but currently we do not make use
  * of this feature.
  *
+ * Our switching policy for the external port is that on those generations
+ * which are able to switch it fully, the port is switched together with the
+ * panel when IGD / DIS commands are issued to vga_switcheroo. It is thus
+ * possible to drive e.g. a beamer on battery power with the integrated GPU.
+ * The user may manually switch to the discrete GPU if more performance is
+ * needed.
+ *
+ * On all newer generations, the external port can only be driven by the
+ * discrete GPU. If a display is plugged in while the panel is switched to
+ * the integrated GPU, *both* GPUs will be in use for maximum performance.
+ * To decrease power consumption, the user may manually switch to the
+ * discrete GPU, thereby suspending the integrated GPU.
+ *
  * gmux' initial switch state on bootup is user configurable via the EFI
  * variable ``gpu-power-prefs-fa4ce28d-b62f-4c99-9cc3-6815686e30f9`` (5th byte,
  * 1 = IGD, 0 = DIS). Based on this setting, the EFI firmware tells gmux to
@@ -414,7 +428,8 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
 	apple_gmux_data->switch_state_ddc = id;
 	apple_gmux_data->switch_state_display = id;
-	apple_gmux_data->switch_state_external = id;
+	if (apple_gmux_data->external_switchable)
+		apple_gmux_data->switch_state_external = id;
 
 	gmux_write_switch_state(apple_gmux_data);
 
@@ -601,6 +616,11 @@ static struct pci_dev *gmux_get_io_pdev(void)
 	return NULL;
 }
 
+static int is_thunderbolt(struct device *dev, void *data)
+{
+	return to_pci_dev(dev)->is_thunderbolt;
+}
+
 static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 {
 	struct apple_gmux_data *gmux_data;
@@ -755,6 +775,15 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		gmux_data->gpe = -1;
 	}
 
+	/*
+	 * If Thunderbolt is present, the external DP port is not fully
+	 * switchable. Force its AUX channel to the discrete GPU.
+	 */
+	gmux_data->external_switchable =
+		!bus_for_each_dev(&pci_bus_type, NULL, NULL, is_thunderbolt);
+	if (!gmux_data->external_switchable)
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
+
 	apple_gmux_data = gmux_data;
 	init_completion(&gmux_data->powerchange_done);
 	gmux_enable_interrupts(gmux_data);
-- 
2.11.0

[PATCH 5/5] drm/amdgpu: Don't register Thunderbolt eGPU with vga_switcheroo

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

Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 944ba0d3874a..99d73504e56d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1759,7 +1759,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		runtime = true;
 	if (amdgpu_device_is_px(ddev))
 		runtime = true;
-	vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, runtime);
+	if (!pci_is_thunderbolt_attached(adev->pdev))
+		vga_switcheroo_register_client(adev->pdev,
+					       &amdgpu_switcheroo_ops, runtime);
 	if (runtime)
 		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
 
@@ -1922,7 +1924,8 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
 	amdgpu_atombios_fini(adev);
 	kfree(adev->bios);
 	adev->bios = NULL;
-	vga_switcheroo_unregister_client(adev->pdev);
+	if (!pci_is_thunderbolt_attached(adev->pdev))
+		vga_switcheroo_unregister_client(adev->pdev);
 	if (adev->flags & AMD_IS_PX)
 		vga_switcheroo_fini_domain_pm_ops(adev->dev);
 	vga_client_register(adev->pdev, NULL, NULL, NULL);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 61d94c745672..2f3b236721c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -103,7 +103,8 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
 	    amdgpu_has_atpx() &&
 	    (amdgpu_is_atpx_hybrid() ||
 	     amdgpu_has_atpx_dgpu_power_cntl()) &&
-	    ((flags & AMD_IS_APU) == 0))
+	    ((flags & AMD_IS_APU) == 0) &&
+	    !pci_is_thunderbolt_attached(dev->pdev))
 		flags |= AMD_IS_PX;
 
 	/* amdgpu_device_init should report only fatal error
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/5] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

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(+)

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>
 
-- 
2.11.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 1/5] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

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(+)

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>
 
-- 
2.11.0

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Bjorn Helgaas]

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
> 

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Bjorn Helgaas]

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
> 

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

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

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Bjorn Helgaas]

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>

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/5] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

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

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Daniel Vetter]

On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
> 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

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


> 
> 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(-)
> 
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Daniel Vetter]

On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
> 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

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


> 
> 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(-)
> 
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Lukas Wunner]

On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
> > 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]
> > 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.

Thanks!

Lukas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Lukas Wunner]

On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
> > 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]
> > 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.

Thanks!

Lukas

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Andy Shevchenko]

On Fri, Mar 10, 2017 at 2:07 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
>> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
>> > 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]
>> > 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.

Yeah, please resend. It's probably buried in the pile of deleted mails.
Though I have still patchwork ticket.

Okay, I have looked at it and I'm fine with the code. Hope this
doesn't break anything.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Thunderbolt GPU fixes

[Andy Shevchenko]

On Fri, Mar 10, 2017 at 2:07 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
>> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
>> > 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]
>> > 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.

Yeah, please resend. It's probably buried in the pile of deleted mails.
Though I have still patchwork ticket.

Okay, I have looked at it and I'm fine with the code. Hope this
doesn't break anything.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko