Messages in this thread | | | Date | Sat, 17 Jul 2021 11:12:31 +1200 | From | Luke Jones <> | Subject | Re: [PATCH 2/3] asus-wmi: Add dgpu disable method |
| |
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 >
| |