lkml.org 
[lkml]   [2022]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] asus-wmi: Support the hardware GPU MUX on some laptops
From
On 8/28/2022 03:05, Luke Jones wrote:
>
>
> On Wed, Aug 24 2022 at 08:09:08 -0500, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>> On 8/24/22 08:03, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 8/24/22 14:53, Mario Limonciello wrote:
>>>> On 8/24/22 07:40, Hans de Goede wrote:
>>>>> Hi All,
>>>>>
>>>>> On 8/22/22 17:43, Limonciello, Mario wrote:
>>>>>> On 8/21/2022 18:07, Luke Jones wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> On Mon, Aug 15 2022 at 23:16:12 -0500, Mario Limonciello
>>>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>>> On 8/13/22 04:26, Luke D. Jones wrote:
>>>>>>>>> Support the hardware GPU MUX switch available on some models. This
>>>>>>>>> switch can toggle the MUX between:
>>>>>>>>>
>>>>>>>>> - 0, Dedicated mode
>>>>>>>>> - 1, Optimus mode
>>>>>>>>>
>>>>>>>>> Optimus mode is the regular iGPU + dGPU available, while dedicated
>>>>>>>>> mode switches the system to have only the dGPU available.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>>>>> ---
>>>>>>>>>     .../ABI/testing/sysfs-platform-asus-wmi       | 11 ++++
>>>>>>>>>     drivers/platform/x86/asus-wmi.c               | 62
>>>>>>>>> +++++++++++++++++++
>>>>>>>>>     include/linux/platform_data/x86/asus-wmi.h    |  3 +
>>>>>>>>>     3 files changed, 76 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>>> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>>> index 574b5170a37d..03124eab7f01 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>>> @@ -58,6 +58,17 @@ Description:
>>>>>>>>>                 * 1 - overboost,
>>>>>>>>>                 * 2 - silent
>>>>>>>>>     +What: /sys/devices/platform/<platform>/gpu_mux_mode
>>>>>>>>> +Date:          Aug 2022
>>>>>>>>> +KernelVersion: 6.1
>>>>>>>>> +Contact:       "Luke Jones" <luke@ljones.dev>
>>>>>>>>> +Description:
>>>>>>>>> +               Switch the GPU hardware MUX mode. Laptops with
>>>>>>>>> this feature can
>>>>>>>>> +               can be toggled to boot with only the dGPU
>>>>>>>>> (discrete mode) or in
>>>>>>>>> +               standard Optimus/Hybrid mode. On switch a
>>>>>>>>> reboot is required:
>>>>>>>>> +                       * 0 - Discrete GPU,
>>>>>>>>> +                       * 1 - Optimus/Hybrid,
>>>>>>>>
>>>>>>>> This feel like it should probably export using
>>>>>>>> /sys/class/firmware-attributes.  That's exactly how those types
>>>>>>>> of attributes work.
>>>>>>>>
>>>>>>>> As a bonus, software like fwupd 1.8.4 knows how to manipulate it
>>>>>>>> and you don't need special documentation.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>>     What: /sys/devices/platform/<platform>/dgpu_disable
>>>>>>>>>     Date:          Aug 2022
>>>>>>>>>     KernelVersion: 5.17
>>>>>>>>> diff --git a/drivers/platform/x86/asus-wmi.c
>>>>>>>>> b/drivers/platform/x86/asus-wmi.c
>>>>>>>>> index e2b51b5550e8..0421ffb81927 100644
>>>>>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>>>>>> @@ -230,6 +230,7 @@ struct asus_wmi {
>>>>>>>>>           bool egpu_enable_available;
>>>>>>>>>         bool dgpu_disable_available;
>>>>>>>>> +    bool gpu_mux_mode_available;
>>>>>>>>>           bool throttle_thermal_policy_available;
>>>>>>>>>         u8 throttle_thermal_policy_mode;
>>>>>>>>> @@ -668,6 +669,59 @@ static ssize_t egpu_enable_store(struct
>>>>>>>>> device *dev,
>>>>>>>>>     }
>>>>>>>>>     static DEVICE_ATTR_RW(egpu_enable);
>>>>>>>>>     +/* gpu mux switch
>>>>>>>>> *************************************************************/
>>>>>>>>> +static int gpu_mux_mode_check_present(struct asus_wmi *asus)
>>>>>>>>> +{
>>>>>>>>> +    asus->gpu_mux_mode_available =
>>>>>>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX);
>>>>>>>>> +
>>>>>>>>> +   return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static ssize_t gpu_mux_mode_show(struct device *dev,
>>>>>>>>> +                  struct device_attribute *attr, char *buf)
>>>>>>>>> +{
>>>>>>>>> +   struct asus_wmi *asus = dev_get_drvdata(dev);
>>>>>>>>> +   int result;
>>>>>>>>> +
>>>>>>>>> +   result = asus_wmi_get_devstate_simple(asus,
>>>>>>>>> ASUS_WMI_DEVID_GPU_MUX);
>>>>>>>>> +   if (result < 0)
>>>>>>>>> +       return result;
>>>>>>>>> +
>>>>>>>>> +   return sysfs_emit(buf, "%d\n", result);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static ssize_t gpu_mux_mode_store(struct device *dev,
>>>>>>>>> +                   struct device_attribute *attr,
>>>>>>>>> +                   const char *buf, size_t count)
>>>>>>>>> +{
>>>>>>>>> +   struct asus_wmi *asus = dev_get_drvdata(dev);
>>>>>>>>> +   int result, err;
>>>>>>>>> +   u32 optimus;
>>>>>>>>> +
>>>>>>>>> +   err = kstrtou32(buf, 10, &optimus);
>>>>>>>>> +   if (err)
>>>>>>>>> +       return err;
>>>>>>>>> +
>>>>>>>>> +   if (optimus > 1)
>>>>>>>>> +       return -EINVAL;
>>>>>>>>> +
>>>>>>>>> +   err = asus_wmi_set_devstate(ASUS_WMI_DEVID_GPU_MUX,
>>>>>>>>> optimus, &result);
>>>>>>>>> +   if (err) {
>>>>>>>>> +       dev_err(dev, "Failed to set GPU MUX mode: %d\n", err);
>>>>>>>>> +       return err;
>>>>>>>>> +   }
>>>>>>>>> +    /* !1 is considered a fail by ASUS */
>>>>>>>>> +    if (result != 1) {
>>>>>>>>> +        dev_warn(dev, "Failed to set GPU MUX mode (result):
>>>>>>>>> 0x%x\n", result);
>>>>>>>>> +       return -EIO;
>>>>>>>>> +   }
>>>>>>>>> +
>>>>>>>>> +   sysfs_notify(&asus->platform_device->dev.kobj, NULL,
>>>>>>>>> "gpu_mux_mode");
>>>>>>>>> +
>>>>>>>>> +   return count;
>>>>>>>>> +}
>>>>>>>>> +static DEVICE_ATTR_RW(gpu_mux_mode);
>>>>>>>>> +
>>>>>>>>>     /* Battery
>>>>>>>>> ********************************************************************/
>>>>>>>>>
>>>>>>>>>       /* The battery maximum charging percentage */
>>>>>>>>> @@ -3165,6 +3219,7 @@ static struct attribute
>>>>>>>>> *platform_attributes[] = {
>>>>>>>>>         &dev_attr_touchpad.attr,
>>>>>>>>>         &dev_attr_egpu_enable.attr,
>>>>>>>>>         &dev_attr_dgpu_disable.attr,
>>>>>>>>> +    &dev_attr_gpu_mux_mode.attr,
>>>>>>>>>         &dev_attr_lid_resume.attr,
>>>>>>>>>         &dev_attr_als_enable.attr,
>>>>>>>>>         &dev_attr_fan_boost_mode.attr,
>>>>>>>>> @@ -3195,6 +3250,8 @@ static umode_t
>>>>>>>>> asus_sysfs_is_visible(struct kobject *kobj,
>>>>>>>>>             ok = asus->egpu_enable_available;
>>>>>>>>>         else if (attr == &dev_attr_dgpu_disable.attr)
>>>>>>>>>             ok = asus->dgpu_disable_available;
>>>>>>>>> +    else if (attr == &dev_attr_gpu_mux_mode.attr)
>>>>>>>>> +        ok = asus->gpu_mux_mode_available;
>>>>>>>>>         else if (attr == &dev_attr_fan_boost_mode.attr)
>>>>>>>>>             ok = asus->fan_boost_mode_available;
>>>>>>>>>         else if (attr == &dev_attr_throttle_thermal_policy.attr)
>>>>>>>>> @@ -3464,6 +3521,10 @@ static int asus_wmi_add(struct
>>>>>>>>> platform_device *pdev)
>>>>>>>>>         if (err)
>>>>>>>>>             goto fail_dgpu_disable;
>>>>>>>>>     +    err = gpu_mux_mode_check_present(asus);
>>>>>>>>> +   if (err)
>>>>>>>>> +       goto fail_gpu_mux_mode;
>>>>>>>>> +
>>>>>>>>>         err = fan_boost_mode_check_present(asus);
>>>>>>>>>         if (err)
>>>>>>>>>             goto fail_fan_boost_mode;
>>>>>>>>> @@ -3578,6 +3639,7 @@ static int asus_wmi_add(struct
>>>>>>>>> platform_device *pdev)
>>>>>>>>>     fail_fan_boost_mode:
>>>>>>>>>     fail_egpu_enable:
>>>>>>>>>     fail_dgpu_disable:
>>>>>>>>> +fail_gpu_mux_mode:
>>>>>>>>>     fail_platform:
>>>>>>>>>     fail_panel_od:
>>>>>>>>>         kfree(asus);
>>>>>>>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>>> index a571b47ff362..c023332842a2 100644
>>>>>>>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>>>>>> @@ -98,6 +98,9 @@
>>>>>>>>>     /* dgpu on/off */
>>>>>>>>>     #define ASUS_WMI_DEVID_DGPU        0x00090020
>>>>>>>>>     +/* gpu mux switch, 0 = dGPU, 1 = Optimus */
>>>>>>>>> +#define ASUS_WMI_DEVID_GPU_MUX 0x00090016
>>>>>>>>> +
>>>>>>>>>     /* DSTS masks */
>>>>>>>>>     #define ASUS_WMI_DSTS_STATUS_BIT    0x00000001
>>>>>>>>>     #define ASUS_WMI_DSTS_UNKNOWN_BIT    0x00000002
>>>>>>>>
>>>>>>>
>>>>>>> You can see previous discussion here
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fplatform-driver-x86%2Fc3bb0989-78d9-c513-1669-75407b2acbac%40redhat.com%2F&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C92a9d94ff13d46f453b608da88cc2e2e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637972707813632096%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Lr6UVW5cxrkItsWMLtI2M7NcaqoEVDCubajSA75K%2FwI%3D&amp;reserved=0
>>>>>>>
>>>>>>>
>>>>>>> Below is Hans response verbatim:
>>>>>>>
>>>>>>>    > Yes it sounds like a BIOS setting is being toggled from within
>>>>>>>    > Linux, which would normally be done through the
>>>>>>>    > "firmware-attributes" class, but all existing
>>>>>>> "firmware-attributes"
>>>>>>>    > class drivers allow changing all BIOS setting not just a single
>>>>>>>    > setting, so using the  "firmware-attributes" class here is
>>>>>>> not really
>>>>>>>    > appropriate.
>>>>>>>
>>>>>>
>>>>>> Although the two consumers thus far (Lenovo and Dell) use WMI
>>>>>> interfaces to build and discover varieties of settings there is no
>>>>>> requirement for how the backend for firwmare-attributes works.
>>>>>>  You can just as well poulate a single attribute statically from
>>>>>> your driver.
>>>>>>
>>>>>> So I guess Hans and I disagree here.  I have a feeling that we
>>>>>> shouldn't be introducing custom ABI to userspace just because only
>>>>>> "one" setting is offered.  I anticipate that some day the DE's
>>>>>> will offer a GUI setting built on top of fwupd which is built on
>>>>>> top of firmware-attributes.
>>>>>>
>>>>>> If you *don't* populate a setting with firmware-attributes the
>>>>>> only way that users will discover such a setting is by installing
>>>>>> other custom userspace software that has the knowledge of it.
>>>>>>
>>>>>> At the end of the day it's up to Hans and Mark though, this is
>>>>>> just my 2c.
>>>>>
>>>>> Mario, thank you for your input here, it is much appreciated.
>>>>>
>>>>> As Luke mentioned in my quote using the firmware-attributes class
>>>>> for this really seems like overkill. As for discoverability, the
>>>>> firmware-attributes class only standardizes how to enum / change
>>>>> BIOS settings. The consumer of the API still must now the name of
>>>>> the setting which can/will be different per vendor.
>>>>>
>>>>> AFAIK fwupd only uses the firmware-attributes class to check for /
>>>>> disable some BIOS flashing protection. So having the GPU mux
>>>>> setting in the firmware-attributes class is not really relevant for
>>>>> fwupd.
>>>>>
>>>>> If in the future some generic tool which uses the
>>>>> firmware-attributes class to toggle GPU muxes is created
>>>>> (presumably with a lookup table for the exact setting's name under
>>>>> the firmare-attributes API) then we can always add
>>>>> firmware-attributes support for the GPU mux to asus-wmi at that
>>>>> point in time.
>>>>>
>>>>> I just don't think it is likely such a generic tool will happen
>>>>> (any time soon), so for now I still believe that using the
>>>>> firmware-attributes class for this is not necessary.
>>>>>
>>>>
>>>> Actually I've been actively working on that.  Take a look at fwupd
>>>> main (what will go into the next tagged 1.8.4 release).
>>>>
>>>> It's got support for "fwupdmgr get-bios-settings" and "fwupdmgr
>>>> set-bios-settings" which will follow the rules the kernel API uses.
>>>>
>>>> So I expect that if this attribute was implemented as I suggested
>>>> you could do:
>>>>
>>>> # fwupdmgr get-bios-settings
>>>>
>>>> and find the mux (including the possible values if it's declared a
>>>> kernel enumeration attribute and possible_values is populated).  If
>>>> you populate the optional description attribute in the kernel fwupd
>>>> will show you what your enumerated settings mean.
>>>>
>>>> followed by:
>>>>
>>>> # fwupdmgr set-bios-setting dGPUMux iGPU
>>>> or
>>>> # fwupdmgr set-bios-setting dGPUMux dGPU
>>>>
>>>> To set it.
>>>>
>>>> fwupd will prompt you to reboot the system after it's done changing
>>>> it as well.
>>>>
>>>> It's implemented such that GUI clients can use libfwupd just the
>>>> same, and I really think this increases discoverability of such a
>>>> setting.
>>>
>>> Interesting, I must admit that that makes your argument for using the
>>> firmware-attributes class stronger.
>>>
>>> But in the end it IMHO still feels wrong to add firmware-attribute
>>> support for just a single setting, rather then for something which
>>> actually exports all or most BIOS settings.
>>
>> OK thanks for your thoughts.
>>
>>>
>>> So for now I'm going to with this patch as is. If eventually it turns
>>> out that having this inside the firmware-attributes class would be
>>> really useful we can add it later, while keeping the sysfs attr for
>>> backward compat. My thinking being here that the code for the single
>>> sysfs attr is quite small, where as adding firmware-attributes class
>>> support will be more involved
>> At least looking at the context of that diff I see a whole bunch of
>> settings listed and this is "just one more". I noticed:  "eGPU", "lid
>> resume", "ALS enable", "fan boost mode".
>>
>> Maybe a later follow up should implement ALL of these as
>> firmware-attributes but keep compatibility for sysfs.
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>
>
> Hi Mario, Hans.
>
> Quite a bit for me to follow here. But going from the gist of it all I'm
> not opposed at all to implementing support for fwupd. I realise we
> already have a series of attributes under the wmi sysfs, and there will
> be more added - but in the first instances of this I (personally) wasn't
> aware of fwupd and who it was aiming to enable this, and secondly it was
> a very fast way to bring up support along with being easily discoverable
> with udev libs.
>
> Unfortunately it looks like it is going to be a large body of work to
> start doing fwupd support, and I'm not likely to have much time for it
> although I'd love to do it. I may well have a go with a single simple
> attribute and see how it works out, but man I wish I was getting paid
> for all this so I could justify the time cost.

I'm not sure how much effort it really will be compared to a "regular
attribute". It's just controlling where it gets put and what sysfs
files are exported.

I would think it was a subset of this kind of stuff:

Get the class:
ret = fw_attributes_class_get(&fw_attr_class);

Create the device:

device_create(fw_attr_class, NULL, MKDEV(0, 0),
NULL, "%s", "asus-wmi");

Create a kset:
kset_create_and_add("attributes", NULL,
&class_dev->kobj);

Create a group:
sysfs_create_group(kobj, &group);

Create sysfs files:
sysfs_create_file(..);
kobject_add(...)

Various cleanup stuff:
device_destroy()
fw_attributes_class_put()

think-lmi's implementation is easier to follow (tlmi_sysfs_init) for an
example.

>
> Mario, as I seem to be the single person adding support for various
> things with ASUS gaming laptops, please feel free to CC or email me (in
> addition to others) whenever required for the ASUS stuff. I tend to
> stick to the gaming laptops for now however (as I have two kinds I can
> test with).

It's too bad ASUS doesn't do any of this and only you do in your free time.

>
> Kind regards,
> Luke.
>
>

\
 
 \ /
  Last update: 2022-08-29 17:04    [W:0.068 / U:1.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site