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:4.093 / U:1.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site