Messages in this thread | | | Date | Mon, 29 Aug 2022 10:03:10 -0500 | Subject | Re: [PATCH] asus-wmi: Support the hardware GPU MUX on some laptops | From | "Limonciello, Mario" <> |
| |
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&data=05%7C01%7Cmario.limonciello%40amd.com%7C92a9d94ff13d46f453b608da88cc2e2e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637972707813632096%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Lr6UVW5cxrkItsWMLtI2M7NcaqoEVDCubajSA75K%2FwI%3D&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. > >
| |