[alsa-devel] [PATCH v3 1/3] dell-led: Change dell-led.h to dell-common.h
This header will be used for more than just led. Change it to a more generic name.
Cc: Mario Limonciello mario.limonciello@dell.com 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.
drivers/platform/x86/dell-laptop.c | 2 +- include/linux/{dell-led.h => dell-common.h} | 4 ++-- sound/pci/hda/dell_wmi_helper.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) rename include/linux/{dell-led.h => dell-common.h} (61%)
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index c52c6723374b..8ba820e6c3d0 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -29,7 +29,7 @@ #include <linux/mm.h> #include <linux/i8042.h> #include <linux/debugfs.h> -#include <linux/dell-led.h> +#include <linux/dell-common.h> #include <linux/seq_file.h> #include <acpi/video.h> #include "dell-rbtn.h" diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h similarity index 61% rename from include/linux/dell-led.h rename to include/linux/dell-common.h index 92521471517f..37e4b614dd74 100644 --- a/include/linux/dell-led.h +++ b/include/linux/dell-common.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#ifndef __DELL_LED_H__ -#define __DELL_LED_H__ +#ifndef __DELL_COMMON_H__ +#define __DELL_COMMON_H__
int dell_micmute_led_set(int on);
diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c index 1b48a8c19d28..56050cc3c0ee 100644 --- a/sound/pci/hda/dell_wmi_helper.c +++ b/sound/pci/hda/dell_wmi_helper.c @@ -4,7 +4,7 @@ */
#if IS_ENABLED(CONFIG_DELL_LAPTOP) -#include <linux/dell-led.h> +#include <linux/dell-common.h>
enum { MICMUTE_LED_ON,
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 mario.limonciello@dell.com 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.
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
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(+)
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);
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: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 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
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
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
participants (5)
-
Darren Hart -
Kai-Heng Feng -
Lukas Wunner -
Pali Rohár -
Takashi Iwai