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 <[email protected]>
---
.../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" <[email protected]>
+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,
+
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
--
2.37.1
On Sat, Aug 13, 2022 at 09:26:24PM +1200, 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.
Could you please integrate this with the framework provided by:
include/linux/vga_switcheroo.h
drivers/gpu/vga/vga_switcheroo.c
vga_switcheroo will then automatically expose a sysfs interface.
Find existing vga_switcheroo mux drivers in the tree like this,
they may serve as a template:
git grep "struct vga_switcheroo_handler" -- :/
Thanks,
Lukas
Hi Lukas,
On Sat, 2022-08-13 at 12:55 +0200, Lukas Wunner wrote:
> On Sat, Aug 13, 2022 at 09:26:24PM +1200, 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.
>
> Could you please integrate this with the framework provided by:
>
> include/linux/vga_switcheroo.h
> drivers/gpu/vga/vga_switcheroo.c
>
> vga_switcheroo will then automatically expose a sysfs interface.
I did investigate this first before submitting. The way asus does it is
not standard at all. On switch you must reboot, and the change isn't
reflected by the ACPI get method until reboot. It's very reflective of
how they used dgpu_disable to work around windows issues that we do in
Linux by removing the device from the device tree.
The key thing is a reboot is required. This is not done on-the-fly. I
have a two year old GX502 which has the same method as exposed here,
and also a 2022 TUF laptop with same method. My understanding of this
pariicular method is that it isn't the same one as what Nvidia is
advertising, and ASUS is perhaps misadvertising it - the suspision is
raised by the fact that my GX502 machine predates what Nvidia is
advertising.
Kind regards,
Luke.
On Sun, Aug 14, 2022 at 09:09:43PM +1200, Luke Jones wrote:
> On Sat, 2022-08-13 at 12:55 +0200, Lukas Wunner wrote:
> > On Sat, Aug 13, 2022 at 09:26:24PM +1200, 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.
> >
> > Could you please integrate this with the framework provided by:
> >
> > include/linux/vga_switcheroo.h
> > drivers/gpu/vga/vga_switcheroo.c
> >
> > vga_switcheroo will then automatically expose a sysfs interface.
>
> I did investigate this first before submitting. The way asus does it is
> not standard at all. On switch you must reboot, and the change isn't
> reflected by the ACPI get method until reboot. It's very reflective of
> how they used dgpu_disable to work around windows issues that we do in
> Linux by removing the device from the device tree.
>
> The key thing is a reboot is required. This is not done on-the-fly. I
> have a two year old GX502 which has the same method as exposed here,
> and also a 2022 TUF laptop with same method. My understanding of this
> pariicular method is that it isn't the same one as what Nvidia is
> advertising, and ASUS is perhaps misadvertising it - the suspision is
> raised by the fact that my GX502 machine predates what Nvidia is
> advertising.
I see, thanks for the explanation. You may want to add that
background information to the commit message if/when respinning.
Indeed vga_switcheroo facilitates GPU switching at runtime,
not upon next reboot.
Thanks,
Lukas
On Sun, Aug 14 2022 at 12:42:10 +0200, Lukas Wunner <[email protected]>
wrote:
> On Sun, Aug 14, 2022 at 09:09:43PM +1200, Luke Jones wrote:
>> On Sat, 2022-08-13 at 12:55 +0200, Lukas Wunner wrote:
>> > On Sat, Aug 13, 2022 at 09:26:24PM +1200, 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.
>> >
>> > Could you please integrate this with the framework provided by:
>> >
>> > include/linux/vga_switcheroo.h
>> > drivers/gpu/vga/vga_switcheroo.c
>> >
>> > vga_switcheroo will then automatically expose a sysfs interface.
>>
>> I did investigate this first before submitting. The way asus does
>> it is
>> not standard at all. On switch you must reboot, and the change isn't
>> reflected by the ACPI get method until reboot. It's very reflective
>> of
>> how they used dgpu_disable to work around windows issues that we do
>> in
>> Linux by removing the device from the device tree.
>>
>> The key thing is a reboot is required. This is not done on-the-fly.
>> I
>> have a two year old GX502 which has the same method as exposed here,
>> and also a 2022 TUF laptop with same method. My understanding of
>> this
>> pariicular method is that it isn't the same one as what Nvidia is
>> advertising, and ASUS is perhaps misadvertising it - the suspision
>> is
>> raised by the fact that my GX502 machine predates what Nvidia is
>> advertising.
>
> I see, thanks for the explanation. You may want to add that
> background information to the commit message if/when respinning.
> Indeed vga_switcheroo facilitates GPU switching at runtime,
> not upon next reboot.
>
> Thanks,
>
> Lukas
Good point, yes I will do that after reviews.
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 <[email protected]>
> ---
> .../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" <[email protected]>
> +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
Hi Mario,
On Mon, Aug 15 2022 at 23:16:12 -0500, Mario Limonciello
<[email protected]> 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 <[email protected]>
>> ---
>> .../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" <[email protected]>
>> +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://lore.kernel.org/platform-driver-x86/[email protected]/
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.
Kind regards,
Luke.
On 8/21/2022 18:07, Luke Jones wrote:
> Hi Mario,
>
> On Mon, Aug 15 2022 at 23:16:12 -0500, Mario Limonciello
> <[email protected]> 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 <[email protected]>
>>> ---
>>> .../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" <[email protected]>
>>> +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%7C3df656b0acd34b793bb908da83c9e8f4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967200474348826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MsHEOH2aUUYGBhOBhVukN6DxWeBE16qz0nw%2BJryfM7M%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.
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> .../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" <[email protected]>
>>>> +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%7C3df656b0acd34b793bb908da83c9e8f4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967200474348826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MsHEOH2aUUYGBhOBhVukN6DxWeBE16qz0nw%2BJryfM7M%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.
Regards,
Hans
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 <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> .../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" <[email protected]>
>>>>>>> +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%7Ceebec0ca0ab74f8babf908da85d10582%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969430056811815%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kFb75Jhr77XZScoegTh5FyJAnnEXm6cInIoxKdu7rhM%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
>
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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> .../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" <[email protected]>
>>>>> +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%7C54f5129bf4ad437bd11108da85cde220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969416575174610%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GqFSQY5%2BVJCX1Wytp5mGq54rcx6ks9sKrGdGFf2WtNI%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.
> Regards,
>
> Hans
>
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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> .../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" <[email protected]>
>>>>>> +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%7C54f5129bf4ad437bd11108da85cde220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969416575174610%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GqFSQY5%2BVJCX1Wytp5mGq54rcx6ks9sKrGdGFf2WtNI%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.
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.
Regards,
Hans
Hi,
On 8/24/22 15:09, Mario Limonciello 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 <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> .../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" <[email protected]>
>>>>>>>> +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%7Ceebec0ca0ab74f8babf908da85d10582%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969430056811815%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kFb75Jhr77XZScoegTh5FyJAnnEXm6cInIoxKdu7rhM%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.
Yes that might be interesting, although most of these do not require a reboot AFAIK, still
exporting them in some way where they are easier to discover/enumerate for userspace
might be useful.
Regards,
Hans
Hi,
On 8/13/22 11: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 <[email protected]>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note there were several whitespace/indentation issues with this
patch. The documentation patch was using spaces instead of tabs
and the .c changes where using 3 space indents instead of tabs
in some places. I've fixed this up while merging this time around,
but for future submissions please ensure that you get the indentation
right and that you only use tabs and not spaces.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
> .../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" <[email protected]>
> +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,
> +
> 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
On 8/24/22 08:49, Hans de Goede wrote:
> Hi,
>
> On 8/24/22 15:09, Mario Limonciello 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 <[email protected]> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../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" <[email protected]>
>>>>>>>>> +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%7Cdfd7f0fd73f4438aa06108da85d787c7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969458019501480%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1dknF207zR%2BTQz96qLaWFOhrYsL2IEYCABDuIAuhuKc%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.
>
> Yes that might be interesting, although most of these do not require a reboot AFAIK, still
> exporting them in some way where they are easier to discover/enumerate for userspace
> might be useful.
There's an assumption in fwupd right now that it doesn't re-read the
pending_reboot attribute that I'll fix.
It's up to the drivers to set that attribute if the system needs a
reboot though. I remember when the Dell driver was implemented that
some attributes don't need a reboot as well (particularly BIOS password
IIRC).
>
> Regards,
>
> Hans
>
On Wed, Aug 24 2022 at 08:09:08 -0500, Mario Limonciello
<[email protected]> 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
>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> .../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" <[email protected]>
>>>>>>>> +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%7Ceebec0ca0ab74f8babf908da85d10582%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969430056811815%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kFb75Jhr77XZScoegTh5FyJAnnEXm6cInIoxKdu7rhM%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.
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).
Kind regards,
Luke.
On 8/28/2022 03:05, Luke Jones wrote:
>
>
> On Wed, Aug 24 2022 at 08:09:08 -0500, Mario Limonciello
> <[email protected]> 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
>>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../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" <[email protected]>
>>>>>>>>> +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.
>
>