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

[Pali RohÃr]

On Friday 09 March 2018 09:34:01 Mario.Limonciello@xxxxxxxx wrote:
> > -----Original Message-----
> > From: Kai Heng Feng [mailto:kai.heng.feng@xxxxxxxxxxxxx]
> > Sent: Friday, March 9, 2018 5:30 PM
> > To: Pali RohÃr <pali.rohar@xxxxxxxxx>
> > Cc: mjg59@xxxxxxxxxxxxx; dvhart@xxxxxxxxxxxxx; andy@xxxxxxxxxxxxx;
> > Limonciello, Mario <Mario_Limonciello@xxxxxxxx>; tiwai@xxxxxxxx; platform-
> > driver-x86@xxxxxxxxxxxxxxx; Linux Kernel Mailing List <linux-
> > kernel@xxxxxxxxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell
> > platforms with Switchable Graphics
> > 
> > 
> > > On Mar 9, 2018, at 5:02 PM, Pali RohÃr <pali.rohar@xxxxxxxxx> wrote:
> > >
> > > On Thursday 08 March 2018 17:10:23 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@xxxxxxxxxxxxx>
> > >> ---
> > >> v2: Mario suggested to squash the HDA part into the same series.
> > >>
> > >>  sound/pci/hda/hda_intel.c | 35 +++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 35 insertions(+)
> > >>
> > >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > >> index 96143df19b21..8e3e8b88624a 100644
> > >> --- a/sound/pci/hda/hda_intel.c
> > >> +++ b/sound/pci/hda/hda_intel.c
> > >> @@ -49,6 +49,7 @@
> > >>  #include <linux/clocksource.h>
> > >>  #include <linux/time.h>
> > >>  #include <linux/completion.h>
> > >> +#include <linux/dell-common.h>
> > >>
> > >>  #ifdef CONFIG_X86
> > >>  /* for snoop control */
> > >> @@ -1620,6 +1621,35 @@ static void check_msi(struct azx *chip)
> > >>  	}
> > >>  }
> > >>
> > >> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
> > >> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
> > >> +{
> > >> +	static int (*dell_switchable_gfx_enabled_func)(bool *);
> > >> +	bool enabled;
> > >> +	int err;
> > >> +
> > >> +	if (pdev->vendor != PCI_VENDOR_ID_ATI ||
> > >> +	    pdev->subsystem_vendor != PCI_VENDOR_ID_DELL)
> > >> +		return false;
> > >
> > > Are you sure that you want to do this check unconditionally on all
> > > machines which have enabled CONFIG_DELL_LAPTOP?
> > >
> > > Subvendor ID_DELL for dell specific code is not suspicious, but ID_ATI
> > > is. What would happen if ATI vendor changes to NVIDIA or other which is
> > > not related to Dell?
> > 
> > We only check it when it's both ATI and DELL, otherwise just return false?
> > 
> > The platform does have a NVIDIA variant, but the discrete NVIDIA have a
> > audio controller, hence it doesn't have the issue.
> > The issue only happens to AMD/ATI configs with "Switchable Graphics" option.
> > 
> 
> Pali is your concern that this code for matching vendor/subsystem is running
> on non-Dell too?  The only other recommendation I think that can be to restrict
> to matching Dell OEM strings in SMBIOS table, but I don't think that's any better
> than the matching for VID/SSVID.

My concern is about adding a new machine specific code into generic
driver, which check is done just by PCI vendor and subvendor.

In future there can be new models or other PCI devices which matches
above condition even they would not have any switchable graphics, nor
they would manufactured by Dell.

Also I can imagine that in future (or maybe already now?) it is possible
to find PCI device which pass above checks and connect this PCI device
into desktop /server / any non-laptop device.

If this switchable graphics solution is specific to dell laptops, then
rather checking for PCI vendor/subvevendor main check, there should be
main check via DMI strings.

Hardware is changing relatively quickly and there is absolutely no
guarantee that e.g. NVIDIA would not start providing audio controller in
similar like AMD and it would be put in those Dell machines.

> > >
> > > Interesting question would be, how handle this situation Windows?
> > 
> > I don't know how this platform handles this on Windows, I guess we need
> > Mario to shed some lights here.
> 
> Sorry I don't have this information to share.   I don't think it's too useful here
> anyway though because Windows driver architecture is much different in this
> area.
> 
> 

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx