| Message ID | 20180412104239.25584-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Thu, 12 Apr 2018 12:42:39 +0200, Kai-Heng Feng wrote: > > Some Dell platforms (Preicsion 7510/7710/7520/7720) have a BIOS option > "Switchable Graphics" (SG). > > When SG is enabled, we have: > 00:02.0 VGA compatible controller: Intel Corporation Device 591b (rev 04) > 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) > 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Polaris10] > 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580] > > The Intel Audio outputs all the sound, including HDMI audio. The audio > controller comes with AMD graphics doesn't get used. > > When SG is disabled, we have: > 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) > 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Polaris10] > 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580] > > Now it's a typical discrete-only system. HDMI audio comes from AMD audio > controller, others from Intel audio controller. > > When SG is enabled, the unused AMD audio controller still exposes its > sysfs, so userspace still opens the control file and stream. If > userspace tries to output sound through the stream, it hangs when > runtime suspend kicks in: > [ 12.796265] snd_hda_intel 0000:01:00.1: Disabling via vga_switcheroo > [ 12.796367] snd_hda_intel 0000:01:00.1: Cannot lock devices! > > Since the discrete audio controller isn't useful when SG enabled, we > should just disable the device. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Adding Lukas to Cc. I thought we manage this better now with runtime PM by Lukas's recent patchset? thanks, Takashi > --- > v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead > of error code. > Use DMI_DEV_TYPE_OEM_STRING to match Dell System. > > v2: Mario suggested to squash the HDA part into the same series. > > sound/pci/hda/hda_intel.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 7720e3102bcc..d9310616d5ca 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -49,6 +49,8 @@ > #include <linux/clocksource.h> > #include <linux/time.h> > #include <linux/completion.h> > +#include <linux/dell-common.h> > +#include <linux/dmi.h> > > #ifdef CONFIG_X86 > /* for snoop control */ > @@ -1629,6 +1631,38 @@ static void check_msi(struct azx *chip) > } > } > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP) > +static bool check_dell_switchable_gfx(struct pci_dev *pdev) > +{ > + bool (*dell_switchable_gfx_is_enabled_func)(void); > + bool enabled; > + > + /* Only need to check for Dell laptops and AIOs */ > + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || > + !(dmi_match(DMI_CHASSIS_TYPE, "10") || > + dmi_match(DMI_CHASSIS_TYPE, "13")) || > + !(pdev->vendor == PCI_VENDOR_ID_ATI || > + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) > + return false; > + > + dell_switchable_gfx_is_enabled_func = > + symbol_request(dell_switchable_gfx_is_enabled); > + if (!dell_switchable_gfx_is_enabled_func) > + return false; > + > + enabled = dell_switchable_gfx_is_enabled_func(); > + > + symbol_put(dell_switchable_gfx_is_enabled); > + > + return enabled; > +} > +#else > +static bool check_dell_switchable_gfx(struct pci_dev *pdev) > +{ > + return false; > +} > +#endif > + > /* check the snoop mode availability */ > static void azx_check_snoop_available(struct azx *chip) > { > @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, > if (err < 0) > return err; > > + if (check_dell_switchable_gfx(pci)) { > + pci_disable_device(pci); > + return -ENODEV; > + } > + > hda = kzalloc(sizeof(*hda), GFP_KERNEL); > if (!hda) { > pci_disable_device(pci); > -- > 2.17.0 > >
On Thursday 12 April 2018 12:50:02 Takashi Iwai wrote: > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP) > > +static bool check_dell_switchable_gfx(struct pci_dev *pdev) > > +{ > > + bool (*dell_switchable_gfx_is_enabled_func)(void); > > + bool enabled; > > + > > + /* Only need to check for Dell laptops and AIOs */ > > + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || > > + !(dmi_match(DMI_CHASSIS_TYPE, "10") || > > + dmi_match(DMI_CHASSIS_TYPE, "13")) || > > + !(pdev->vendor == PCI_VENDOR_ID_ATI || > > + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) > > + return false; ... > > @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, > > if (err < 0) > > return err; > > > > + if (check_dell_switchable_gfx(pci)) { > > + pci_disable_device(pci); Hi! Now looking at it again... This code disables all ATI and NVIDIA sound cards available in any Dell System (laptop or AIO) if system says that SG is enabled, right? It means that also any external ATI or NVIDIA PCI card with audio device connected to Thunderbolt (e.g. via PCI <--> TB bridge) is always unconditionally disabled too? > > + return -ENODEV; > > + }
at 6:50 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Thu, 12 Apr 2018 12:42:39 +0200, > Kai-Heng Feng wrote: >> Some Dell platforms (Preicsion 7510/7710/7520/7720) have a BIOS option >> "Switchable Graphics" (SG). >> >> When SG is enabled, we have: >> 00:02.0 VGA compatible controller: Intel Corporation Device 591b (rev 04) >> 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) >> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. >> [AMD/ATI] Ellesmere [Polaris10] >> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere >> [Radeon RX 580] >> >> The Intel Audio outputs all the sound, including HDMI audio. The audio >> controller comes with AMD graphics doesn't get used. >> >> When SG is disabled, we have: >> 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) >> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. >> [AMD/ATI] Ellesmere [Polaris10] >> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere >> [Radeon RX 580] >> >> Now it's a typical discrete-only system. HDMI audio comes from AMD audio >> controller, others from Intel audio controller. >> >> When SG is enabled, the unused AMD audio controller still exposes its >> sysfs, so userspace still opens the control file and stream. If >> userspace tries to output sound through the stream, it hangs when >> runtime suspend kicks in: >> [ 12.796265] snd_hda_intel 0000:01:00.1: Disabling via vga_switcheroo >> [ 12.796367] snd_hda_intel 0000:01:00.1: Cannot lock devices! >> >> Since the discrete audio controller isn't useful when SG enabled, we >> should just disable the device. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Adding Lukas to Cc. > > I thought we manage this better now with runtime PM by Lukas's recent > patchset? > Yes, that's true. I'll update commit log for next iteration. Nevertheless, the unusable control file and stream still get exposed via sysfs. We should disable them when SG is enabled. Kai-Heng > > thanks, > > Takashi > >> --- >> v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead >> of error code. >> Use DMI_DEV_TYPE_OEM_STRING to match Dell System. >> >> v2: Mario suggested to squash the HDA part into the same series. >> >> sound/pci/hda/hda_intel.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >> index 7720e3102bcc..d9310616d5ca 100644 >> --- a/sound/pci/hda/hda_intel.c >> +++ b/sound/pci/hda/hda_intel.c >> @@ -49,6 +49,8 @@ >> #include <linux/clocksource.h> >> #include <linux/time.h> >> #include <linux/completion.h> >> +#include <linux/dell-common.h> >> +#include <linux/dmi.h> >> >> #ifdef CONFIG_X86 >> /* for snoop control */ >> @@ -1629,6 +1631,38 @@ static void check_msi(struct azx *chip) >> } >> } >> >> +#if IS_ENABLED(CONFIG_DELL_LAPTOP) >> +static bool check_dell_switchable_gfx(struct pci_dev *pdev) >> +{ >> + bool (*dell_switchable_gfx_is_enabled_func)(void); >> + bool enabled; >> + >> + /* Only need to check for Dell laptops and AIOs */ >> + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || >> + !(dmi_match(DMI_CHASSIS_TYPE, "10") || >> + dmi_match(DMI_CHASSIS_TYPE, "13")) || >> + !(pdev->vendor == PCI_VENDOR_ID_ATI || >> + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) >> + return false; >> + >> + dell_switchable_gfx_is_enabled_func = >> + symbol_request(dell_switchable_gfx_is_enabled); >> + if (!dell_switchable_gfx_is_enabled_func) >> + return false; >> + >> + enabled = dell_switchable_gfx_is_enabled_func(); >> + >> + symbol_put(dell_switchable_gfx_is_enabled); >> + >> + return enabled; >> +} >> +#else >> +static bool check_dell_switchable_gfx(struct pci_dev *pdev) >> +{ >> + return false; >> +} >> +#endif >> + >> /* check the snoop mode availability */ >> static void azx_check_snoop_available(struct azx *chip) >> { >> @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, >> struct pci_dev *pci, >> if (err < 0) >> return err; >> >> + if (check_dell_switchable_gfx(pci)) { >> + pci_disable_device(pci); >> + return -ENODEV; >> + } >> + >> hda = kzalloc(sizeof(*hda), GFP_KERNEL); >> if (!hda) { >> pci_disable_device(pci); >> -- >> 2.17.0
at 6:59 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Thursday 12 April 2018 12:50:02 Takashi Iwai wrote: >>> +#if IS_ENABLED(CONFIG_DELL_LAPTOP) >>> +static bool check_dell_switchable_gfx(struct pci_dev *pdev) >>> +{ >>> + bool (*dell_switchable_gfx_is_enabled_func)(void); >>> + bool enabled; >>> + >>> + /* Only need to check for Dell laptops and AIOs */ >>> + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || >>> + !(dmi_match(DMI_CHASSIS_TYPE, "10") || >>> + dmi_match(DMI_CHASSIS_TYPE, "13")) || >>> + !(pdev->vendor == PCI_VENDOR_ID_ATI || >>> + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) >>> + return false; > ... >>> @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, >>> struct pci_dev *pci, >>> if (err < 0) >>> return err; >>> >>> + if (check_dell_switchable_gfx(pci)) { >>> + pci_disable_device(pci); > > Hi! > > Now looking at it again... This code disables all ATI and NVIDIA sound > cards available in any Dell System (laptop or AIO) if system says that > SG is enabled, right? Yes. > > It means that also any external ATI or NVIDIA PCI card with audio device > connected to Thunderbolt (e.g. via PCI <--> TB bridge) is always > unconditionally disabled too? I never thought of this case, thanks for bringing this up. Do you have any suggestion to check if it connects to the system via Thunderbolt? Kai-Heng > >>> + return -ENODEV; >>> + } > > -- > Pali Rohár > pali.rohar@gmail.com
On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > at 6:59 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > > On Thursday 12 April 2018 12:50:02 Takashi Iwai wrote: > > > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP) > > > > +static bool check_dell_switchable_gfx(struct pci_dev *pdev) > > > > +{ > > > > + bool (*dell_switchable_gfx_is_enabled_func)(void); > > > > + bool enabled; > > > > + > > > > + /* Only need to check for Dell laptops and AIOs */ > > > > + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || > > > > + !(dmi_match(DMI_CHASSIS_TYPE, "10") || > > > > + dmi_match(DMI_CHASSIS_TYPE, "13")) || > > > > + !(pdev->vendor == PCI_VENDOR_ID_ATI || > > > > + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) > > > > + return false; > > ... > > > > @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card > > > > *card, struct pci_dev *pci, > > > > if (err < 0) > > > > return err; > > > > > > > > + if (check_dell_switchable_gfx(pci)) { > > > > + pci_disable_device(pci); > > > > Hi! > > > > Now looking at it again... This code disables all ATI and NVIDIA sound > > cards available in any Dell System (laptop or AIO) if system says that > > SG is enabled, right? > > Yes. > > > > > It means that also any external ATI or NVIDIA PCI card with audio device > > connected to Thunderbolt (e.g. via PCI <--> TB bridge) is always > > unconditionally disabled too? > > I never thought of this case, thanks for bringing this up. > Do you have any suggestion to check if it connects to the system via > Thunderbolt? Is there any kind of indicator for a PCI device if it is a removable device? Only disabling those PCI devices which are wholly integrated with the platform would be ideal. Failing that, is there an indicator in the PCI configuration which will distinguish such devices? Are the integrated devices using specific lanes, are the external devices behind a switch? etc. And can we do this in a generic way for all relevant platforms.
On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > >>@@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, > > >>struct pci_dev *pci, > > >> if (err < 0) > > >> return err; > > >> > > >>+ if (check_dell_switchable_gfx(pci)) { > > >>+ pci_disable_device(pci); > > > > Now looking at it again... This code disables all ATI and NVIDIA sound > > cards available in any Dell System (laptop or AIO) if system says that > > SG is enabled, right? > > > > It means that also any external ATI or NVIDIA PCI card with audio device > > connected to Thunderbolt (e.g. via PCI <--> TB bridge) is always > > unconditionally disabled too? > > I never thought of this case, thanks for bringing this up. > Do you have any suggestion to check if it connects to the system via > Thunderbolt? Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, like this: if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > >>>+ /* Only need to check for Dell laptops and AIOs */ > >>>+ if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || > >>>+ !(dmi_match(DMI_CHASSIS_TYPE, "10") || > >>>+ dmi_match(DMI_CHASSIS_TYPE, "13")) || > >>>+ !(pdev->vendor == PCI_VENDOR_ID_ATI || > >>>+ pdev->vendor == PCI_VENDOR_ID_NVIDIA)) > >>>+ return false; It sure would be nice if someone could add macros for the chassis type to include/linux/dmi.h so that we don't have to use these magic numbers everywhere: $ git grep -l DMI_CHASSIS_TYPE drivers/firmware/dmi-id.c drivers/firmware/dmi_scan.c drivers/input/keyboard/atkbd.c drivers/input/serio/i8042-x86ia64io.h drivers/platform/x86/asus-wmi.c drivers/platform/x86/dell-laptop.c drivers/platform/x86/samsung-laptop.c include/linux/mod_devicetable.h scripts/mod/file2alias.c Thanks, Lukas
On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > >>@@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, > > > >>struct pci_dev *pci, > > > >> if (err < 0) > > > >> return err; > > > >> > > > >>+ if (check_dell_switchable_gfx(pci)) { > > > >>+ pci_disable_device(pci); > > > > > > Now looking at it again... This code disables all ATI and NVIDIA sound > > > cards available in any Dell System (laptop or AIO) if system says that > > > SG is enabled, right? > > > > > > It means that also any external ATI or NVIDIA PCI card with audio device > > > connected to Thunderbolt (e.g. via PCI <--> TB bridge) is always > > > unconditionally disabled too? > > > > I never thought of this case, thanks for bringing this up. > > Do you have any suggestion to check if it connects to the system via > > Thunderbolt? > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > like this: > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) And what about PCI-e device attached to ExpressCard slot? > > >>>+ /* Only need to check for Dell laptops and AIOs */ > > >>>+ if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || > > >>>+ !(dmi_match(DMI_CHASSIS_TYPE, "10") || > > >>>+ dmi_match(DMI_CHASSIS_TYPE, "13")) || > > >>>+ !(pdev->vendor == PCI_VENDOR_ID_ATI || > > >>>+ pdev->vendor == PCI_VENDOR_ID_NVIDIA)) > > >>>+ return false; > > It sure would be nice if someone could add macros for the chassis type > to include/linux/dmi.h so that we don't have to use these magic numbers > everywhere: > > $ git grep -l DMI_CHASSIS_TYPE > drivers/firmware/dmi-id.c > drivers/firmware/dmi_scan.c > drivers/input/keyboard/atkbd.c > drivers/input/serio/i8042-x86ia64io.h > drivers/platform/x86/asus-wmi.c > drivers/platform/x86/dell-laptop.c > drivers/platform/x86/samsung-laptop.c > include/linux/mod_devicetable.h > scripts/mod/file2alias.c > > Thanks, > > Lukas
On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > Do you have any suggestion to check if it connects to the system via > > > Thunderbolt? > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > like this: > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > And what about PCI-e device attached to ExpressCard slot? I don't know of a bullet-proof way to recognize those. In theory one could check if the PCIe port above the GPU is a non-hotplug root port, but I think there are machines with hotplug capable root ports with GPUs below them that aren't actually removable. However I think ExpressCard-attached GPUs were rare, much less ones with integrated HDA controller, so in reality that's probably a non-issue. Thanks, Lukas
On Thu, Apr 12, 2018 at 10:12:49PM +0800, Kai-Heng Feng wrote: > at 6:50 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 12 Apr 2018 12:42:39 +0200, Kai-Heng Feng wrote: > > > When SG is enabled, the unused AMD audio controller still exposes its > > > sysfs, so userspace still opens the control file and stream. If > > > userspace tries to output sound through the stream, it hangs when > > > runtime suspend kicks in: > > > [ 12.796265] snd_hda_intel 0000:01:00.1: Disabling via vga_switcheroo > > > [ 12.796367] snd_hda_intel 0000:01:00.1: Cannot lock devices! > > > > > > Since the discrete audio controller isn't useful when SG enabled, we > > > should just disable the device. > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > I thought we manage this better now with runtime PM by Lukas's recent > > patchset? > > Yes, that's true. I'll update commit log for next iteration. > > Nevertheless, the unusable control file and stream still get exposed via > sysfs. > We should disable them when SG is enabled. Right, the hang on runtime suspend as mentioned in the commit message should be gone in 4.17. The purpose of this patch is thus to prevent the user from seeing or opening the HDA controller on the discrete GPU. If SG is enabled, external DP/HDMI displays are muxed to the Intel GPU, hence the HDA controller on the discrete GPU cannot communicate with the attached displays. Thanks, Lukas
On Saturday 14 April 2018 13:17:11 Lukas Wunner wrote: > On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > > Do you have any suggestion to check if it connects to the system via > > > > Thunderbolt? > > > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > > like this: > > > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > > > And what about PCI-e device attached to ExpressCard slot? > > I don't know of a bullet-proof way to recognize those. In theory > one could check if the PCIe port above the GPU is a non-hotplug > root port, but I think there are machines with hotplug capable > root ports with GPUs below them that aren't actually removable. > > However I think ExpressCard-attached GPUs were rare, much less ones > with integrated HDA controller, so in reality that's probably a > non-issue. Hm... maybe another idea: Is it possible to detect which audio pci device belongs to graphics card via vga_switcheroo? Currently, looking at output it is same PCI device as graphic card, just different PCI function.
On Sun, Apr 15, 2018 at 07:17:46PM +0200, Pali Rohár wrote: > On Saturday 14 April 2018 13:17:11 Lukas Wunner wrote: > > On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > > > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > > > Do you have any suggestion to check if it connects to the system via > > > > > Thunderbolt? > > > > > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > > > like this: > > > > > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > > > > > And what about PCI-e device attached to ExpressCard slot? > > > > I don't know of a bullet-proof way to recognize those. In theory > > one could check if the PCIe port above the GPU is a non-hotplug > > root port, but I think there are machines with hotplug capable > > root ports with GPUs below them that aren't actually removable. > > > > However I think ExpressCard-attached GPUs were rare, much less ones > > with integrated HDA controller, so in reality that's probably a > > non-issue. > > Hm... maybe another idea: Is it possible to detect which audio pci > device belongs to graphics card via vga_switcheroo? Currently, looking > at output it is same PCI device as graphic card, just different PCI > function. No, the DRM drivers don't filter ExpressCard-attached GPUs when registering with vga_switcheroo. They do filter Thunderbolt- attached GPUs. The ExpressCard 2.0 spec defines some ACPI stuff that *might* be used to recognize root ports that are ExpressCard slots, but I'm not sure how reliable that is. I don't have such a machine and have no experience with it. This is from the MacBookPro8,3 DSDT: Device (RP04) { Name (_ADR, 0x001C0003) OperationRegion (A1E0, PCI_Config, 0x19, 0x01) Field (A1E0, ByteAcc, NoLock, Preserve) { SECB, 8 } Device (EXCD) { Name (_ADR, 0x00) Name (_SUN, 0x01) Method (_RMV, 0, NotSerialized) { Return (0x01) } Name (_EJD, "\\_SB.PCI0.EHC2.HUBN.PRTN.PRT4") } ... } Thanks, Lukas
On Sunday 15 April 2018 21:05:23 Lukas Wunner wrote: > On Sun, Apr 15, 2018 at 07:17:46PM +0200, Pali Rohár wrote: > > On Saturday 14 April 2018 13:17:11 Lukas Wunner wrote: > > > On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > > > > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > > > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > > > > Do you have any suggestion to check if it connects to the system via > > > > > > Thunderbolt? > > > > > > > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > > > > like this: > > > > > > > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > > > > > > > And what about PCI-e device attached to ExpressCard slot? > > > > > > I don't know of a bullet-proof way to recognize those. In theory > > > one could check if the PCIe port above the GPU is a non-hotplug > > > root port, but I think there are machines with hotplug capable > > > root ports with GPUs below them that aren't actually removable. > > > > > > However I think ExpressCard-attached GPUs were rare, much less ones > > > with integrated HDA controller, so in reality that's probably a > > > non-issue. > > > > Hm... maybe another idea: Is it possible to detect which audio pci > > device belongs to graphics card via vga_switcheroo? Currently, looking > > at output it is same PCI device as graphic card, just different PCI > > function. > > No, the DRM drivers don't filter ExpressCard-attached GPUs when > registering with vga_switcheroo. > They do filter Thunderbolt-attached GPUs. So check via vga_switcheroo should at least work for Thunderbolt GPUs. > The ExpressCard 2.0 spec defines some ACPI stuff that *might* be > used to recognize root ports that are ExpressCard slots, but I'm > not sure how reliable that is. So for EC we do not know or have reliable detection. I do not know if it is possible, but for me it looks like that check via vga_switcheroo should be better then adding another heuristic to other drivers. Lukas, what do you think? And it is possible to use this check for detecting audio device? And once we would have good/reliable check for EC devices we can add it into vga_switcheroo and all users of it could benefit. Anyway, I think that vga_switcheroo should filter also EC GPU cards if it already filters Thunderbolt. > I don't have such a machine and have no experience with it. > > This is from the MacBookPro8,3 DSDT: > > Device (RP04) > { > Name (_ADR, 0x001C0003) > OperationRegion (A1E0, PCI_Config, 0x19, 0x01) > Field (A1E0, ByteAcc, NoLock, Preserve) > { > SECB, 8 > } > > Device (EXCD) > { > Name (_ADR, 0x00) > Name (_SUN, 0x01) > Method (_RMV, 0, NotSerialized) > { > Return (0x01) > } > > Name (_EJD, "\\_SB.PCI0.EHC2.HUBN.PRTN.PRT4") > } > ... > } > > Thanks, > > Lukas
On Mon, Apr 16, 2018 at 04:25:12PM +0200, Pali Rohár wrote: > On Sunday 15 April 2018 21:05:23 Lukas Wunner wrote: > > On Sun, Apr 15, 2018 at 07:17:46PM +0200, Pali Rohár wrote: > > > On Saturday 14 April 2018 13:17:11 Lukas Wunner wrote: > > > > On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > > > > > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > > > > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > > > > > Do you have any suggestion to check if it connects to the system via > > > > > > > Thunderbolt? > > > > > > > > > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > > > > > like this: > > > > > > > > > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > > > > > > > > > And what about PCI-e device attached to ExpressCard slot? > > > > > > > > I don't know of a bullet-proof way to recognize those. > > > > > > Hm... maybe another idea: Is it possible to detect which audio pci > > > device belongs to graphics card via vga_switcheroo? Currently, looking > > > at output it is same PCI device as graphic card, just different PCI > > > function. > > > > No, the DRM drivers don't filter ExpressCard-attached GPUs when > > registering with vga_switcheroo. > > > They do filter Thunderbolt-attached GPUs. > > So check via vga_switcheroo should at least work for Thunderbolt GPUs. > > I do not know if it is possible, but for me it looks like that check via > vga_switcheroo should be better then adding another heuristic to other > drivers. It is way simpler and more straightforward if hda_intel.c calls pci_is_thunderbolt_attached() directly, as I've suggested above. The DRM drivers call that as well and register with vga_switcheroo only if it returns false. So if hda_intel.c asked vga_switcheroo and got back a negative answer, it would never know whether it's because the DRM driver hasn't finished probing yet, or it's module is blacklisted, or whatever. > And once we would have good/reliable check for EC devices we can add it > into vga_switcheroo and all users of it could benefit. Anyway, I think > that vga_switcheroo should filter also EC GPU cards if it already > filters Thunderbolt. vga_switcheroo doesn't do the filtering, the DRM drivers themselves decice whether to register with vga_switcheroo or not. The motivation is to avoid middle layers and instead provide the DRM drivers with a library of helpers that they may call at their own discretion. See this blog post and the links therein for background info: http://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html Thanks, Lukas
On Tuesday 17 April 2018 04:38:29 Lukas Wunner wrote: > On Mon, Apr 16, 2018 at 04:25:12PM +0200, Pali Rohár wrote: > > On Sunday 15 April 2018 21:05:23 Lukas Wunner wrote: > > > On Sun, Apr 15, 2018 at 07:17:46PM +0200, Pali Rohár wrote: > > > > On Saturday 14 April 2018 13:17:11 Lukas Wunner wrote: > > > > > On Sat, Apr 14, 2018 at 12:49:50PM +0200, Pali Rohár wrote: > > > > > > On Saturday 14 April 2018 12:45:12 Lukas Wunner wrote: > > > > > > > On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote: > > > > > > > > Do you have any suggestion to check if it connects to the system via > > > > > > > > Thunderbolt? > > > > > > > > > > > > > > Just use pci_is_thunderbolt_attached(), introduced by 8531e283bee6, > > > > > > > like this: > > > > > > > > > > > > > > if (check_dell_switchable_gfx(pci) && !pci_is_thunderbolt_attached(pci)) > > > > > > > > > > > > And what about PCI-e device attached to ExpressCard slot? > > > > > > > > > > I don't know of a bullet-proof way to recognize those. > > > > > > > > Hm... maybe another idea: Is it possible to detect which audio pci > > > > device belongs to graphics card via vga_switcheroo? Currently, looking > > > > at output it is same PCI device as graphic card, just different PCI > > > > function. > > > > > > No, the DRM drivers don't filter ExpressCard-attached GPUs when > > > registering with vga_switcheroo. > > > > > They do filter Thunderbolt-attached GPUs. > > > > So check via vga_switcheroo should at least work for Thunderbolt GPUs. > > > > I do not know if it is possible, but for me it looks like that check via > > vga_switcheroo should be better then adding another heuristic to other > > drivers. > > It is way simpler and more straightforward if hda_intel.c calls > pci_is_thunderbolt_attached() directly, as I've suggested above. > > The DRM drivers call that as well and register with vga_switcheroo > only if it returns false. So if hda_intel.c asked vga_switcheroo > and got back a negative answer, it would never know whether it's > because the DRM driver hasn't finished probing yet, or it's module > is blacklisted, or whatever. Ok, module blacklisting can be a problem. > > > And once we would have good/reliable check for EC devices we can add it > > into vga_switcheroo and all users of it could benefit. Anyway, I think > > that vga_switcheroo should filter also EC GPU cards if it already > > filters Thunderbolt. > > vga_switcheroo doesn't do the filtering, the DRM drivers themselves > decice whether to register with vga_switcheroo or not. The motivation > is to avoid middle layers and instead provide the DRM drivers with > a library of helpers that they may call at their own discretion. Understood. Driver itself can also do more work, e.g. ask ACPI or do some other check to decide whatever to register to vga_switcheroo or not. But probably, hda_intel should have same logic for disabling device like gpu drivers for detection if they are going to register into vga_switcheroo or not. Right now this seems to be really complicated and suggested solution with pci_is_thunderbolt_attached() looks to be really simple and better for now. > See this blog post and the links therein for background info: > http://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html > > Thanks, > > Lukas
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 7720e3102bcc..d9310616d5ca 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -49,6 +49,8 @@ #include <linux/clocksource.h> #include <linux/time.h> #include <linux/completion.h> +#include <linux/dell-common.h> +#include <linux/dmi.h> #ifdef CONFIG_X86 /* for snoop control */ @@ -1629,6 +1631,38 @@ static void check_msi(struct azx *chip) } } +#if IS_ENABLED(CONFIG_DELL_LAPTOP) +static bool check_dell_switchable_gfx(struct pci_dev *pdev) +{ + bool (*dell_switchable_gfx_is_enabled_func)(void); + bool enabled; + + /* Only need to check for Dell laptops and AIOs */ + if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) || + !(dmi_match(DMI_CHASSIS_TYPE, "10") || + dmi_match(DMI_CHASSIS_TYPE, "13")) || + !(pdev->vendor == PCI_VENDOR_ID_ATI || + pdev->vendor == PCI_VENDOR_ID_NVIDIA)) + return false; + + dell_switchable_gfx_is_enabled_func = + symbol_request(dell_switchable_gfx_is_enabled); + if (!dell_switchable_gfx_is_enabled_func) + return false; + + enabled = dell_switchable_gfx_is_enabled_func(); + + symbol_put(dell_switchable_gfx_is_enabled); + + return enabled; +} +#else +static bool check_dell_switchable_gfx(struct pci_dev *pdev) +{ + return false; +} +#endif + /* check the snoop mode availability */ static void azx_check_snoop_available(struct azx *chip) { @@ -1711,6 +1745,11 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, if (err < 0) return err; + if (check_dell_switchable_gfx(pci)) { + pci_disable_device(pci); + return -ENODEV; + } + hda = kzalloc(sizeof(*hda), GFP_KERNEL); if (!hda) { pci_disable_device(pci);
Some Dell platforms (Preicsion 7510/7710/7520/7720) have a BIOS option "Switchable Graphics" (SG). When SG is enabled, we have: 00:02.0 VGA compatible controller: Intel Corporation Device 591b (rev 04) 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Polaris10] 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580] The Intel Audio outputs all the sound, including HDMI audio. The audio controller comes with AMD graphics doesn't get used. When SG is disabled, we have: 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31) 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Polaris10] 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580] Now it's a typical discrete-only system. HDMI audio comes from AMD audio controller, others from Intel audio controller. When SG is enabled, the unused AMD audio controller still exposes its sysfs, so userspace still opens the control file and stream. If userspace tries to output sound through the stream, it hangs when runtime suspend kicks in: [ 12.796265] snd_hda_intel 0000:01:00.1: Disabling via vga_switcheroo [ 12.796367] snd_hda_intel 0000:01:00.1: Cannot lock devices! Since the discrete audio controller isn't useful when SG enabled, we should just disable the device. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead of error code. Use DMI_DEV_TYPE_OEM_STRING to match Dell System. v2: Mario suggested to squash the HDA part into the same series. sound/pci/hda/hda_intel.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)