LinuxLists.cc - [PATCH v3 1/3] dell-led: Change dell-led.h to dell-common.h

[permalink] [raw]

Subject: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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 <[email protected]>
---
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

2018-04-12 10:46:43

[permalink] [raw]

Subject: [PATCH v3 2/3] platform/x86: dell-*: Add interface for switchable graphics status query

On some Dell platforms, there's a BIOS option "Enable Switchable
Graphics". This information is useful if we want to do different things
based on this value, e.g. disable unused audio controller that comes
with the discrete graphics.

Cc: Mario Limonciello <[email protected]>
Signed-off-by: Kai-Heng Feng <[email protected]>
---
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.

drivers/platform/x86/dell-laptop.c | 17 +++++++++++++++++
drivers/platform/x86/dell-smbios-base.c | 2 ++
drivers/platform/x86/dell-smbios.h | 2 ++
include/linux/dell-common.h | 1 +
4 files changed, 22 insertions(+)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 8ba820e6c3d0..033a27b190cc 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2116,6 +2116,23 @@ int dell_micmute_led_set(int state)
}
EXPORT_SYMBOL_GPL(dell_micmute_led_set);

+bool dell_switchable_gfx_is_enabled(void)
+{
+ struct calling_interface_buffer buffer;
+ struct calling_interface_token *token;
+
+ token = dell_smbios_find_token(SWITCHABLE_GRAPHICS_ENABLE);
+ if (!token)
+ return false;
+
+ dell_fill_request(&buffer, token->location, 0, 0, 0);
+ if (dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD))
+ return false;
+
+ return !!buffer.output[1];
+}
+EXPORT_SYMBOL_GPL(dell_switchable_gfx_is_enabled);
+
static int __init dell_init(void)
{
struct calling_interface_token *token;
diff --git a/drivers/platform/x86/dell-smbios-base.c b/drivers/platform/x86/dell-smbios-base.c
index 33fb2a20458a..881ce42f0ca7 100644
--- a/drivers/platform/x86/dell-smbios-base.c
+++ b/drivers/platform/x86/dell-smbios-base.c
@@ -86,6 +86,8 @@ struct token_range {
static struct token_range token_whitelist[] = {
/* used by userspace: fwupdate */
{CAP_SYS_ADMIN, CAPSULE_EN_TOKEN, CAPSULE_DIS_TOKEN},
+ /* can indicate to userspace Switchable Graphics enable status */
+ {CAP_SYS_ADMIN, SWITCHABLE_GRAPHICS_ENABLE, SWITCHABLE_GRAPHICS_DISABLE},
/* can indicate to userspace that WMI is needed */
{0x0000, WSMT_EN_TOKEN, WSMT_DIS_TOKEN}
};
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index d8adaf959740..7863e6a7cff8 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -37,6 +37,8 @@
#define KBD_LED_AUTO_100_TOKEN 0x02F6
#define GLOBAL_MIC_MUTE_ENABLE 0x0364
#define GLOBAL_MIC_MUTE_DISABLE 0x0365
+#define SWITCHABLE_GRAPHICS_ENABLE 0x037A
+#define SWITCHABLE_GRAPHICS_DISABLE 0x037B

struct notifier_block;

diff --git a/include/linux/dell-common.h b/include/linux/dell-common.h
index 37e4b614dd74..1a90bc9a3bea 100644
--- a/include/linux/dell-common.h
+++ b/include/linux/dell-common.h
@@ -3,5 +3,6 @@
#define __DELL_COMMON_H__

int dell_micmute_led_set(int on);
+bool dell_switchable_gfx_is_enabled(void);

#endif
--
2.17.0

2018-04-12 10:53:30

by Takashi Iwai

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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 <[email protected]>

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

2018-04-12 12:03:35

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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;
> > + }

--
Pali Rohár
[email protected]

2018-04-12 14:16:50

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

at 6:50 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>
> 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

2018-04-12 14:19:38

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

at 6:59 PM, Pali Rohár <[email protected]> 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
> [email protected]

2018-04-13 16:10:13

by Darren Hart

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

On Thu, Apr 12, 2018 at 10:15:41PM +0800, Kai-Heng Feng wrote:
> at 6:59 PM, Pali Roh?r <[email protected]> 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.

--
Darren Hart
VMware Open Source Technology Center

2018-04-14 10:47:30

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

2018-04-14 10:51:15

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

--
Pali Rohár
[email protected]

Attachments:

(No filename) (2.08 kB)
signature.asc (201.00 B)
Download all attachments

2018-04-14 11:21:57

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

2018-04-14 11:26:59

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

On Thu, Apr 12, 2018 at 10:12:49PM +0800, Kai-Heng Feng wrote:
> at 6:50 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
> >
> > 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

2018-04-15 17:19:11

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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.

--
Pali Rohár
[email protected]

Attachments:

(No filename) (1.27 kB)
signature.asc (201.00 B)
Download all attachments

2018-04-15 19:07:20

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

2018-04-16 14:37:20

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

--
Pali Rohár
[email protected]

2018-04-17 02:40:06

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

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

2018-04-17 07:54:44