All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kai Heng Feng <kai.heng.feng@canonical.com>
To: Mario.Limonciello@dell.com
Cc: pali.rohar@gmail.com, mjg59@srcf.ucam.org, dvhart@infradead.org,
	andy@infradead.org, tiwai@suse.com,
	platform-driver-x86@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	alsa-devel@alsa-project.org
Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics
Date: Tue, 13 Mar 2018 15:56:10 +0800	[thread overview]
Message-ID: <F24A81D4-F447-4128-85EA-7C4A04E844E0@canonical.com> (raw)
In-Reply-To: <c49f28560996405fa4173a5a492bf201@ausx13mpc124.AMER.DELL.COM>



> On Mar 12, 2018, at 9:30 AM, Mario.Limonciello@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.

WARNING: multiple messages have this Message-ID (diff)
From: Kai Heng Feng <kai.heng.feng@canonical.com>
To: Mario.Limonciello@dell.com
Cc: mjg59@srcf.ucam.org, alsa-devel@alsa-project.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	tiwai@suse.com, platform-driver-x86@vger.kernel.org,
	pali.rohar@gmail.com, dvhart@infradead.org, andy@infradead.org
Subject: Re: [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics
Date: Tue, 13 Mar 2018 15:56:10 +0800	[thread overview]
Message-ID: <F24A81D4-F447-4128-85EA-7C4A04E844E0@canonical.com> (raw)
In-Reply-To: <c49f28560996405fa4173a5a492bf201@ausx13mpc124.AMER.DELL.COM>



> On Mar 12, 2018, at 9:30 AM, Mario.Limonciello@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.

  reply	other threads:[~2018-03-13  7:56 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-08  9:10 [PATCH v2 1/3] dell-led: Change dell-led.h to dell-common.h Kai-Heng Feng
2018-03-08  9:10 ` [PATCH v2 2/3] platform/x86: dell-*: Add interface for switchable graphics status query Kai-Heng Feng
2018-03-08 16:38   ` Pali Rohár
2018-03-08 16:38     ` Pali Rohár
2018-03-09  8:14     ` Kai Heng Feng
2018-03-08  9:10 ` [PATCH v2 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics Kai-Heng Feng
2018-03-08  9:38   ` [alsa-devel] " Lukas Wunner
2018-03-08 10:38     ` Kai Heng Feng
2018-03-08 10:38       ` Kai Heng Feng
2018-03-08 11:30       ` [alsa-devel] " Lukas Wunner
2018-03-08 11:30         ` Lukas Wunner
2018-03-09  8:22         ` [alsa-devel] " Kai Heng Feng
2018-03-09  9:02   ` Pali Rohár
2018-03-09  9:02     ` Pali Rohár
2018-03-09  9:30     ` Kai Heng Feng
2018-03-09  9:34       ` Mario.Limonciello
2018-03-09  9:34         ` Mario.Limonciello
2018-03-09  9:46         ` Pali Rohár
2018-03-09  9:46           ` Pali Rohár
2018-03-09  9:59           ` Mario.Limonciello
2018-03-09  9:59             ` Mario.Limonciello
2018-03-10 10:38             ` Pali Rohár
2018-03-10 10:38               ` Pali Rohár
2018-03-11 14:03               ` Mario.Limonciello
2018-03-11 14:03                 ` Mario.Limonciello
2018-03-11 14:30                 ` Pali Rohár
2018-03-11 14:30                   ` Pali Rohár
2018-03-12  1:30                   ` Mario.Limonciello
2018-03-12  1:30                     ` Mario.Limonciello
2018-03-13  7:56                     ` Kai Heng Feng [this message]
2018-03-13  7:56                       ` Kai Heng Feng
2018-03-13  8:13                       ` Mario.Limonciello
2018-03-13  8:13                         ` Mario.Limonciello
2018-03-10  6:50       ` Lukas Wunner
2018-03-10  6:50         ` Lukas Wunner
2018-03-10 10:40         ` Pali Rohár
2018-03-10 10:40           ` Pali Rohár
2018-03-11 14:06         ` Mario.Limonciello
2018-03-11 14:06           ` Mario.Limonciello
2018-03-13  8:24         ` Kai Heng Feng
2018-04-07 16:44 ` [PATCH v2 1/3] dell-led: Change dell-led.h to dell-common.h Darren Hart
2018-04-07 16:44   ` Darren Hart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F24A81D4-F447-4128-85EA-7C4A04E844E0@canonical.com \
    --to=kai.heng.feng@canonical.com \
    --cc=Mario.Limonciello@dell.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=andy@infradead.org \
    --cc=dvhart@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjg59@srcf.ucam.org \
    --cc=pali.rohar@gmail.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.