| Message ID | 20210704222148.880848-3-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support for ASUS egpu, dpgu disable, panel overdrive | expand |
Hi I have added a couple comments inline. 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta: > In Windows the ASUS Armory Crate progrm can enable or disable the ^ "program" > dGPU via a WMI call. This functions much the same as various Linux > methods in software where the dGPU is removed from the device tree. > > However the WMI call saves the state of dGPU enabled or not and this I think "[...] the WMI call saves whether the dGPU is enabled or not, and [...]" might be better. Or "[...] the WMI call saves the state of the dGPU (enabled or not) and [...]". > then changes the dGPU visibility in Linux with no way for Linux > users to re-enable it. We expose the WMI method so users can see > and change the dGPU ACPI state. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > drivers/platform/x86/asus-wmi.c | 98 ++++++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 3 + > 2 files changed, 101 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 2468076d6cd8..8dc3f7ed021f 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -210,6 +210,9 @@ struct asus_wmi { > u8 fan_boost_mode_mask; > u8 fan_boost_mode; > > + bool dgpu_disable_available; > + u8 dgpu_disable_mode; > + > bool throttle_thermal_policy_available; > u8 throttle_thermal_policy_mode; > > @@ -427,6 +430,93 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus) > } > } > > +/* dGPU ********************************************************************/ > +static int dgpu_disable_check_present(struct asus_wmi *asus) > +{ > + u32 result; > + int err; > + > + asus->dgpu_disable_available = false; > + > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); > + if (err) { > + if (err == -ENODEV) > + return 0; > + return err; > + } > + > + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) > + asus->dgpu_disable_available = true; > + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; > + Aren't braces missing here? > + return 0; > +} > + > +static int dgpu_disable_write(struct asus_wmi *asus) > +{ > + int err; > + u8 value; > + u32 retval; > + > + value = asus->dgpu_disable_mode; > + > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); > + > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, > + "dgpu_disable"); A similar line with the exact same length in patch 3/3 is not broken in two. And shouldn't the notification be sent if the operation succeeded? > + > + if (err) { > + pr_warn("Failed to set dgpu disable: %d\n", err); > + return err; > + } > + > + if (retval > 1 || retval < 0) { > + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", > + retval); > + return -EIO; > + } > + > + return 0; > +} > + > +static ssize_t dgpu_disable_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct asus_wmi *asus = dev_get_drvdata(dev); > + u8 mode = asus->dgpu_disable_mode; > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); You could use `sysfs_emit()`. > +} > + > +static ssize_t dgpu_disable_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int result; > + u8 disable; > + struct asus_wmi *asus = dev_get_drvdata(dev); > + > + result = kstrtou8(buf, 10, &disable); You could use `kstrtobool()`. I think that would be better since it accepts 'y', 'n', etc. in addition to 0 and 1. > + if (result < 0) > + return result; > + > + if (disable > 1 || disable < 0) > + return -EINVAL; > + > + asus->dgpu_disable_mode = disable; > + /* > + * The ACPI call used does not save the mode unless the call is run twice. > + * Once to disable, then once to check status and save - this is two code > + * paths in the method in the ACPI dumps. > + */ > + dgpu_disable_write(asus); > + dgpu_disable_write(asus); Is there any reason the potential error codes are not returned? > + > + return count; > +} > + > +static DEVICE_ATTR_RW(dgpu_disable); > + > /* Battery ********************************************************************/ > > /* The battery maximum charging percentage */ > @@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = { > &dev_attr_camera.attr, > &dev_attr_cardr.attr, > &dev_attr_touchpad.attr, > + &dev_attr_dgpu_disable.attr, > &dev_attr_lid_resume.attr, > &dev_attr_als_enable.attr, > &dev_attr_fan_boost_mode.attr, > @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, > devid = ASUS_WMI_DEVID_LID_RESUME; > else if (attr == &dev_attr_als_enable.attr) > devid = ASUS_WMI_DEVID_ALS_ENABLE; > + else if (attr == &dev_attr_dgpu_disable.attr) > + ok = asus->dgpu_disable_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) > @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev) > if (err) > goto fail_platform; > > + err = dgpu_disable_check_present(asus); > + if (err) > + goto fail_dgpu_disable; > + Should this really be considered a "fatal" error? > err = fan_boost_mode_check_present(asus); > if (err) > goto fail_fan_boost_mode; > @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev) > fail_sysfs: > fail_throttle_thermal_policy: > fail_fan_boost_mode: > +fail_dgpu_disable: > 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 428aea701c7b..a528f9d0e4b7 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -90,6 +90,9 @@ > /* Keyboard dock */ > #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 > > +/* dgpu on/off */ > +#define ASUS_WMI_DEVID_DGPU 0x00090020 > + > /* DSTS masks */ > #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 > #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 > -- > 2.31.1 Regards, Barnabás Pőcze
Hi, Some review remarks inline (mostly just echo-ing what Barnabás already said): On 7/5/21 12:21 AM, Luke D. Jones wrote: > In Windows the ASUS Armory Crate progrm can enable or disable the > dGPU via a WMI call. This functions much the same as various Linux > methods in software where the dGPU is removed from the device tree. > > However the WMI call saves the state of dGPU enabled or not and this > then changes the dGPU visibility in Linux with no way for Linux > users to re-enable it. We expose the WMI method so users can see > and change the dGPU ACPI state. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > drivers/platform/x86/asus-wmi.c | 98 ++++++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 3 + > 2 files changed, 101 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 2468076d6cd8..8dc3f7ed021f 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -210,6 +210,9 @@ struct asus_wmi { > u8 fan_boost_mode_mask; > u8 fan_boost_mode; > > + bool dgpu_disable_available; > + u8 dgpu_disable_mode; > + > bool throttle_thermal_policy_available; > u8 throttle_thermal_policy_mode; > > @@ -427,6 +430,93 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus) > } > } > > +/* dGPU ********************************************************************/ > +static int dgpu_disable_check_present(struct asus_wmi *asus) > +{ > + u32 result; > + int err; > + > + asus->dgpu_disable_available = false; > + > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); > + if (err) { > + if (err == -ENODEV) > + return 0; > + return err; > + } > + > + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) > + asus->dgpu_disable_available = true; > + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; Missing {} > + > + return 0; > +} > + > +static int dgpu_disable_write(struct asus_wmi *asus) > +{ > + int err; > + u8 value; > + u32 retval; > + > + value = asus->dgpu_disable_mode; > + > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); > + > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, > + "dgpu_disable"); Make this one line please. > + > + if (err) { > + pr_warn("Failed to set dgpu disable: %d\n", err); > + return err; > + } > + > + if (retval > 1 || retval < 0) { > + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", > + retval); Make this one line please. > + return -EIO; > + } > + > + return 0; > +} > + > +static ssize_t dgpu_disable_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct asus_wmi *asus = dev_get_drvdata(dev); > + u8 mode = asus->dgpu_disable_mode; > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); sysfs_emit() please. > +} > + > +static ssize_t dgpu_disable_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int result; > + u8 disable; > + struct asus_wmi *asus = dev_get_drvdata(dev); > + > + result = kstrtou8(buf, 10, &disable); > + if (result < 0) > + return result; > + > + if (disable > 1 || disable < 0) > + return -EINVAL; Drop the "disable < 0" check please. > + > + asus->dgpu_disable_mode = disable; > + /* > + * The ACPI call used does not save the mode unless the call is run twice. > + * Once to disable, then once to check status and save - this is two code > + * paths in the method in the ACPI dumps. > + */ > + dgpu_disable_write(asus); > + dgpu_disable_write(asus); > + > + return count; > +} > + > +static DEVICE_ATTR_RW(dgpu_disable); > + > /* Battery ********************************************************************/ > > /* The battery maximum charging percentage */ > @@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = { > &dev_attr_camera.attr, > &dev_attr_cardr.attr, > &dev_attr_touchpad.attr, > + &dev_attr_dgpu_disable.attr, > &dev_attr_lid_resume.attr, > &dev_attr_als_enable.attr, > &dev_attr_fan_boost_mode.attr, > @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, > devid = ASUS_WMI_DEVID_LID_RESUME; > else if (attr == &dev_attr_als_enable.attr) > devid = ASUS_WMI_DEVID_ALS_ENABLE; > + else if (attr == &dev_attr_dgpu_disable.attr) > + ok = asus->dgpu_disable_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) > @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev) > if (err) > goto fail_platform; > > + err = dgpu_disable_check_present(asus); > + if (err) > + goto fail_dgpu_disable; > + > err = fan_boost_mode_check_present(asus); > if (err) > goto fail_fan_boost_mode; > @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev) > fail_sysfs: > fail_throttle_thermal_policy: > fail_fan_boost_mode: > +fail_dgpu_disable: > 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 428aea701c7b..a528f9d0e4b7 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -90,6 +90,9 @@ > /* Keyboard dock */ > #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 > > +/* dgpu on/off */ > +#define ASUS_WMI_DEVID_DGPU 0x00090020 > + > /* DSTS masks */ > #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 > #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 > Otherwise this looks good to me. Regards, Hans
Hi, Barnabás made some good points which I missed. See me reply inline. On 7/5/21 2:47 AM, Barnabás Pőcze wrote: > Hi > > I have added a couple comments inline. > > > 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta: > <snip> >> +static ssize_t dgpu_disable_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int result; >> + u8 disable; >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + >> + result = kstrtou8(buf, 10, &disable); > > You could use `kstrtobool()`. I think that would be better since it accepts > 'y', 'n', etc. in addition to 0 and 1. Good point and the same applies to patch 1/3. >> + if (result < 0) >> + return result; >> + >> + if (disable > 1 || disable < 0) >> + return -EINVAL; >> + >> + asus->dgpu_disable_mode = disable; >> + /* >> + * The ACPI call used does not save the mode unless the call is run twice. >> + * Once to disable, then once to check status and save - this is two code >> + * paths in the method in the ACPI dumps. >> + */ >> + dgpu_disable_write(asus); >> + dgpu_disable_write(asus); > > Is there any reason the potential error codes are not returned? Good question. <snip> >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev) >> if (err) >> goto fail_platform; >> >> + err = dgpu_disable_check_present(asus); >> + if (err) >> + goto fail_dgpu_disable; >> + > > Should this really be considered a "fatal" error? Well dgpu_disable_check_present() does already contain: if (err == -ENODEV) return 0; IOW it only returns an error on unexpected errors and asus_wmi_add() already contains a couple of other foo_present() checks which are dealt with in the same way, so this is consistent with that and being consistent is good, so I think this is fine. Regards, Hans
Hi 2021. július 6., kedd 12:17 keltezéssel, Hans de Goede írta: > [...] > >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev) > >> if (err) > >> goto fail_platform; > >> > >> + err = dgpu_disable_check_present(asus); > >> + if (err) > >> + goto fail_dgpu_disable; > >> + > > > > Should this really be considered a "fatal" error? > > Well dgpu_disable_check_present() does already contain: > > if (err == -ENODEV) > return 0; > > IOW it only returns an error on unexpected errors and asus_wmi_add() > already contains a couple of other foo_present() checks which are > dealt with in the same way, so this is consistent with that and > being consistent is good, so I think this is fine. > Indeed, that's right, I missed that. I am still unsure whether any error should cause it to fail to load. But I guess if there is precedent for that in the module, then consistency is probably better. Regards, Barnabás Pőcze
Thank you for the insightful feedback. On Mon, Jul 5 2021 at 00:47:31 +0000, Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > I have added a couple comments inline. > > > 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta: > >> In Windows the ASUS Armory Crate progrm can enable or disable the > ^ > "program" > My "a" key is a little hard to press sometimes :( > >> dGPU via a WMI call. This functions much the same as various Linux >> methods in software where the dGPU is removed from the device tree. >> >> However the WMI call saves the state of dGPU enabled or not and this > > I think "[...] the WMI call saves whether the dGPU is enabled or not, > and [...]" > might be better. > Or "[...] the WMI call saves the state of the dGPU (enabled or not) > and [...]". > I've used the second option, thanks for pointing this out. > >> then changes the dGPU visibility in Linux with no way for Linux >> users to re-enable it. We expose the WMI method so users can see >> and change the dGPU ACPI state. >> >> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> --- >> drivers/platform/x86/asus-wmi.c | 98 >> ++++++++++++++++++++++ >> include/linux/platform_data/x86/asus-wmi.h | 3 + >> 2 files changed, 101 insertions(+) >> >> diff --git a/drivers/platform/x86/asus-wmi.c >> b/drivers/platform/x86/asus-wmi.c >> index 2468076d6cd8..8dc3f7ed021f 100644 >> --- a/drivers/platform/x86/asus-wmi.c >> +++ b/drivers/platform/x86/asus-wmi.c >> @@ -210,6 +210,9 @@ struct asus_wmi { >> u8 fan_boost_mode_mask; >> u8 fan_boost_mode; >> >> + bool dgpu_disable_available; >> + u8 dgpu_disable_mode; >> + >> bool throttle_thermal_policy_available; >> u8 throttle_thermal_policy_mode; >> >> @@ -427,6 +430,93 @@ static void >> lid_flip_tablet_mode_get_state(struct asus_wmi *asus) >> } >> } >> >> +/* dGPU >> ********************************************************************/ >> +static int dgpu_disable_check_present(struct asus_wmi *asus) >> +{ >> + u32 result; >> + int err; >> + >> + asus->dgpu_disable_available = false; >> + >> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); >> + if (err) { >> + if (err == -ENODEV) >> + return 0; >> + return err; >> + } >> + >> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) >> + asus->dgpu_disable_available = true; >> + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; >> + > > Aren't braces missing here? > Yep. Fixed. > >> + return 0; >> +} >> + >> +static int dgpu_disable_write(struct asus_wmi *asus) >> +{ >> + int err; >> + u8 value; >> + u32 retval; >> + >> + value = asus->dgpu_disable_mode; >> + >> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); >> + >> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, >> + "dgpu_disable"); > > A similar line with the exact same length in patch 3/3 is not broken > in two. > And shouldn't the notification be sent if the operation succeeded? > Fixed. I moved the sysfs_notify down below the error returns. > >> + >> + if (err) { >> + pr_warn("Failed to set dgpu disable: %d\n", err); >> + return err; >> + } >> + >> + if (retval > 1 || retval < 0) { >> + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", >> + retval); >> + return -EIO; >> + } >> + >> + return 0; >> +} >> + >> +static ssize_t dgpu_disable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + u8 mode = asus->dgpu_disable_mode; >> + >> + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); > > You could use `sysfs_emit()`. > Thanks. Many things like this I'm actually unaware of. Most of these patches are done by reading existing code. > >> +} >> + >> +static ssize_t dgpu_disable_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int result; >> + u8 disable; >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + >> + result = kstrtou8(buf, 10, &disable); > > You could use `kstrtobool()`. I think that would be better since it > accepts > 'y', 'n', etc. in addition to 0 and 1. > Thanks! Wasn't aware of that. And since the setting for all 3 patches can only ever be 0/1 I've changed to use a bool instead of u8 > >> + if (result < 0) >> + return result; >> + >> + if (disable > 1 || disable < 0) >> + return -EINVAL; >> + >> + asus->dgpu_disable_mode = disable; >> + /* >> + * The ACPI call used does not save the mode unless the call is >> run twice. >> + * Once to disable, then once to check status and save - this is >> two code >> + * paths in the method in the ACPI dumps. >> + */ >> + dgpu_disable_write(asus); >> + dgpu_disable_write(asus); > > Is there any reason the potential error codes are not returned? > No I've fixed now. Guess I missed it. > >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(dgpu_disable); >> + >> /* Battery >> ********************************************************************/ >> >> /* The battery maximum charging percentage */ >> @@ -2412,6 +2502,7 @@ static struct attribute >> *platform_attributes[] = { >> &dev_attr_camera.attr, >> &dev_attr_cardr.attr, >> &dev_attr_touchpad.attr, >> + &dev_attr_dgpu_disable.attr, >> &dev_attr_lid_resume.attr, >> &dev_attr_als_enable.attr, >> &dev_attr_fan_boost_mode.attr, >> @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct >> kobject *kobj, >> devid = ASUS_WMI_DEVID_LID_RESUME; >> else if (attr == &dev_attr_als_enable.attr) >> devid = ASUS_WMI_DEVID_ALS_ENABLE; >> + else if (attr == &dev_attr_dgpu_disable.attr) >> + ok = asus->dgpu_disable_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) >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> if (err) >> goto fail_platform; >> >> + err = dgpu_disable_check_present(asus); >> + if (err) >> + goto fail_dgpu_disable; >> + > > Should this really be considered a "fatal" error? > I was modelling this on fail_fan_boost_mode and fail_throttle_thermal_policy since a laptop can't have both of those which indicates that a failed one is fine, it seemed appropriate to follow the same behaviour here > >> err = fan_boost_mode_check_present(asus); >> if (err) >> goto fail_fan_boost_mode; >> @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> fail_sysfs: >> fail_throttle_thermal_policy: >> fail_fan_boost_mode: >> +fail_dgpu_disable: >> 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 428aea701c7b..a528f9d0e4b7 100644 >> --- a/include/linux/platform_data/x86/asus-wmi.h >> +++ b/include/linux/platform_data/x86/asus-wmi.h >> @@ -90,6 +90,9 @@ >> /* Keyboard dock */ >> #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 >> >> +/* dgpu on/off */ >> +#define ASUS_WMI_DEVID_DGPU 0x00090020 >> + >> /* DSTS masks */ >> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 >> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 >> -- >> 2.31.1 > > > Regards, > Barnabás Pőcze Many thanks for your feedback again. Luke.
Thanks Hans, all feedback applied. On Tue, Jul 6 2021 at 12:10:45 +0200, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > Some review remarks inline (mostly just echo-ing what Barnabás > already said): > > On 7/5/21 12:21 AM, Luke D. Jones wrote: >> In Windows the ASUS Armory Crate progrm can enable or disable the >> dGPU via a WMI call. This functions much the same as various Linux >> methods in software where the dGPU is removed from the device tree. >> >> However the WMI call saves the state of dGPU enabled or not and this >> then changes the dGPU visibility in Linux with no way for Linux >> users to re-enable it. We expose the WMI method so users can see >> and change the dGPU ACPI state. >> >> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> --- >> drivers/platform/x86/asus-wmi.c | 98 >> ++++++++++++++++++++++ >> include/linux/platform_data/x86/asus-wmi.h | 3 + >> 2 files changed, 101 insertions(+) >> >> diff --git a/drivers/platform/x86/asus-wmi.c >> b/drivers/platform/x86/asus-wmi.c >> index 2468076d6cd8..8dc3f7ed021f 100644 >> --- a/drivers/platform/x86/asus-wmi.c >> +++ b/drivers/platform/x86/asus-wmi.c >> @@ -210,6 +210,9 @@ struct asus_wmi { >> u8 fan_boost_mode_mask; >> u8 fan_boost_mode; >> >> + bool dgpu_disable_available; >> + u8 dgpu_disable_mode; >> + >> bool throttle_thermal_policy_available; >> u8 throttle_thermal_policy_mode; >> >> @@ -427,6 +430,93 @@ static void >> lid_flip_tablet_mode_get_state(struct asus_wmi *asus) >> } >> } >> >> +/* dGPU >> ********************************************************************/ >> +static int dgpu_disable_check_present(struct asus_wmi *asus) >> +{ >> + u32 result; >> + int err; >> + >> + asus->dgpu_disable_available = false; >> + >> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); >> + if (err) { >> + if (err == -ENODEV) >> + return 0; >> + return err; >> + } >> + >> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) >> + asus->dgpu_disable_available = true; >> + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; > > Missing {} > >> + >> + return 0; >> +} >> + >> +static int dgpu_disable_write(struct asus_wmi *asus) >> +{ >> + int err; >> + u8 value; >> + u32 retval; >> + >> + value = asus->dgpu_disable_mode; >> + >> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); >> + >> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, >> + "dgpu_disable"); > > Make this one line please. > >> + >> + if (err) { >> + pr_warn("Failed to set dgpu disable: %d\n", err); >> + return err; >> + } >> + >> + if (retval > 1 || retval < 0) { >> + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", >> + retval); > > Make this one line please. > >> + return -EIO; >> + } >> + >> + return 0; >> +} >> + >> +static ssize_t dgpu_disable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + u8 mode = asus->dgpu_disable_mode; >> + >> + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); > > sysfs_emit() please. > >> +} >> + >> +static ssize_t dgpu_disable_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int result; >> + u8 disable; >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + >> + result = kstrtou8(buf, 10, &disable); >> + if (result < 0) >> + return result; >> + >> + if (disable > 1 || disable < 0) >> + return -EINVAL; > > Drop the "disable < 0" check please. > >> + >> + asus->dgpu_disable_mode = disable; >> + /* >> + * The ACPI call used does not save the mode unless the call is >> run twice. >> + * Once to disable, then once to check status and save - this is >> two code >> + * paths in the method in the ACPI dumps. >> + */ >> + dgpu_disable_write(asus); >> + dgpu_disable_write(asus); >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(dgpu_disable); >> + >> /* Battery >> ********************************************************************/ >> >> /* The battery maximum charging percentage */ >> @@ -2412,6 +2502,7 @@ static struct attribute >> *platform_attributes[] = { >> &dev_attr_camera.attr, >> &dev_attr_cardr.attr, >> &dev_attr_touchpad.attr, >> + &dev_attr_dgpu_disable.attr, >> &dev_attr_lid_resume.attr, >> &dev_attr_als_enable.attr, >> &dev_attr_fan_boost_mode.attr, >> @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct >> kobject *kobj, >> devid = ASUS_WMI_DEVID_LID_RESUME; >> else if (attr == &dev_attr_als_enable.attr) >> devid = ASUS_WMI_DEVID_ALS_ENABLE; >> + else if (attr == &dev_attr_dgpu_disable.attr) >> + ok = asus->dgpu_disable_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) >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> if (err) >> goto fail_platform; >> >> + err = dgpu_disable_check_present(asus); >> + if (err) >> + goto fail_dgpu_disable; >> + >> err = fan_boost_mode_check_present(asus); >> if (err) >> goto fail_fan_boost_mode; >> @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> fail_sysfs: >> fail_throttle_thermal_policy: >> fail_fan_boost_mode: >> +fail_dgpu_disable: >> 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 428aea701c7b..a528f9d0e4b7 100644 >> --- a/include/linux/platform_data/x86/asus-wmi.h >> +++ b/include/linux/platform_data/x86/asus-wmi.h >> @@ -90,6 +90,9 @@ >> /* Keyboard dock */ >> #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 >> >> +/* dgpu on/off */ >> +#define ASUS_WMI_DEVID_DGPU 0x00090020 >> + >> /* DSTS masks */ >> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 >> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 >> > > Otherwise this looks good to me. > > Regards, > > Hans >
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 2468076d6cd8..8dc3f7ed021f 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -210,6 +210,9 @@ struct asus_wmi { u8 fan_boost_mode_mask; u8 fan_boost_mode; + bool dgpu_disable_available; + u8 dgpu_disable_mode; + bool throttle_thermal_policy_available; u8 throttle_thermal_policy_mode; @@ -427,6 +430,93 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus) } } +/* dGPU ********************************************************************/ +static int dgpu_disable_check_present(struct asus_wmi *asus) +{ + u32 result; + int err; + + asus->dgpu_disable_available = false; + + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); + if (err) { + if (err == -ENODEV) + return 0; + return err; + } + + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) + asus->dgpu_disable_available = true; + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; + + return 0; +} + +static int dgpu_disable_write(struct asus_wmi *asus) +{ + int err; + u8 value; + u32 retval; + + value = asus->dgpu_disable_mode; + + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); + + sysfs_notify(&asus->platform_device->dev.kobj, NULL, + "dgpu_disable"); + + if (err) { + pr_warn("Failed to set dgpu disable: %d\n", err); + return err; + } + + if (retval > 1 || retval < 0) { + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", + retval); + return -EIO; + } + + return 0; +} + +static ssize_t dgpu_disable_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct asus_wmi *asus = dev_get_drvdata(dev); + u8 mode = asus->dgpu_disable_mode; + + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); +} + +static ssize_t dgpu_disable_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int result; + u8 disable; + struct asus_wmi *asus = dev_get_drvdata(dev); + + result = kstrtou8(buf, 10, &disable); + if (result < 0) + return result; + + if (disable > 1 || disable < 0) + return -EINVAL; + + asus->dgpu_disable_mode = disable; + /* + * The ACPI call used does not save the mode unless the call is run twice. + * Once to disable, then once to check status and save - this is two code + * paths in the method in the ACPI dumps. + */ + dgpu_disable_write(asus); + dgpu_disable_write(asus); + + return count; +} + +static DEVICE_ATTR_RW(dgpu_disable); + /* Battery ********************************************************************/ /* The battery maximum charging percentage */ @@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = { &dev_attr_camera.attr, &dev_attr_cardr.attr, &dev_attr_touchpad.attr, + &dev_attr_dgpu_disable.attr, &dev_attr_lid_resume.attr, &dev_attr_als_enable.attr, &dev_attr_fan_boost_mode.attr, @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, devid = ASUS_WMI_DEVID_LID_RESUME; else if (attr == &dev_attr_als_enable.attr) devid = ASUS_WMI_DEVID_ALS_ENABLE; + else if (attr == &dev_attr_dgpu_disable.attr) + ok = asus->dgpu_disable_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) @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev) if (err) goto fail_platform; + err = dgpu_disable_check_present(asus); + if (err) + goto fail_dgpu_disable; + err = fan_boost_mode_check_present(asus); if (err) goto fail_fan_boost_mode; @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev) fail_sysfs: fail_throttle_thermal_policy: fail_fan_boost_mode: +fail_dgpu_disable: 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 428aea701c7b..a528f9d0e4b7 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -90,6 +90,9 @@ /* Keyboard dock */ #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 +/* dgpu on/off */ +#define ASUS_WMI_DEVID_DGPU 0x00090020 + /* DSTS masks */ #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002
In Windows the ASUS Armory Crate progrm can enable or disable the dGPU via a WMI call. This functions much the same as various Linux methods in software where the dGPU is removed from the device tree. However the WMI call saves the state of dGPU enabled or not and this then changes the dGPU visibility in Linux with no way for Linux users to re-enable it. We expose the WMI method so users can see and change the dGPU ACPI state. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 98 ++++++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 3 + 2 files changed, 101 insertions(+)