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

[Kai Heng Feng]

On Mar 10, 2018, at 2:50 PM, Lukas Wunner  wrote:

On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!

Since the discrete audio controller isn't useful when SG enabled, we
should just disable the device.


The platform does have a NVIDIA variant, but the discrete NVIDIA have a
audio controller, hence it doesn't have the issue.


Sorry, I don't quite understand:  The AMD variant *also* has an audio
controller, so what's the difference?  Or did you mean the Nvidia
variant *doesn't* have an audio controller?


It does, but the audio controller only shows under SG disabled.



Pretty much all modern Nvidia GPUs do have an integrated HDA
controller, however it's possible to hide it by clearing a bit
at offset 0x488 in the GPU's config space.  Some BIOSes hide
the HDA if no external display is attached.


This is how the Nvidia variant works:

When SG is enabled:
One audio controller, the integrated one.
All sounds comes from the integrated one, including DP/HDMI.

When SG is disabled:
Two audio controller.
The DP/HDMI sounds come from the Nvidia audio controller. Others are from  
the integrated audio controller.




I could imagine that the BIOS of the Dell machines in question
hides the HDA if Switchable Graphics is enabled.  If that is the
case, be aware that there's an ongoing discussion to always expose
the HDA controller because the behavior of some BIOSes to only
expose the HDA when a display is attached causes massive problems
with Linux' HDA driver:
https://bugs.freedesktop.org/show_bug.cgi?id=75985


The system in the bug doesn't have "Switchable Graphics".

Right now all sounds output are from integrated audio controller on SG  
enabled machines.




If we decide to always expose the HDA controller on Nvidia cards,
you may need to also match for the Nvidia vendor ID here.


Thanks for the info, I'll also put Nvidia vendor ID on next patch version.



Thanks,

Lukas

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

[Kai Heng Feng]

On Mar 10, 2018, at 2:50 PM, Lukas Wunner  wrote:

On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!

Since the discrete audio controller isn't useful when SG enabled, we
should just disable the device.


The platform does have a NVIDIA variant, but the discrete NVIDIA have a
audio controller, hence it doesn't have the issue.


Sorry, I don't quite understand:  The AMD variant *also* has an audio
controller, so what's the difference?  Or did you mean the Nvidia
variant *doesn't* have an audio controller?


It does, but the audio controller only shows under SG disabled.



Pretty much all modern Nvidia GPUs do have an integrated HDA
controller, however it's possible to hide it by clearing a bit
at offset 0x488 in the GPU's config space.  Some BIOSes hide
the HDA if no external display is attached.


This is how the Nvidia variant works:

When SG is enabled:
One audio controller, the integrated one.
All sounds comes from the integrated one, including DP/HDMI.

When SG is disabled:
Two audio controller.
The DP/HDMI sounds come from the Nvidia audio controller. Others are from  
the integrated audio controller.




I could imagine that the BIOS of the Dell machines in question
hides the HDA if Switchable Graphics is enabled.  If that is the
case, be aware that there's an ongoing discussion to always expose
the HDA controller because the behavior of some BIOSes to only
expose the HDA when a display is attached causes massive problems
with Linux' HDA driver:
https://bugs.freedesktop.org/show_bug.cgi?id=75985


The system in the bug doesn't have "Switchable Graphics".

Right now all sounds output are from integrated audio controller on SG  
enabled machines.




If we decide to always expose the HDA controller on Nvidia cards,
you may need to also match for the Nvidia vendor ID here.


Thanks for the info, I'll also put Nvidia vendor ID on next patch version.



Thanks,

Lukas

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

[Mario.Limonciello]

> -Original Message-
> From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> Sent: Tuesday, March 13, 2018 3:56 PM
> To: Limonciello, Mario <mario_limoncie...@dell.com>
> Cc: pali.ro...@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; 
> alsa-de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> 
> 
> > On Mar 12, 2018, at 9:30 AM, mario.limoncie...@dell.com wrote:
> >
> >>> I think the missing aspect is that this is only used in AIO and laptop
> >>> form
> >>> factors where the discrete graphics is in a non-removable form factor.
> >>
> >> Why we are not checking if kernel is running on AIO or laptop form
> >> factor then? Or it is not possible?
> >
> > Kai Heng, can you please confirm if AIO sets chassis type properly?
> > It should be 0Dh.
> >
> > If so, then I think as second check for chassis type should be possible for
> > next version of this patch.
> 
> The chassis type is correctly set as 0Dh on AIOs.
> 
> >
> >> Basically what I see there is that we need to detect if current HW
> >> platform has switchable graphics and check how is configured AUDIO MUX.
> >>
> >> But instead of directly checking hw state of audio MUX, we are trying to
> >> check something different which could get us information of state of the
> >> audio mux.
> >>
> >> I suspect that we do not have a way how to check audio MUX directly, so
> >> it needs to be done indirectly -- via some Dell SMBIOS call and some
> >> other heuristic. This is something which should be specified either in
> >> comment or in commit message (problem of type: we need X, but check for
> >> Y).
> >>
> >> And if we are doing this check indirectly, we should do the most
> >> specific test and not more general.
> >
> > Right.
> >
> >> I think that PCI vendor ID check of audio device is more general test
> >> then checking if kernel is running on Dell laptop (check via DMI). And
> >> if we can check also if running on AIO or laptop form, then it would be
> >> more specific test.
> >>
> >>> Running with your hypothetical though, what would happen is if it's
> >>> on a non-Dell machine the PCI check would pass and then the code
> >>> would not be executed by dell-laptop (since dell-smbios didn't load).
> >>
> >> Right.
> >>
> >>> If it was on a Dell machine it would load but the BIOS would return
> >>> either Switchable graphics turned off, or invalid token.
> >>>
> >>> Even though these aren't real for switchable graphics on Dell I don't
> >>> see a problem with either of these situations if it ever came up.
> >>
> >> I see, this solution is working...
> >>
> >> ... but, I see there a very bad precedense. What would happen if another
> >> laptop manufactor comes with similar solution for hybrid graphics. Does
> >> it mean that hda audio driver would try to call for every one vendor its
> >> vendor dependent API function (EFI, SMM, WMI, whatever) to check if
> >> current HW has some switchable graphics and needs special checks?
> >>
> >> Those vendor dependent API functions (which Dell SMBIOS is) should be
> >> really called on vendor hardware.
> >>
> >> Otherwise audio drivers would load bunch of the other vendor dependent
> >> platform modules and all of those modules (except maximally one) just
> >> return error.
> >
> > Sure the more specific the check the less symbol requests needed that
> > will fail.
> >
> > Kai Heng,
> >
> > Can you please use Alex Hung's OEM strings patch in your series to match
> > "Dell System" in OEM strings too?
> 
> Sure, but this probably need to wait till it gets merged in Linus' tree.
> 

It's already in -next isn't it?  Isn't that sufficient to indicate your patch 
depends
on that?  Or it could come through platform-x86 tree too if needed.  During
the merge window one of the duplicates will get dropped.

> >
> > Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
> > vendor that should satisfy all of Pali's concerns I think.
> >
> > If anyone thinks that's too much, please speak up.
> >
> >>> Compiling a whitelist is a wasted effort because it will have to change
> >>> Every year for every new platform that has AMD switchable graphics.
> >>
> >> I agree, But see that this patch already uses vendor ID whitelisting.
> >
> > I mean making a whitelist of all individual affected Dell platforms will
> > grow
> > each year.  I want to avoid that approach.

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

[Mario.Limonciello]

> -Original Message-
> From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> Sent: Tuesday, March 13, 2018 3:56 PM
> To: Limonciello, Mario 
> Cc: pali.ro...@gmail.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; Linux
> Kernel Mailing List ; 
> alsa-de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> 
> 
> > On Mar 12, 2018, at 9:30 AM, mario.limoncie...@dell.com wrote:
> >
> >>> I think the missing aspect is that this is only used in AIO and laptop
> >>> form
> >>> factors where the discrete graphics is in a non-removable form factor.
> >>
> >> Why we are not checking if kernel is running on AIO or laptop form
> >> factor then? Or it is not possible?
> >
> > Kai Heng, can you please confirm if AIO sets chassis type properly?
> > It should be 0Dh.
> >
> > If so, then I think as second check for chassis type should be possible for
> > next version of this patch.
> 
> The chassis type is correctly set as 0Dh on AIOs.
> 
> >
> >> Basically what I see there is that we need to detect if current HW
> >> platform has switchable graphics and check how is configured AUDIO MUX.
> >>
> >> But instead of directly checking hw state of audio MUX, we are trying to
> >> check something different which could get us information of state of the
> >> audio mux.
> >>
> >> I suspect that we do not have a way how to check audio MUX directly, so
> >> it needs to be done indirectly -- via some Dell SMBIOS call and some
> >> other heuristic. This is something which should be specified either in
> >> comment or in commit message (problem of type: we need X, but check for
> >> Y).
> >>
> >> And if we are doing this check indirectly, we should do the most
> >> specific test and not more general.
> >
> > Right.
> >
> >> I think that PCI vendor ID check of audio device is more general test
> >> then checking if kernel is running on Dell laptop (check via DMI). And
> >> if we can check also if running on AIO or laptop form, then it would be
> >> more specific test.
> >>
> >>> Running with your hypothetical though, what would happen is if it's
> >>> on a non-Dell machine the PCI check would pass and then the code
> >>> would not be executed by dell-laptop (since dell-smbios didn't load).
> >>
> >> Right.
> >>
> >>> If it was on a Dell machine it would load but the BIOS would return
> >>> either Switchable graphics turned off, or invalid token.
> >>>
> >>> Even though these aren't real for switchable graphics on Dell I don't
> >>> see a problem with either of these situations if it ever came up.
> >>
> >> I see, this solution is working...
> >>
> >> ... but, I see there a very bad precedense. What would happen if another
> >> laptop manufactor comes with similar solution for hybrid graphics. Does
> >> it mean that hda audio driver would try to call for every one vendor its
> >> vendor dependent API function (EFI, SMM, WMI, whatever) to check if
> >> current HW has some switchable graphics and needs special checks?
> >>
> >> Those vendor dependent API functions (which Dell SMBIOS is) should be
> >> really called on vendor hardware.
> >>
> >> Otherwise audio drivers would load bunch of the other vendor dependent
> >> platform modules and all of those modules (except maximally one) just
> >> return error.
> >
> > Sure the more specific the check the less symbol requests needed that
> > will fail.
> >
> > Kai Heng,
> >
> > Can you please use Alex Hung's OEM strings patch in your series to match
> > "Dell System" in OEM strings too?
> 
> Sure, but this probably need to wait till it gets merged in Linus' tree.
> 

It's already in -next isn't it?  Isn't that sufficient to indicate your patch 
depends
on that?  Or it could come through platform-x86 tree too if needed.  During
the merge window one of the duplicates will get dropped.

> >
> > Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
> > vendor that should satisfy all of Pali's concerns I think.
> >
> > If anyone thinks that's too much, please speak up.
> >
> >>> Compiling a whitelist is a wasted effort because it will have to change
> >>> Every year for every new platform that has AMD switchable graphics.
> >>
> >> I agree, But see that this patch already uses vendor ID whitelisting.
> >
> > I mean making a whitelist of all individual affected Dell platforms will
> > grow
> > each year.  I want to avoid that approach.

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

[Kai Heng Feng]

On Mar 12, 2018, at 9:30 AM, mario.limoncie...@dell.com wrote:

I think the missing aspect is that this is only used in AIO and laptop  
form

factors where the discrete graphics is in a non-removable form factor.


Why we are not checking if kernel is running on AIO or laptop form
factor then? Or it is not possible?


Kai Heng, can you please confirm if AIO sets chassis type properly?
It should be 0Dh.

If so, then I think as second check for chassis type should be possible for
next version of this patch.


The chassis type is correctly set as 0Dh on AIOs.




Basically what I see there is that we need to detect if current HW
platform has switchable graphics and check how is configured AUDIO MUX.

But instead of directly checking hw state of audio MUX, we are trying to
check something different which could get us information of state of the
audio mux.

I suspect that we do not have a way how to check audio MUX directly, so
it needs to be done indirectly -- via some Dell SMBIOS call and some
other heuristic. This is something which should be specified either in
comment or in commit message (problem of type: we need X, but check for
Y).

And if we are doing this check indirectly, we should do the most
specific test and not more general.


Right.


I think that PCI vendor ID check of audio device is more general test
then checking if kernel is running on Dell laptop (check via DMI). And
if we can check also if running on AIO or laptop form, then it would be
more specific test.


Running with your hypothetical though, what would happen is if it's
on a non-Dell machine the PCI check would pass and then the code
would not be executed by dell-laptop (since dell-smbios didn't load).


Right.


If it was on a Dell machine it would load but the BIOS would return
either Switchable graphics turned off, or invalid token.

Even though these aren't real for switchable graphics on Dell I don't
see a problem with either of these situations if it ever came up.


I see, this solution is working...

... but, I see there a very bad precedense. What would happen if another
laptop manufactor comes with similar solution for hybrid graphics. Does
it mean that hda audio driver would try to call for every one vendor its
vendor dependent API function (EFI, SMM, WMI, whatever) to check if
current HW has some switchable graphics and needs special checks?

Those vendor dependent API functions (which Dell SMBIOS is) should be
really called on vendor hardware.

Otherwise audio drivers would load bunch of the other vendor dependent
platform modules and all of those modules (except maximally one) just
return error.


Sure the more specific the check the less symbol requests needed that
will fail.

Kai Heng,

Can you please use Alex Hung's OEM strings patch in your series to match
"Dell System" in OEM strings too?


Sure, but this probably need to wait till it gets merged in Linus' tree.



Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
vendor that should satisfy all of Pali's concerns I think.

If anyone thinks that's too much, please speak up.


Compiling a whitelist is a wasted effort because it will have to change
Every year for every new platform that has AMD switchable graphics.


I agree, But see that this patch already uses vendor ID whitelisting.


I mean making a whitelist of all individual affected Dell platforms will  
grow

each year.  I want to avoid that approach.

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

[Kai Heng Feng]

On Mar 12, 2018, at 9:30 AM, mario.limoncie...@dell.com wrote:

I think the missing aspect is that this is only used in AIO and laptop  
form

factors where the discrete graphics is in a non-removable form factor.


Why we are not checking if kernel is running on AIO or laptop form
factor then? Or it is not possible?


Kai Heng, can you please confirm if AIO sets chassis type properly?
It should be 0Dh.

If so, then I think as second check for chassis type should be possible for
next version of this patch.


The chassis type is correctly set as 0Dh on AIOs.




Basically what I see there is that we need to detect if current HW
platform has switchable graphics and check how is configured AUDIO MUX.

But instead of directly checking hw state of audio MUX, we are trying to
check something different which could get us information of state of the
audio mux.

I suspect that we do not have a way how to check audio MUX directly, so
it needs to be done indirectly -- via some Dell SMBIOS call and some
other heuristic. This is something which should be specified either in
comment or in commit message (problem of type: we need X, but check for
Y).

And if we are doing this check indirectly, we should do the most
specific test and not more general.


Right.


I think that PCI vendor ID check of audio device is more general test
then checking if kernel is running on Dell laptop (check via DMI). And
if we can check also if running on AIO or laptop form, then it would be
more specific test.


Running with your hypothetical though, what would happen is if it's
on a non-Dell machine the PCI check would pass and then the code
would not be executed by dell-laptop (since dell-smbios didn't load).


Right.


If it was on a Dell machine it would load but the BIOS would return
either Switchable graphics turned off, or invalid token.

Even though these aren't real for switchable graphics on Dell I don't
see a problem with either of these situations if it ever came up.


I see, this solution is working...

... but, I see there a very bad precedense. What would happen if another
laptop manufactor comes with similar solution for hybrid graphics. Does
it mean that hda audio driver would try to call for every one vendor its
vendor dependent API function (EFI, SMM, WMI, whatever) to check if
current HW has some switchable graphics and needs special checks?

Those vendor dependent API functions (which Dell SMBIOS is) should be
really called on vendor hardware.

Otherwise audio drivers would load bunch of the other vendor dependent
platform modules and all of those modules (except maximally one) just
return error.


Sure the more specific the check the less symbol requests needed that
will fail.

Kai Heng,

Can you please use Alex Hung's OEM strings patch in your series to match
"Dell System" in OEM strings too?


Sure, but this probably need to wait till it gets merged in Linus' tree.



Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
vendor that should satisfy all of Pali's concerns I think.

If anyone thinks that's too much, please speak up.


Compiling a whitelist is a wasted effort because it will have to change
Every year for every new platform that has AMD switchable graphics.


I agree, But see that this patch already uses vendor ID whitelisting.


I mean making a whitelist of all individual affected Dell platforms will  
grow

each year.  I want to avoid that approach.

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

[Mario.Limonciello]

> > I think the missing aspect is that this is only used in AIO and laptop form
> > factors where the discrete graphics is in a non-removable form factor.
> 
> Why we are not checking if kernel is running on AIO or laptop form
> factor then? Or it is not possible?

Kai Heng, can you please confirm if AIO sets chassis type properly?
It should be 0Dh.

If so, then I think as second check for chassis type should be possible for
next version of this patch.

> 
> Basically what I see there is that we need to detect if current HW
> platform has switchable graphics and check how is configured AUDIO MUX.
> 
> But instead of directly checking hw state of audio MUX, we are trying to
> check something different which could get us information of state of the
> audio mux.
> 
> I suspect that we do not have a way how to check audio MUX directly, so
> it needs to be done indirectly -- via some Dell SMBIOS call and some
> other heuristic. This is something which should be specified either in
> comment or in commit message (problem of type: we need X, but check for
> Y).
> 
> And if we are doing this check indirectly, we should do the most
> specific test and not more general.

Right.

> 
> I think that PCI vendor ID check of audio device is more general test
> then checking if kernel is running on Dell laptop (check via DMI). And
> if we can check also if running on AIO or laptop form, then it would be
> more specific test.
> 
> > Running with your hypothetical though, what would happen is if it's
> > on a non-Dell machine the PCI check would pass and then the code
> > would not be executed by dell-laptop (since dell-smbios didn't load).
> 
> Right.
> 
> > If it was on a Dell machine it would load but the BIOS would return
> > either Switchable graphics turned off, or invalid token.
> >
> > Even though these aren't real for switchable graphics on Dell I don't
> > see a problem with either of these situations if it ever came up.
> 
> I see, this solution is working...
> 
> ... but, I see there a very bad precedense. What would happen if another
> laptop manufactor comes with similar solution for hybrid graphics. Does
> it mean that hda audio driver would try to call for every one vendor its
> vendor dependent API function (EFI, SMM, WMI, whatever) to check if
> current HW has some switchable graphics and needs special checks?
> 
> Those vendor dependent API functions (which Dell SMBIOS is) should be
> really called on vendor hardware.
> 
> Otherwise audio drivers would load bunch of the other vendor dependent
> platform modules and all of those modules (except maximally one) just
> return error.
> 

Sure the more specific the check the less symbol requests needed that
will fail.

Kai Heng,

Can you please use Alex Hung's OEM strings patch in your series to match
"Dell System" in OEM strings too?

Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
vendor that should satisfy all of Pali's concerns I think.

If anyone thinks that's too much, please speak up.

> > Compiling a whitelist is a wasted effort because it will have to change
> > Every year for every new platform that has AMD switchable graphics.
> 
> I agree, But see that this patch already uses vendor ID whitelisting.

I mean making a whitelist of all individual affected Dell platforms will grow
each year.  I want to avoid that approach.

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

[Mario.Limonciello]

> > I think the missing aspect is that this is only used in AIO and laptop form
> > factors where the discrete graphics is in a non-removable form factor.
> 
> Why we are not checking if kernel is running on AIO or laptop form
> factor then? Or it is not possible?

Kai Heng, can you please confirm if AIO sets chassis type properly?
It should be 0Dh.

If so, then I think as second check for chassis type should be possible for
next version of this patch.

> 
> Basically what I see there is that we need to detect if current HW
> platform has switchable graphics and check how is configured AUDIO MUX.
> 
> But instead of directly checking hw state of audio MUX, we are trying to
> check something different which could get us information of state of the
> audio mux.
> 
> I suspect that we do not have a way how to check audio MUX directly, so
> it needs to be done indirectly -- via some Dell SMBIOS call and some
> other heuristic. This is something which should be specified either in
> comment or in commit message (problem of type: we need X, but check for
> Y).
> 
> And if we are doing this check indirectly, we should do the most
> specific test and not more general.

Right.

> 
> I think that PCI vendor ID check of audio device is more general test
> then checking if kernel is running on Dell laptop (check via DMI). And
> if we can check also if running on AIO or laptop form, then it would be
> more specific test.
> 
> > Running with your hypothetical though, what would happen is if it's
> > on a non-Dell machine the PCI check would pass and then the code
> > would not be executed by dell-laptop (since dell-smbios didn't load).
> 
> Right.
> 
> > If it was on a Dell machine it would load but the BIOS would return
> > either Switchable graphics turned off, or invalid token.
> >
> > Even though these aren't real for switchable graphics on Dell I don't
> > see a problem with either of these situations if it ever came up.
> 
> I see, this solution is working...
> 
> ... but, I see there a very bad precedense. What would happen if another
> laptop manufactor comes with similar solution for hybrid graphics. Does
> it mean that hda audio driver would try to call for every one vendor its
> vendor dependent API function (EFI, SMM, WMI, whatever) to check if
> current HW has some switchable graphics and needs special checks?
> 
> Those vendor dependent API functions (which Dell SMBIOS is) should be
> really called on vendor hardware.
> 
> Otherwise audio drivers would load bunch of the other vendor dependent
> platform modules and all of those modules (except maximally one) just
> return error.
> 

Sure the more specific the check the less symbol requests needed that
will fail.

Kai Heng,

Can you please use Alex Hung's OEM strings patch in your series to match
"Dell System" in OEM strings too?

Between chassis type, OEM strings match, and AMD vendor/Dell subsystem
vendor that should satisfy all of Pali's concerns I think.

If anyone thinks that's too much, please speak up.

> > Compiling a whitelist is a wasted effort because it will have to change
> > Every year for every new platform that has AMD switchable graphics.
> 
> I agree, But see that this patch already uses vendor ID whitelisting.

I mean making a whitelist of all individual affected Dell platforms will grow
each year.  I want to avoid that approach.

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

[Pali Rohár]

On Sunday 11 March 2018 14:03:13 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-x86-
> > ow...@vger.kernel.org] On Behalf Of Pali Rohár
> > Sent: Saturday, March 10, 2018 6:38 PM
> > To: Limonciello, Mario <mario_limoncie...@dell.com>
> > Cc: kai.heng.f...@canonical.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> > a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; 
> > linux-
> > ker...@vger.kernel.org; alsa-de...@alsa-project.org
> > Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> > Dell
> > platforms with Switchable Graphics
> > 
> > On Friday 09 March 2018 09:59:39 mario.limoncie...@dell.com wrote:
> > > > > 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.
> > >
> > > Uh Dell subsystem ID means it's Dell no?
> > 
> > What would prevent you to take PCI device marked with Dell ID and put it
> > into non-Dell computer? I do not believe that Dell PCI devices are
> > configured to work only in Dell branded devices and refuse to power up
> > in others.
> 
> I think the missing aspect is that this is only used in AIO and laptop form
> factors where the discrete graphics is in a non-removable form factor.

Why we are not checking if kernel is running on AIO or laptop form
factor then? Or it is not possible?

Basically what I see there is that we need to detect if current HW
platform has switchable graphics and check how is configured AUDIO MUX.

But instead of directly checking hw state of audio MUX, we are trying to
check something different which could get us information of state of the
audio mux.

I suspect that we do not have a way how to check audio MUX directly, so
it needs to be done indirectly -- via some Dell SMBIOS call and some
other heuristic. This is something which should be specified either in
comment or in commit message (problem of type: we need X, but check for
Y).

And if we are doing this check indirectly, we should do the most
specific test and not more general.

I think that PCI vendor ID check of audio device is more general test
then checking if kernel is running on Dell laptop (check via DMI). And
if we can check also if running on AIO or laptop form, then it would be
more specific test.

> Running with your hypothetical though, what would happen is if it's
> on a non-Dell machine the PCI check would pass and then the code
> would not be executed by dell-laptop (since dell-smbios didn't load).

Right.

> If it was on a Dell machine it would load but the BIOS would return
> either Switchable graphics turned off, or invalid token.
> 
> Even though these aren't real for switchable graphics on Dell I don't
> see a problem with either of these situations if it ever came up.

I see, this solution is working...

... but, I see there a very bad precedense. What would happen if another
laptop manufactor comes with similar solution for hybrid graphics. Does
it mean that hda audio driver would try to call for every one vendor its
vendor dependent API function (EFI, SMM, WMI, whatever) to check if
current HW has some switchable graphics and needs special checks?

Those vendor dependent API functions (which Dell SMBIOS is) should be
really called on vendor hardware.

Otherwise audio drivers would load bunch of the other vendor dependent
platform modules and all of those modules (except maximally one) just
return error.

> > 
> > If there is Dell ID then it just means that PCI device itself is Dell's.
> > And not that machine in which that device is plugged is also 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
> > &g

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

[Pali Rohár]

On Sunday 11 March 2018 14:03:13 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-x86-
> > ow...@vger.kernel.org] On Behalf Of Pali Rohár
> > Sent: Saturday, March 10, 2018 6:38 PM
> > To: Limonciello, Mario 
> > Cc: kai.heng.f...@canonical.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> > a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; 
> > linux-
> > ker...@vger.kernel.org; alsa-de...@alsa-project.org
> > Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> > Dell
> > platforms with Switchable Graphics
> > 
> > On Friday 09 March 2018 09:59:39 mario.limoncie...@dell.com wrote:
> > > > > 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.
> > >
> > > Uh Dell subsystem ID means it's Dell no?
> > 
> > What would prevent you to take PCI device marked with Dell ID and put it
> > into non-Dell computer? I do not believe that Dell PCI devices are
> > configured to work only in Dell branded devices and refuse to power up
> > in others.
> 
> I think the missing aspect is that this is only used in AIO and laptop form
> factors where the discrete graphics is in a non-removable form factor.

Why we are not checking if kernel is running on AIO or laptop form
factor then? Or it is not possible?

Basically what I see there is that we need to detect if current HW
platform has switchable graphics and check how is configured AUDIO MUX.

But instead of directly checking hw state of audio MUX, we are trying to
check something different which could get us information of state of the
audio mux.

I suspect that we do not have a way how to check audio MUX directly, so
it needs to be done indirectly -- via some Dell SMBIOS call and some
other heuristic. This is something which should be specified either in
comment or in commit message (problem of type: we need X, but check for
Y).

And if we are doing this check indirectly, we should do the most
specific test and not more general.

I think that PCI vendor ID check of audio device is more general test
then checking if kernel is running on Dell laptop (check via DMI). And
if we can check also if running on AIO or laptop form, then it would be
more specific test.

> Running with your hypothetical though, what would happen is if it's
> on a non-Dell machine the PCI check would pass and then the code
> would not be executed by dell-laptop (since dell-smbios didn't load).

Right.

> If it was on a Dell machine it would load but the BIOS would return
> either Switchable graphics turned off, or invalid token.
> 
> Even though these aren't real for switchable graphics on Dell I don't
> see a problem with either of these situations if it ever came up.

I see, this solution is working...

... but, I see there a very bad precedense. What would happen if another
laptop manufactor comes with similar solution for hybrid graphics. Does
it mean that hda audio driver would try to call for every one vendor its
vendor dependent API function (EFI, SMM, WMI, whatever) to check if
current HW has some switchable graphics and needs special checks?

Those vendor dependent API functions (which Dell SMBIOS is) should be
really called on vendor hardware.

Otherwise audio drivers would load bunch of the other vendor dependent
platform modules and all of those modules (except maximally one) just
return error.

> > 
> > If there is Dell ID then it just means that PCI device itself is Dell's.
> > And not that machine in which that device is plugged is also 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

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

[Mario.Limonciello]

> -Original Message-
> From: Lukas Wunner [mailto:lu...@wunner.de]
> Sent: Saturday, March 10, 2018 2:51 PM
> To: Kai Heng Feng <kai.heng.f...@canonical.com>
> Cc: Pali Rohár <pali.ro...@gmail.com>; mj...@srcf.ucam.org;
> dvh...@infradead.org; a...@infradead.org; Limonciello, Mario
> <mario_limoncie...@dell.com>; ti...@suse.com; platform-driver-
> x...@vger.kernel.org; Linux Kernel Mailing List 
> <linux-kernel@vger.kernel.org>; alsa-
> de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
> > >>[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!
> > >>
> > >>Since the discrete audio controller isn't useful when SG enabled, we
> > >>should just disable the device.
> >
> > The platform does have a NVIDIA variant, but the discrete NVIDIA have a
> > audio controller, hence it doesn't have the issue.
> 
> Sorry, I don't quite understand:  The AMD variant *also* has an audio
> controller, so what's the difference?  Or did you mean the Nvidia
> variant *doesn't* have an audio controller?
> 
> Pretty much all modern Nvidia GPUs do have an integrated HDA
> controller, however it's possible to hide it by clearing a bit
> at offset 0x488 in the GPU's config space.  Some BIOSes hide
> the HDA if no external display is attached.
> 
> I could imagine that the BIOS of the Dell machines in question
> hides the HDA if Switchable Graphics is enabled.  If that is the
> case, be aware that there's an ongoing discussion to always expose
> the HDA controller because the behavior of some BIOSes to only
> expose the HDA when a display is attached causes massive problems
> with Linux' HDA driver:
> https://bugs.freedesktop.org/show_bug.cgi?id=75985
> 
> If we decide to always expose the HDA controller on Nvidia cards,
> you may need to also match for the Nvidia vendor ID here.
> 
Like I said I don't know the details on why NVIDIA isn't affected, Kai
Heng will need to comment here.  You might very well be right.

I stand by my point that the heuristic makes sense for what exists
In the kernel today though.  If the point you mention is the reason
why NV isn't affected and this changes in the future it's a simple extra
|| to match NV cards too, or to remove the AMD part of the check
and just look for Dell subsystem vendor instead of AMD product
vendor and Dell subsystem vendor.

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

[Mario.Limonciello]

> -Original Message-
> From: Lukas Wunner [mailto:lu...@wunner.de]
> Sent: Saturday, March 10, 2018 2:51 PM
> To: Kai Heng Feng 
> Cc: Pali Rohár ; mj...@srcf.ucam.org;
> dvh...@infradead.org; a...@infradead.org; Limonciello, Mario
> ; ti...@suse.com; platform-driver-
> x...@vger.kernel.org; Linux Kernel Mailing List 
> ; alsa-
> de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
> > >>[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!
> > >>
> > >>Since the discrete audio controller isn't useful when SG enabled, we
> > >>should just disable the device.
> >
> > The platform does have a NVIDIA variant, but the discrete NVIDIA have a
> > audio controller, hence it doesn't have the issue.
> 
> Sorry, I don't quite understand:  The AMD variant *also* has an audio
> controller, so what's the difference?  Or did you mean the Nvidia
> variant *doesn't* have an audio controller?
> 
> Pretty much all modern Nvidia GPUs do have an integrated HDA
> controller, however it's possible to hide it by clearing a bit
> at offset 0x488 in the GPU's config space.  Some BIOSes hide
> the HDA if no external display is attached.
> 
> I could imagine that the BIOS of the Dell machines in question
> hides the HDA if Switchable Graphics is enabled.  If that is the
> case, be aware that there's an ongoing discussion to always expose
> the HDA controller because the behavior of some BIOSes to only
> expose the HDA when a display is attached causes massive problems
> with Linux' HDA driver:
> https://bugs.freedesktop.org/show_bug.cgi?id=75985
> 
> If we decide to always expose the HDA controller on Nvidia cards,
> you may need to also match for the Nvidia vendor ID here.
> 
Like I said I don't know the details on why NVIDIA isn't affected, Kai
Heng will need to comment here.  You might very well be right.

I stand by my point that the heuristic makes sense for what exists
In the kernel today though.  If the point you mention is the reason
why NV isn't affected and this changes in the future it's a simple extra
|| to match NV cards too, or to remove the AMD part of the check
and just look for Dell subsystem vendor instead of AMD product
vendor and Dell subsystem vendor.

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

[Mario.Limonciello]

> -Original Message-
> From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-x86-
> ow...@vger.kernel.org] On Behalf Of Pali Rohár
> Sent: Saturday, March 10, 2018 6:38 PM
> To: Limonciello, Mario <mario_limoncie...@dell.com>
> Cc: kai.heng.f...@canonical.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; 
> linux-
> ker...@vger.kernel.org; alsa-de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> On Friday 09 March 2018 09:59:39 mario.limoncie...@dell.com wrote:
> > > > 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.
> >
> > Uh Dell subsystem ID means it's Dell no?
> 
> What would prevent you to take PCI device marked with Dell ID and put it
> into non-Dell computer? I do not believe that Dell PCI devices are
> configured to work only in Dell branded devices and refuse to power up
> in others.

I think the missing aspect is that this is only used in AIO and laptop form
factors where the discrete graphics is in a non-removable form factor.

Running with your hypothetical though, what would happen is if it's
on a non-Dell machine the PCI check would pass and then the code
would not be executed by dell-laptop (since dell-smbios didn't load).

If it was on a Dell machine it would load but the BIOS would return
either Switchable graphics turned off, or invalid token.

Even though these aren't real for switchable graphics on Dell I don't
see a problem with either of these situations if it ever came up.
 
> 
> If there is Dell ID then it just means that PCI device itself is Dell's.
> And not that machine in which that device is plugged is also 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.
> >
> > Right now this is affected to both AIO desktop and laptops.
> >
> > IIRC you won't end up with switchable graphics in traditional desktop that 
> > you
> > can remove PCI card.  If this code was run on a traditional desktop with a
> > AMD PCI card that BIOS query result should be invalid token (which will 
> > infer
> > switchable off to this routine).
> >
> > >
> > > 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.
> >
> > Kai Heng can explain exactly why NVIDIA isn't affected.
> > This is probably good information to include in the commit message too.
> 
> Yes, extending commit message is a good idea.
> 
> But here I'm talking about future, NVIDIA cards could be in future.
> 
> I still think that whitelisting devices based on vendor ID by some
> measurements at one time is a bad idea. It is fragile which can stop
> working in the future.

Compiling a whitelist is a wasted effort because it will have to change
Every year for every new platform that has AMD switchable graphics.

This heuristic that is selected covers switchable graphics back for the
past generations that Canonical has tested and fixed, not just this
current one.

If that situation you refer to happens, it will be on new hardware that's
not yet enabled by the Linux kernel.  We can cross that bridge when we
come to it with either a newly proposed heuristic or some adjustments
for whitelist/blacklist.

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

[Mario.Limonciello]

> -Original Message-
> From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-x86-
> ow...@vger.kernel.org] On Behalf Of Pali Rohár
> Sent: Saturday, March 10, 2018 6:38 PM
> To: Limonciello, Mario 
> Cc: kai.heng.f...@canonical.com; mj...@srcf.ucam.org; dvh...@infradead.org;
> a...@infradead.org; ti...@suse.com; platform-driver-...@vger.kernel.org; 
> linux-
> ker...@vger.kernel.org; alsa-de...@alsa-project.org
> Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for 
> Dell
> platforms with Switchable Graphics
> 
> On Friday 09 March 2018 09:59:39 mario.limoncie...@dell.com wrote:
> > > > 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.
> >
> > Uh Dell subsystem ID means it's Dell no?
> 
> What would prevent you to take PCI device marked with Dell ID and put it
> into non-Dell computer? I do not believe that Dell PCI devices are
> configured to work only in Dell branded devices and refuse to power up
> in others.

I think the missing aspect is that this is only used in AIO and laptop form
factors where the discrete graphics is in a non-removable form factor.

Running with your hypothetical though, what would happen is if it's
on a non-Dell machine the PCI check would pass and then the code
would not be executed by dell-laptop (since dell-smbios didn't load).

If it was on a Dell machine it would load but the BIOS would return
either Switchable graphics turned off, or invalid token.

Even though these aren't real for switchable graphics on Dell I don't
see a problem with either of these situations if it ever came up.
 
> 
> If there is Dell ID then it just means that PCI device itself is Dell's.
> And not that machine in which that device is plugged is also 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.
> >
> > Right now this is affected to both AIO desktop and laptops.
> >
> > IIRC you won't end up with switchable graphics in traditional desktop that 
> > you
> > can remove PCI card.  If this code was run on a traditional desktop with a
> > AMD PCI card that BIOS query result should be invalid token (which will 
> > infer
> > switchable off to this routine).
> >
> > >
> > > 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.
> >
> > Kai Heng can explain exactly why NVIDIA isn't affected.
> > This is probably good information to include in the commit message too.
> 
> Yes, extending commit message is a good idea.
> 
> But here I'm talking about future, NVIDIA cards could be in future.
> 
> I still think that whitelisting devices based on vendor ID by some
> measurements at one time is a bad idea. It is fragile which can stop
> working in the future.

Compiling a whitelist is a wasted effort because it will have to change
Every year for every new platform that has AMD switchable graphics.

This heuristic that is selected covers switchable graphics back for the
past generations that Canonical has tested and fixed, not just this
current one.

If that situation you refer to happens, it will be on new hardware that's
not yet enabled by the Linux kernel.  We can cross that bridge when we
come to it with either a newly proposed heuristic or some adjustments
for whitelist/blacklist.

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

[Pali Rohár]

On Saturday 10 March 2018 07:50:39 Lukas Wunner wrote:
> Pretty much all modern Nvidia GPUs do have an integrated HDA
> controller, however it's possible to hide it by clearing a bit
> at offset 0x488 in the GPU's config space.  Some BIOSes hide
> the HDA if no external display is attached.
> 
> I could imagine that the BIOS of the Dell machines in question
> hides the HDA if Switchable Graphics is enabled.  If that is the
> case, be aware that there's an ongoing discussion to always expose
> the HDA controller because the behavior of some BIOSes to only
> expose the HDA when a display is attached causes massive problems
> with Linux' HDA driver:
> https://bugs.freedesktop.org/show_bug.cgi?id=75985
> 
> If we decide to always expose the HDA controller on Nvidia cards,
> you may need to also match for the Nvidia vendor ID here.

This is probably the example of reason why current solution based on
vendor id is fragile and could be broken in future... if such changes to
unhide devices are going to be implemented.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature

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

[Pali Rohár]

On Saturday 10 March 2018 07:50:39 Lukas Wunner wrote:
> Pretty much all modern Nvidia GPUs do have an integrated HDA
> controller, however it's possible to hide it by clearing a bit
> at offset 0x488 in the GPU's config space.  Some BIOSes hide
> the HDA if no external display is attached.
> 
> I could imagine that the BIOS of the Dell machines in question
> hides the HDA if Switchable Graphics is enabled.  If that is the
> case, be aware that there's an ongoing discussion to always expose
> the HDA controller because the behavior of some BIOSes to only
> expose the HDA when a display is attached causes massive problems
> with Linux' HDA driver:
> https://bugs.freedesktop.org/show_bug.cgi?id=75985
> 
> If we decide to always expose the HDA controller on Nvidia cards,
> you may need to also match for the Nvidia vendor ID here.

This is probably the example of reason why current solution based on
vendor id is fragile and could be broken in future... if such changes to
unhide devices are going to be implemented.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature

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:59:39 mario.limoncie...@dell.com wrote:
> > > 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.
> 
> Uh Dell subsystem ID means it's Dell no?

What would prevent you to take PCI device marked with Dell ID and put it
into non-Dell computer? I do not believe that Dell PCI devices are
configured to work only in Dell branded devices and refuse to power up
in others.

If there is Dell ID then it just means that PCI device itself is Dell's.
And not that machine in which that device is plugged is also 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.
> 
> Right now this is affected to both AIO desktop and laptops.
> 
> IIRC you won't end up with switchable graphics in traditional desktop that you
> can remove PCI card.  If this code was run on a traditional desktop with a 
> AMD PCI card that BIOS query result should be invalid token (which will infer
> switchable off to this routine).
> 
> > 
> > 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.
> 
> Kai Heng can explain exactly why NVIDIA isn't affected.
> This is probably good information to include in the commit message too.

Yes, extending commit message is a good idea.

But here I'm talking about future, NVIDIA cards could be in future.

I still think that whitelisting devices based on vendor ID by some
measurements at one time is a bad idea. It is fragile which can stop
working in the future.

> > 
> > > > >
> > > > > 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.ro...@gmail.com

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature

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:59:39 mario.limoncie...@dell.com wrote:
> > > 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.
> 
> Uh Dell subsystem ID means it's Dell no?

What would prevent you to take PCI device marked with Dell ID and put it
into non-Dell computer? I do not believe that Dell PCI devices are
configured to work only in Dell branded devices and refuse to power up
in others.

If there is Dell ID then it just means that PCI device itself is Dell's.
And not that machine in which that device is plugged is also 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.
> 
> Right now this is affected to both AIO desktop and laptops.
> 
> IIRC you won't end up with switchable graphics in traditional desktop that you
> can remove PCI card.  If this code was run on a traditional desktop with a 
> AMD PCI card that BIOS query result should be invalid token (which will infer
> switchable off to this routine).
> 
> > 
> > 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.
> 
> Kai Heng can explain exactly why NVIDIA isn't affected.
> This is probably good information to include in the commit message too.

Yes, extending commit message is a good idea.

But here I'm talking about future, NVIDIA cards could be in future.

I still think that whitelisting devices based on vendor ID by some
measurements at one time is a bad idea. It is fragile which can stop
working in the future.

> > 
> > > > >
> > > > > 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.ro...@gmail.com

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature

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

[Lukas Wunner]

On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
> >>[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!
> >>
> >>Since the discrete audio controller isn't useful when SG enabled, we
> >>should just disable the device.
> 
> The platform does have a NVIDIA variant, but the discrete NVIDIA have a
> audio controller, hence it doesn't have the issue.

Sorry, I don't quite understand:  The AMD variant *also* has an audio
controller, so what's the difference?  Or did you mean the Nvidia
variant *doesn't* have an audio controller?

Pretty much all modern Nvidia GPUs do have an integrated HDA
controller, however it's possible to hide it by clearing a bit
at offset 0x488 in the GPU's config space.  Some BIOSes hide
the HDA if no external display is attached.

I could imagine that the BIOS of the Dell machines in question
hides the HDA if Switchable Graphics is enabled.  If that is the
case, be aware that there's an ongoing discussion to always expose
the HDA controller because the behavior of some BIOSes to only
expose the HDA when a display is attached causes massive problems
with Linux' HDA driver:
https://bugs.freedesktop.org/show_bug.cgi?id=75985

If we decide to always expose the HDA controller on Nvidia cards,
you may need to also match for the Nvidia vendor ID here.

Thanks,

Lukas

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

[Lukas Wunner]

On Fri, Mar 09, 2018 at 05:30:15PM +0800, Kai Heng Feng 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 :01:00.1: Disabling via vga_switcheroo
> >>[ 12.796367] snd_hda_intel :01:00.1: Cannot lock devices!
> >>
> >>Since the discrete audio controller isn't useful when SG enabled, we
> >>should just disable the device.
> 
> The platform does have a NVIDIA variant, but the discrete NVIDIA have a
> audio controller, hence it doesn't have the issue.

Sorry, I don't quite understand:  The AMD variant *also* has an audio
controller, so what's the difference?  Or did you mean the Nvidia
variant *doesn't* have an audio controller?

Pretty much all modern Nvidia GPUs do have an integrated HDA
controller, however it's possible to hide it by clearing a bit
at offset 0x488 in the GPU's config space.  Some BIOSes hide
the HDA if no external display is attached.

I could imagine that the BIOS of the Dell machines in question
hides the HDA if Switchable Graphics is enabled.  If that is the
case, be aware that there's an ongoing discussion to always expose
the HDA controller because the behavior of some BIOSes to only
expose the HDA when a display is attached causes massive problems
with Linux' HDA driver:
https://bugs.freedesktop.org/show_bug.cgi?id=75985

If we decide to always expose the HDA controller on Nvidia cards,
you may need to also match for the Nvidia vendor ID here.

Thanks,

Lukas

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

[Mario.Limonciello]

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

Uh Dell subsystem ID means it's Dell no?

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

Right now this is affected to both AIO desktop and laptops.

IIRC you won't end up with switchable graphics in traditional desktop that you
can remove PCI card.  If this code was run on a traditional desktop with a 
AMD PCI card that BIOS query result should be invalid token (which will infer
switchable off to this routine).

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

Kai Heng can explain exactly why NVIDIA isn't affected.
This is probably good information to include in the commit message too.

> 
> > > >
> > > > 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.ro...@gmail.com

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

[Mario.Limonciello]

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

Uh Dell subsystem ID means it's Dell no?

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

Right now this is affected to both AIO desktop and laptops.

IIRC you won't end up with switchable graphics in traditional desktop that you
can remove PCI card.  If this code was run on a traditional desktop with a 
AMD PCI card that BIOS query result should be invalid token (which will infer
switchable off to this routine).

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

Kai Heng can explain exactly why NVIDIA isn't affected.
This is probably good information to include in the commit message too.

> 
> > > >
> > > > 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.ro...@gmail.com

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.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> > Sent: Friday, March 9, 2018 5:30 PM
> > To: Pali Rohár <pali.ro...@gmail.com>
> > Cc: mj...@srcf.ucam.org; dvh...@infradead.org; a...@infradead.org;
> > Limonciello, Mario <mario_limoncie...@dell.com>; ti...@suse.com; platform-
> > driver-...@vger.kernel.org; Linux Kernel Mailing List  > ker...@vger.kernel.org>; alsa-de...@alsa-project.org
> > 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.ro...@gmail.com> 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 :01:00.1: Disabling via vga_switcheroo
> > >> [ 12.796367] snd_hda_intel :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.f...@canonical.com>
> > >> ---
> > >> 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 
> > >>  #include 
> > >>  #include 
> > >> +#include 
> > >>
> > >>  #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
> > > 

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.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> > Sent: Friday, March 9, 2018 5:30 PM
> > To: Pali Rohár 
> > Cc: mj...@srcf.ucam.org; dvh...@infradead.org; a...@infradead.org;
> > Limonciello, Mario ; ti...@suse.com; platform-
> > driver-...@vger.kernel.org; Linux Kernel Mailing List  > ker...@vger.kernel.org>; alsa-de...@alsa-project.org
> > 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  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 :01:00.1: Disabling via vga_switcheroo
> > >> [ 12.796367] snd_hda_intel :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 
> > >> ---
> > >> 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 
> > >>  #include 
> > >>  #include 
> > >> +#include 
> > >>
> > >>  #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?
> > 

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

[Mario.Limonciello]

> -Original Message-
> From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> Sent: Friday, March 9, 2018 5:30 PM
> To: Pali Rohár <pali.ro...@gmail.com>
> Cc: mj...@srcf.ucam.org; dvh...@infradead.org; a...@infradead.org;
> Limonciello, Mario <mario_limoncie...@dell.com>; ti...@suse.com; platform-
> driver-...@vger.kernel.org; Linux Kernel Mailing List  ker...@vger.kernel.org>; alsa-de...@alsa-project.org
> 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.ro...@gmail.com> 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 :01:00.1: Disabling via vga_switcheroo
> >> [ 12.796367] snd_hda_intel :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.f...@canonical.com>
> >> ---
> >> 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 
> >>  #include 
> >>  #include 
> >> +#include 
> >>
> >>  #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.

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

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

[Mario.Limonciello]

> -Original Message-
> From: Kai Heng Feng [mailto:kai.heng.f...@canonical.com]
> Sent: Friday, March 9, 2018 5:30 PM
> To: Pali Rohár 
> Cc: mj...@srcf.ucam.org; dvh...@infradead.org; a...@infradead.org;
> Limonciello, Mario ; ti...@suse.com; platform-
> driver-...@vger.kernel.org; Linux Kernel Mailing List  ker...@vger.kernel.org>; alsa-de...@alsa-project.org
> 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  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 :01:00.1: Disabling via vga_switcheroo
> >> [ 12.796367] snd_hda_intel :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 
> >> ---
> >> 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 
> >>  #include 
> >>  #include 
> >> +#include 
> >>
> >>  #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.

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

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

[Kai Heng Feng]

On Mar 9, 2018, at 5:02 PM, Pali Rohár  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 :01:00.1: Disabling via vga_switcheroo
[ 12.796367] snd_hda_intel :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 
---
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 
 #include 
 #include 
+#include 

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



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.


Kai-Heng




+   dell_switchable_gfx_enabled_func =
+   symbol_request(dell_switchable_gfx_enabled);
+   if (!dell_switchable_gfx_enabled_func)
+   return false;
+
+   err = dell_switchable_gfx_enabled_func();
+
+   symbol_put(dell_switchable_gfx_enabled);
+
+   return !err ? enabled : false;
+}
+#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)
 {
@@ -1702,6 +1732,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);


--
Pali Rohár
pali.ro...@gmail.com

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

[Kai Heng Feng]

On Mar 9, 2018, at 5:02 PM, Pali Rohár  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 :01:00.1: Disabling via vga_switcheroo
[ 12.796367] snd_hda_intel :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 
---
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 
 #include 
 #include 
+#include 

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



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.


Kai-Heng




+   dell_switchable_gfx_enabled_func =
+   symbol_request(dell_switchable_gfx_enabled);
+   if (!dell_switchable_gfx_enabled_func)
+   return false;
+
+   err = dell_switchable_gfx_enabled_func();
+
+   symbol_put(dell_switchable_gfx_enabled);
+
+   return !err ? enabled : false;
+}
+#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)
 {
@@ -1702,6 +1732,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);


--
Pali Rohár
pali.ro...@gmail.com

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

[Pali Rohár]

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 :01:00.1: Disabling via vga_switcheroo
> [ 12.796367] snd_hda_intel :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 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  
>  #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?

Interesting question would be, how handle this situation Windows?

> + dell_switchable_gfx_enabled_func =
> + symbol_request(dell_switchable_gfx_enabled);
> + if (!dell_switchable_gfx_enabled_func)
> + return false;
> +
> + err = dell_switchable_gfx_enabled_func();
> +
> + symbol_put(dell_switchable_gfx_enabled);
> +
> + return !err ? enabled : false;
> +}
> +#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)
>  {
> @@ -1702,6 +1732,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);

-- 
Pali Rohár
pali.ro...@gmail.com

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

[Pali Rohár]

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 :01:00.1: Disabling via vga_switcheroo
> [ 12.796367] snd_hda_intel :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 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  
>  #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?

Interesting question would be, how handle this situation Windows?

> + dell_switchable_gfx_enabled_func =
> + symbol_request(dell_switchable_gfx_enabled);
> + if (!dell_switchable_gfx_enabled_func)
> + return false;
> +
> + err = dell_switchable_gfx_enabled_func();
> +
> + symbol_put(dell_switchable_gfx_enabled);
> +
> + return !err ? enabled : false;
> +}
> +#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)
>  {
> @@ -1702,6 +1732,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);

-- 
Pali Rohár
pali.ro...@gmail.com