[PATCH 1/4] asus-wmi: Add panel overdrive functionality

[PATCH 1/4] asus-wmi: Add panel overdrive functionality

[Luke D. Jones]

Some ASUS ROG laptops have the ability to drive the display panel
a higher rate to eliminate or reduce ghosting.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 91 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 2 files changed, 92 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ebaeb7bb80f5..cd881443bc2f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -216,6 +216,9 @@ struct asus_wmi {
 	// The RSOC controls the maximum charging percentage.
 	bool battery_rsoc_available;
 
+	bool panel_overdrive_available;
+	bool panel_overdrive;
+
 	struct hotplug_slot hotplug_slot;
 	struct mutex hotplug_lock;
 	struct mutex wmi_lock;
@@ -1221,6 +1224,86 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
 	return result;
 }
 
+/* Panel Overdrive ************************************************************/
+static int panel_od_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->panel_overdrive_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_PANEL_OD, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+
+	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+		asus->panel_overdrive_available = true;
+		asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;
+	}
+
+	return 0;
+}
+
+static int panel_od_write(struct asus_wmi *asus)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->panel_overdrive;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, value, &retval);
+
+	if (err) {
+		pr_warn("Failed to set panel overdrive: %d\n", err);
+		return err;
+	}
+
+	if (retval > 1 || retval < 0) {
+		pr_warn("Failed to set panel overdrive (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "panel_od");
+
+	return 0;
+}
+
+static ssize_t panel_od_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	bool mode = asus->panel_overdrive;
+
+	return sysfs_emit(buf, "%d\n", mode);
+}
+
+static ssize_t panel_od_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	bool overdrive;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &overdrive);
+	if (result == -EINVAL)
+		return result;
+
+	asus->panel_overdrive = overdrive;
+	result = panel_od_write(asus);
+
+	if (result != 0)
+		return result;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(panel_od);
+
 /* Quirks *********************************************************************/
 
 static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
@@ -2332,6 +2415,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
 	&dev_attr_throttle_thermal_policy.attr,
+	&dev_attr_panel_od.attr,
 	NULL
 };
 
@@ -2357,6 +2441,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
 		ok = asus->throttle_thermal_policy_available;
+	else if (attr == &dev_attr_panel_od.attr)
+		ok = asus->panel_overdrive_available;
 
 	if (devid != -1)
 		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -2622,6 +2708,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else
 		throttle_thermal_policy_set_default(asus);
 
+	err = panel_od_check_present(asus);
+	if (err)
+		goto fail_panel_od;
+
 	err = asus_wmi_sysfs_init(asus->platform_device);
 	if (err)
 		goto fail_sysfs;
@@ -2709,6 +2799,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_throttle_thermal_policy:
 fail_fan_boost_mode:
 fail_platform:
+fail_panel_od:
 	kfree(asus);
 	return err;
 }
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 2f274cf52805..428aea701c7b 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -61,6 +61,7 @@
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
 
 /* Misc */
+#define ASUS_WMI_DEVID_PANEL_OD		0x00050019
 #define ASUS_WMI_DEVID_CAMERA		0x00060013
 #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
 
-- 
2.31.1

[PATCH 2/4] asus-wmi: Add dgpu disable method

[Luke D. Jones]

In Windows the ASUS Armory Crate program 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            | 99 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  3 +
 2 files changed, 102 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cd881443bc2f..02762a60d27a 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;
+	bool dgpu_disable;
+
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
@@ -427,6 +430,94 @@ 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 = 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;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
+
+	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;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_disable");
+
+	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);
+	bool mode = asus->dgpu_disable;
+
+	return sysfs_emit(buf, "%d\n", mode);
+}
+
+static ssize_t dgpu_disable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	bool disable;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &disable);
+	if (result == -EINVAL)
+		return result;
+
+	asus->dgpu_disable = 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.
+	*/
+	result = dgpu_disable_write(asus);
+	if (result != 0)
+		return result;
+
+	result = dgpu_disable_write(asus);
+	if (result != 0)
+		return result;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(dgpu_disable);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -2411,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,
@@ -2437,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)
@@ -2698,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;
@@ -2798,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

Re: [PATCH 2/4] asus-wmi: Add dgpu disable method

[Hans de Goede]

Hi,

On 7/17/21 10:13 AM, Luke D. Jones wrote:
> In Windows the ASUS Armory Crate program 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            | 99 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 102 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index cd881443bc2f..02762a60d27a 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;
> +	bool dgpu_disable;
> +
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
>  
> @@ -427,6 +430,94 @@ 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 = result & ASUS_WMI_DSTS_STATUS_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dgpu_disable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;

Upside-down-christmastree declaration order please.

> +
> +	value = asus->dgpu_disable;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
> +
> +	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;
> +	}
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_disable");
> +
> +	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);
> +	bool mode = asus->dgpu_disable;

This bool local variable is not really necessary, instead you can just do:

	return sysfs_emit(buf, "%d\n", asus->dgpu_disable);

The same applies to patch 1/3 btw.

> +
> +	return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t dgpu_disable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	bool disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);

Upside-down-christmastree declaration order please.

> +
> +	result = kstrtobool(buf, &disable);
> +	if (result == -EINVAL)
> +		return result;

Just "if (result) ..." please.

> +
> +	asus->dgpu_disable = 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.
> +	*/
> +	result = dgpu_disable_write(asus);
> +	if (result != 0)
> +		return result;
> +
> +	result = dgpu_disable_write(asus);
> +	if (result != 0)
> +		return result;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dgpu_disable);
> +
>  /* Battery ********************************************************************/
>  
>  /* The battery maximum charging percentage */
> @@ -2411,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,
> @@ -2437,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)
> @@ -2698,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;
> @@ -2798,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
> 

Regards,

Hans

Re: [PATCH 2/4] asus-wmi: Add dgpu disable method

[Luke Jones]

Great feedback and thorough, thank you. I feel I need to describe the 
full issue this patch tries to address now as it is causing some 
headaches.

ASUS have added an ACPI method that enables or disables the dGPU on 
these particular ROG laptops, presumably because they are unable to 
reliably deal with the dGPU waking and being used by processes in 
Windows. The ACPI method works similar to what we do in software (with 
asusctl and system76-power) by removing the device from the device tree 
so nothing can attach to it - but here it is ejected via ACPI and 
really does not exist anymore until enabled again.

So the problem is two-fold:
- A user can disable it in Windows via Armoury Crate, then reboot to 
Linux and not be able to use the dGPU as it is just not there.
- They can use the patch here to enable it, but this comes with 
caveats..

Proper enable of the dGPU again as far as my testing goes works such 
that:
1. call the ACPI method
2. rescan PCI bus to ensure the device is powered
3. call the ACPI method again to save the setting

But it appears that recent work in-kernel for many things AMD related 
has broken this for us: if the 3 steps are attempted then there is a 
fail with the parent device not able to pull out of D3cold. But only if 
the kernel is fully loaded.
```
acpi device:03: Cannot transition to power state D0 for parent in D3cold
```

If I patch the kernel to run the ACPI call as a default setting to 
enable it, then it occurs early enough in boot that it can be enabled 
correctly with userspace issuing step 3 to save it.

Getting down to brass tacks, I need to solve:
a. the D3cold issue so that a write does: write, power up, write again, 
or,
b. find how to set the default in 3 steps.

We would be perfectly fine to set a default on boot. First set and save 
takes time due to while-loops in acpi but subsequent runs after proper 
save return early. We do not suffer the same consequences as Windows 
does either, as we can do what windows does (ASUS really) in software 
rather than ACPI.

The reason this particular patch is required is because users of 
laptops with this function are unable to use the dGPU at all if 
disabled via windows. The only thing we really need to do here is 
enable it on boot and ensure the setting is saved so reboots do not 
stick on the ACPI while loop (which is approx 5 seconds).

I hope the above is clear.

Many thanks,
Luke.

On Sat, Jul 17 2021 at 18:05:03 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 7/17/21 10:13 AM, Luke D. Jones wrote:
>>  In Windows the ASUS Armory Crate program 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            | 99 
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 102 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index cd881443bc2f..02762a60d27a 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;
>>  +	bool dgpu_disable;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  @@ -427,6 +430,94 @@ 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 = result & ASUS_WMI_DSTS_STATUS_BIT;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int dgpu_disable_write(struct asus_wmi *asus)
>>  +{
>>  +	int err;
>>  +	u8 value;
>>  +	u32 retval;
> 
> Upside-down-christmastree declaration order please.
> 
>>  +
>>  +	value = asus->dgpu_disable;
>>  +
>>  +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
>>  +
>>  +	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;
>>  +	}
>>  +
>>  +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
>> "dgpu_disable");
>>  +
>>  +	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);
>>  +	bool mode = asus->dgpu_disable;
> 
> This bool local variable is not really necessary, instead you can 
> just do:
> 
> 	return sysfs_emit(buf, "%d\n", asus->dgpu_disable);
> 
> The same applies to patch 1/3 btw.
> 
>>  +
>>  +	return sysfs_emit(buf, "%d\n", mode);
>>  +}
>>  +
>>  +static ssize_t dgpu_disable_store(struct device *dev,
>>  +				    struct device_attribute *attr,
>>  +				    const char *buf, size_t count)
>>  +{
>>  +	int result;
>>  +	bool disable;
>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
> 
> Upside-down-christmastree declaration order please.
> 
>>  +
>>  +	result = kstrtobool(buf, &disable);
>>  +	if (result == -EINVAL)
>>  +		return result;
> 
> Just "if (result) ..." please.
> 
>>  +
>>  +	asus->dgpu_disable = 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.
>>  +	*/
>>  +	result = dgpu_disable_write(asus);
>>  +	if (result != 0)
>>  +		return result;
>>  +
>>  +	result = dgpu_disable_write(asus);
>>  +	if (result != 0)
>>  +		return result;
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +static DEVICE_ATTR_RW(dgpu_disable);
>>  +
>>   /* Battery 
>> ********************************************************************/
>> 
>>   /* The battery maximum charging percentage */
>>  @@ -2411,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,
>>  @@ -2437,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)
>>  @@ -2698,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;
>>  @@ -2798,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
>> 
> 
> Regards,
> 
> Hans
> 

Re: [PATCH 2/4] asus-wmi: Add dgpu disable method

[Hans de Goede]

Hi Luke,

On 7/24/21 1:37 PM, Luke Jones wrote:
> Great feedback and thorough, thank you. I feel I need to describe the full issue this patch tries to address now as it is causing some headaches.
> 
> ASUS have added an ACPI method that enables or disables the dGPU on these particular ROG laptops, presumably because they are unable to reliably deal with the dGPU waking and being used by processes in Windows. The ACPI method works similar to what we do in software (with asusctl and system76-power) by removing the device from the device tree so nothing can attach to it - but here it is ejected via ACPI and really does not exist anymore until enabled again.
> 
> So the problem is two-fold:
> - A user can disable it in Windows via Armoury Crate, then reboot to Linux and not be able to use the dGPU as it is just not there.
> - They can use the patch here to enable it, but this comes with caveats..
> 
> Proper enable of the dGPU again as far as my testing goes works such that:
> 1. call the ACPI method
> 2. rescan PCI bus to ensure the device is powered
> 3. call the ACPI method again to save the setting
> 
> But it appears that recent work in-kernel for many things AMD related has broken this for us: if the 3 steps are attempted then there is a fail with the parent device not able to pull out of D3cold. But only if the kernel is fully loaded.
> ```
> acpi device:03: Cannot transition to power state D0 for parent in D3cold
> ```
> 
> If I patch the kernel to run the ACPI call as a default setting to enable it, then it occurs early enough in boot that it can be enabled correctly with userspace issuing step 3 to save it.
> 
> Getting down to brass tacks, I need to solve:
> a. the D3cold issue so that a write does: write, power up, write again, or,
> b. find how to set the default in 3 steps.

I  think we should try to get b. to work (again), we really don't want to add
something highly Asus specific to the standard ACPI boot-patrhs. IOW ideally
this should all be contained inside the asus-wmi.ko file which I believe
means that we need b.

I think the best way forward here is to send an email to the authors of the
recent changes which broke b. +  the linux-acpi and linux-pci mailinglists and
then try to solve things with those folks.

I guess it would be good if you can first figure out which recent commit
exactly broke b.

Regards,

Hans



> 
> We would be perfectly fine to set a default on boot. First set and save takes time due to while-loops in acpi but subsequent runs after proper save return early. We do not suffer the same consequences as Windows does either, as we can do what windows does (ASUS really) in software rather than ACPI.
> 
> The reason this particular patch is required is because users of laptops with this function are unable to use the dGPU at all if disabled via windows. The only thing we really need to do here is enable it on boot and ensure the setting is saved so reboots do not stick on the ACPI while loop (which is approx 5 seconds).
> 
> I hope the above is clear.
> 
> Many thanks,
> Luke.
> 
> On Sat, Jul 17 2021 at 18:05:03 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 7/17/21 10:13 AM, Luke D. Jones wrote:
>>>  In Windows the ASUS Armory Crate program 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            | 99 ++++++++++++++++++++++
>>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>>   2 files changed, 102 insertions(+)
>>>
>>>  diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>  index cd881443bc2f..02762a60d27a 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;
>>>  +    bool dgpu_disable;
>>>  +
>>>       bool throttle_thermal_policy_available;
>>>       u8 throttle_thermal_policy_mode;
>>>
>>>  @@ -427,6 +430,94 @@ 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 = result & ASUS_WMI_DSTS_STATUS_BIT;
>>>  +    }
>>>  +
>>>  +    return 0;
>>>  +}
>>>  +
>>>  +static int dgpu_disable_write(struct asus_wmi *asus)
>>>  +{
>>>  +    int err;
>>>  +    u8 value;
>>>  +    u32 retval;
>>
>> Upside-down-christmastree declaration order please.
>>
>>>  +
>>>  +    value = asus->dgpu_disable;
>>>  +
>>>  +    err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
>>>  +
>>>  +    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;
>>>  +    }
>>>  +
>>>  +    sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_disable");
>>>  +
>>>  +    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);
>>>  +    bool mode = asus->dgpu_disable;
>>
>> This bool local variable is not really necessary, instead you can just do:
>>
>>     return sysfs_emit(buf, "%d\n", asus->dgpu_disable);
>>
>> The same applies to patch 1/3 btw.
>>
>>>  +
>>>  +    return sysfs_emit(buf, "%d\n", mode);
>>>  +}
>>>  +
>>>  +static ssize_t dgpu_disable_store(struct device *dev,
>>>  +                    struct device_attribute *attr,
>>>  +                    const char *buf, size_t count)
>>>  +{
>>>  +    int result;
>>>  +    bool disable;
>>>  +    struct asus_wmi *asus = dev_get_drvdata(dev);
>>
>> Upside-down-christmastree declaration order please.
>>
>>>  +
>>>  +    result = kstrtobool(buf, &disable);
>>>  +    if (result == -EINVAL)
>>>  +        return result;
>>
>> Just "if (result) ..." please.
>>
>>>  +
>>>  +    asus->dgpu_disable = 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.
>>>  +    */
>>>  +    result = dgpu_disable_write(asus);
>>>  +    if (result != 0)
>>>  +        return result;
>>>  +
>>>  +    result = dgpu_disable_write(asus);
>>>  +    if (result != 0)
>>>  +        return result;
>>>  +
>>>  +    return count;
>>>  +}
>>>  +
>>>  +static DEVICE_ATTR_RW(dgpu_disable);
>>>  +
>>>   /* Battery ********************************************************************/
>>>
>>>   /* The battery maximum charging percentage */
>>>  @@ -2411,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,
>>>  @@ -2437,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)
>>>  @@ -2698,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;
>>>  @@ -2798,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
>>>
>>
>> Regards,
>>
>> Hans
>>
> 
> 

[PATCH 3/4] asus-wmi: Add egpu enable method

[Luke D. Jones]

The X13 Flow laptops can utilise an external GPU. This requires
toggling an ACPI method which will first disable the internal
dGPU, and then enable the eGPU.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 91 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  3 +
 2 files changed, 94 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
+	bool egpu_enable;
+
 	bool dgpu_disable_available;
 	bool dgpu_disable;
 
@@ -430,6 +433,86 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
 	}
 }
 
+/* eGPU ********************************************************************/
+static int egpu_enable_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->egpu_enable_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+
+	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+		asus->egpu_enable_available = true;
+		asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
+	}
+
+	return 0;
+}
+
+static int egpu_enable_write(struct asus_wmi *asus)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->egpu_enable;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
+
+	if (err) {
+		pr_warn("Failed to set egpu disable: %d\n", err);
+		return err;
+	}
+
+	if (retval > 1 || retval < 0) {
+		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
+
+	return 0;
+}
+
+static ssize_t egpu_enable_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	bool mode = asus->egpu_enable;
+
+	return sysfs_emit(buf, "%d\n", mode);
+}
+
+static ssize_t egpu_enable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	bool disable;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &disable);
+	if (result == -EINVAL)
+		return result;
+
+	asus->egpu_enable = disable;
+
+	result = egpu_enable_write(asus);
+	if (result != 0)
+		return result;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(egpu_enable);
+
 /* dGPU ********************************************************************/
 static int dgpu_disable_check_present(struct asus_wmi *asus)
 {
@@ -2502,6 +2585,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_camera.attr,
 	&dev_attr_cardr.attr,
 	&dev_attr_touchpad.attr,
+	&dev_attr_egpu_enable.attr,
 	&dev_attr_dgpu_disable.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
@@ -2529,6 +2613,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_egpu_enable.attr)
+		ok = asus->egpu_enable_available;
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
@@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	err = egpu_enable_check_present(asus);
+	if (err)
+		goto fail_egpu_enable;
+
 	err = dgpu_disable_check_present(asus);
 	if (err)
 		goto fail_dgpu_disable;
@@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_sysfs:
 fail_throttle_thermal_policy:
 fail_fan_boost_mode:
+fail_egpu_enable:
 fail_dgpu_disable:
 fail_platform:
 fail_panel_od:
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU		0x00090019
+
 /* dgpu on/off */
 #define ASUS_WMI_DEVID_DGPU		0x00090020
 
-- 
2.31.1

Re: [PATCH 3/4] asus-wmi: Add egpu enable method

[Luke Jones]

Apologies, I forgot that the patches contain sequence. There is 
actually no 4th patch (the patch itself was already upstreamed).

Sincere regards,
Luke.

On Sat, Jul 17 2021 at 20:13:23 +1200, Luke D. Jones <luke@ljones.dev> 
wrote:
> The X13 Flow laptops can utilise an external GPU. This requires
> toggling an ACPI method which will first disable the internal
> dGPU, and then enable the eGPU.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 91 
> ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 94 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c 
> b/drivers/platform/x86/asus-wmi.c
> index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
> +	bool egpu_enable;
> +
>  	bool dgpu_disable_available;
>  	bool dgpu_disable;
> 
> @@ -430,6 +433,86 @@ static void 
> lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>  	}
>  }
> 
> +/* eGPU 
> ********************************************************************/
> +static int egpu_enable_check_present(struct asus_wmi *asus)
> +{
> +	u32 result;
> +	int err;
> +
> +	asus->egpu_enable_available = false;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
> +	if (err) {
> +		if (err == -ENODEV)
> +			return 0;
> +		return err;
> +	}
> +
> +	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> +		asus->egpu_enable_available = true;
> +		asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int egpu_enable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;
> +
> +	value = asus->egpu_enable;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
> +
> +	if (err) {
> +		pr_warn("Failed to set egpu disable: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
> +		return -EIO;
> +	}
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
> +
> +	return 0;
> +}
> +
> +static ssize_t egpu_enable_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	bool mode = asus->egpu_enable;
> +
> +	return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t egpu_enable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	bool disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &disable);
> +	if (result == -EINVAL)
> +		return result;
> +
> +	asus->egpu_enable = disable;
> +
> +	result = egpu_enable_write(asus);
> +	if (result != 0)
> +		return result;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(egpu_enable);
> +
>  /* dGPU 
> ********************************************************************/
>  static int dgpu_disable_check_present(struct asus_wmi *asus)
>  {
> @@ -2502,6 +2585,7 @@ static struct attribute *platform_attributes[] 
> = {
>  	&dev_attr_camera.attr,
>  	&dev_attr_cardr.attr,
>  	&dev_attr_touchpad.attr,
> +	&dev_attr_egpu_enable.attr,
>  	&dev_attr_dgpu_disable.attr,
>  	&dev_attr_lid_resume.attr,
>  	&dev_attr_als_enable.attr,
> @@ -2529,6 +2613,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_egpu_enable.attr)
> +		ok = asus->egpu_enable_available;
>  	else if (attr == &dev_attr_dgpu_disable.attr)
>  		ok = asus->dgpu_disable_available;
>  	else if (attr == &dev_attr_fan_boost_mode.attr)
> @@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct platform_device 
> *pdev)
>  	if (err)
>  		goto fail_platform;
> 
> +	err = egpu_enable_check_present(asus);
> +	if (err)
> +		goto fail_egpu_enable;
> +
>  	err = dgpu_disable_check_present(asus);
>  	if (err)
>  		goto fail_dgpu_disable;
> @@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct platform_device 
> *pdev)
>  fail_sysfs:
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
> +fail_egpu_enable:
>  fail_dgpu_disable:
>  fail_platform:
>  fail_panel_od:
> diff --git a/include/linux/platform_data/x86/asus-wmi.h 
> b/include/linux/platform_data/x86/asus-wmi.h
> index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU		0x00090019
> +
>  /* dgpu on/off */
>  #define ASUS_WMI_DEVID_DGPU		0x00090020
> 
> --
> 2.31.1
> 

Re: [PATCH 3/4] asus-wmi: Add egpu enable method

[Luke Jones]

Damn. I thought `-v2` on `git send-email` would bump patch version too. 
What is the correct way to do that for a full patch sequence?

On Sat, Jul 17 2021 at 20:15:30 +1200, Luke Jones <luke@ljones.dev> 
wrote:
> Apologies, I forgot that the patches contain sequence. There is 
> actually no 4th patch (the patch itself was already upstreamed).
> 
> Sincere regards,
> Luke.
> 
> On Sat, Jul 17 2021 at 20:13:23 +1200, Luke D. Jones 
> <luke@ljones.dev> wrote:
>> The X13 Flow laptops can utilise an external GPU. This requires
>> toggling an ACPI method which will first disable the internal
>> dGPU, and then enable the eGPU.
>> 
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>> ---
>>  drivers/platform/x86/asus-wmi.c            | 91 
>> \x7f++++++++++++++++++++++
>>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>>  2 files changed, 94 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/asus-wmi.c 
>> \x7fb/drivers/platform/x86/asus-wmi.c
>> index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
>> +	bool egpu_enable;
>> +
>>  	bool dgpu_disable_available;
>>  	bool dgpu_disable;
>> 
>> @@ -430,6 +433,86 @@ static void 
>> \x7flid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>>  	}
>>  }
>> 
>> +/* eGPU 
>> \x7f********************************************************************/
>> +static int egpu_enable_check_present(struct asus_wmi *asus)
>> +{
>> +	u32 result;
>> +	int err;
>> +
>> +	asus->egpu_enable_available = false;
>> +
>> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
>> +	if (err) {
>> +		if (err == -ENODEV)
>> +			return 0;
>> +		return err;
>> +	}
>> +
>> +	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
>> +		asus->egpu_enable_available = true;
>> +		asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int egpu_enable_write(struct asus_wmi *asus)
>> +{
>> +	int err;
>> +	u8 value;
>> +	u32 retval;
>> +
>> +	value = asus->egpu_enable;
>> +
>> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
>> +
>> +	if (err) {
>> +		pr_warn("Failed to set egpu disable: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	if (retval > 1 || retval < 0) {
>> +		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
>> +		return -EIO;
>> +	}
>> +
>> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
>> "egpu_enable");
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t egpu_enable_show(struct device *dev,
>> +				   struct device_attribute *attr, char *buf)
>> +{
>> +	struct asus_wmi *asus = dev_get_drvdata(dev);
>> +	bool mode = asus->egpu_enable;
>> +
>> +	return sysfs_emit(buf, "%d\n", mode);
>> +}
>> +
>> +static ssize_t egpu_enable_store(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    const char *buf, size_t count)
>> +{
>> +	int result;
>> +	bool disable;
>> +	struct asus_wmi *asus = dev_get_drvdata(dev);
>> +
>> +	result = kstrtobool(buf, &disable);
>> +	if (result == -EINVAL)
>> +		return result;
>> +
>> +	asus->egpu_enable = disable;
>> +
>> +	result = egpu_enable_write(asus);
>> +	if (result != 0)
>> +		return result;
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(egpu_enable);
>> +
>>  /* dGPU 
>> \x7f********************************************************************/
>>  static int dgpu_disable_check_present(struct asus_wmi *asus)
>>  {
>> @@ -2502,6 +2585,7 @@ static struct attribute *platform_attributes[] 
>> \x7f= {
>>  	&dev_attr_camera.attr,
>>  	&dev_attr_cardr.attr,
>>  	&dev_attr_touchpad.attr,
>> +	&dev_attr_egpu_enable.attr,
>>  	&dev_attr_dgpu_disable.attr,
>>  	&dev_attr_lid_resume.attr,
>>  	&dev_attr_als_enable.attr,
>> @@ -2529,6 +2613,8 @@ static umode_t asus_sysfs_is_visible(struct 
>> \x7fkobject *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_egpu_enable.attr)
>> +		ok = asus->egpu_enable_available;
>>  	else if (attr == &dev_attr_dgpu_disable.attr)
>>  		ok = asus->dgpu_disable_available;
>>  	else if (attr == &dev_attr_fan_boost_mode.attr)
>> @@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct 
>> platform_device \x7f*pdev)
>>  	if (err)
>>  		goto fail_platform;
>> 
>> +	err = egpu_enable_check_present(asus);
>> +	if (err)
>> +		goto fail_egpu_enable;
>> +
>>  	err = dgpu_disable_check_present(asus);
>>  	if (err)
>>  		goto fail_dgpu_disable;
>> @@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct platform_device 
>> \x7f*pdev)
>>  fail_sysfs:
>>  fail_throttle_thermal_policy:
>>  fail_fan_boost_mode:
>> +fail_egpu_enable:
>>  fail_dgpu_disable:
>>  fail_platform:
>>  fail_panel_od:
>> diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> \x7fb/include/linux/platform_data/x86/asus-wmi.h
>> index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU		0x00090019
>> +
>>  /* dgpu on/off */
>>  #define ASUS_WMI_DEVID_DGPU		0x00090020
>> 
>> --
>> 2.31.1
>> 
> 

Re: [PATCH 3/4] asus-wmi: Add egpu enable method

[Hans de Goede]

Hi Luke,

On 7/17/21 10:19 AM, Luke Jones wrote:
> Damn. I thought `-v2` on `git send-email` would bump patch version too. What is the correct way to do that for a full patch sequence?

You need to split the sending of patches into 2 steps, which generally is a good
idea anyways, since that will also allow you to easily add a cover-letter to the
series:

Lets say you are ready to send v3 and you have the 3 patches as the last 3
commits in your git tree's current HEAD, then you would do:

git format-patch --cover-letter -v3 HEAD~3
$EDITOR v3-0000*.patch
# Edit the cover letter, say something like:
# Hi All here is v3 of my ... series, which does foobar
# new in v3 is ...
# And don't forget to set the Subject
git send-email v3-00*.patch

And you're done. I hope this helps.

Regards,

Hans





> 
> On Sat, Jul 17 2021 at 20:15:30 +1200, Luke Jones <luke@ljones.dev> wrote:
>> Apologies, I forgot that the patches contain sequence. There is actually no 4th patch (the patch itself was already upstreamed).
>>
>> Sincere regards,
>> Luke.
>>
>> On Sat, Jul 17 2021 at 20:13:23 +1200, Luke D. Jones <luke@ljones.dev> wrote:
>>> The X13 Flow laptops can utilise an external GPU. This requires
>>> toggling an ACPI method which will first disable the internal
>>> dGPU, and then enable the eGPU.
>>>
>>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>> ---
>>>  drivers/platform/x86/asus-wmi.c            | 91 \x7f++++++++++++++++++++++
>>>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>>>  2 files changed, 94 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c \x7fb/drivers/platform/x86/asus-wmi.c
>>> index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
>>> +    bool egpu_enable;
>>> +
>>>      bool dgpu_disable_available;
>>>      bool dgpu_disable;
>>>
>>> @@ -430,6 +433,86 @@ static void \x7flid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>>>      }
>>>  }
>>>
>>> +/* eGPU \x7f********************************************************************/
>>> +static int egpu_enable_check_present(struct asus_wmi *asus)
>>> +{
>>> +    u32 result;
>>> +    int err;
>>> +
>>> +    asus->egpu_enable_available = false;
>>> +
>>> +    err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
>>> +    if (err) {
>>> +        if (err == -ENODEV)
>>> +            return 0;
>>> +        return err;
>>> +    }
>>> +
>>> +    if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
>>> +        asus->egpu_enable_available = true;
>>> +        asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int egpu_enable_write(struct asus_wmi *asus)
>>> +{
>>> +    int err;
>>> +    u8 value;
>>> +    u32 retval;
>>> +
>>> +    value = asus->egpu_enable;
>>> +
>>> +    err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
>>> +
>>> +    if (err) {
>>> +        pr_warn("Failed to set egpu disable: %d\n", err);
>>> +        return err;
>>> +    }
>>> +
>>> +    if (retval > 1 || retval < 0) {
>>> +        pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static ssize_t egpu_enable_show(struct device *dev,
>>> +                   struct device_attribute *attr, char *buf)
>>> +{
>>> +    struct asus_wmi *asus = dev_get_drvdata(dev);
>>> +    bool mode = asus->egpu_enable;
>>> +
>>> +    return sysfs_emit(buf, "%d\n", mode);
>>> +}
>>> +
>>> +static ssize_t egpu_enable_store(struct device *dev,
>>> +                    struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +    int result;
>>> +    bool disable;
>>> +    struct asus_wmi *asus = dev_get_drvdata(dev);
>>> +
>>> +    result = kstrtobool(buf, &disable);
>>> +    if (result == -EINVAL)
>>> +        return result;
>>> +
>>> +    asus->egpu_enable = disable;
>>> +
>>> +    result = egpu_enable_write(asus);
>>> +    if (result != 0)
>>> +        return result;
>>> +
>>> +    return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RW(egpu_enable);
>>> +
>>>  /* dGPU \x7f********************************************************************/
>>>  static int dgpu_disable_check_present(struct asus_wmi *asus)
>>>  {
>>> @@ -2502,6 +2585,7 @@ static struct attribute *platform_attributes[] \x7f= {
>>>      &dev_attr_camera.attr,
>>>      &dev_attr_cardr.attr,
>>>      &dev_attr_touchpad.attr,
>>> +    &dev_attr_egpu_enable.attr,
>>>      &dev_attr_dgpu_disable.attr,
>>>      &dev_attr_lid_resume.attr,
>>>      &dev_attr_als_enable.attr,
>>> @@ -2529,6 +2613,8 @@ static umode_t asus_sysfs_is_visible(struct \x7fkobject *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_egpu_enable.attr)
>>> +        ok = asus->egpu_enable_available;
>>>      else if (attr == &dev_attr_dgpu_disable.attr)
>>>          ok = asus->dgpu_disable_available;
>>>      else if (attr == &dev_attr_fan_boost_mode.attr)
>>> @@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct platform_device \x7f*pdev)
>>>      if (err)
>>>          goto fail_platform;
>>>
>>> +    err = egpu_enable_check_present(asus);
>>> +    if (err)
>>> +        goto fail_egpu_enable;
>>> +
>>>      err = dgpu_disable_check_present(asus);
>>>      if (err)
>>>          goto fail_dgpu_disable;
>>> @@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct platform_device \x7f*pdev)
>>>  fail_sysfs:
>>>  fail_throttle_thermal_policy:
>>>  fail_fan_boost_mode:
>>> +fail_egpu_enable:
>>>  fail_dgpu_disable:
>>>  fail_platform:
>>>  fail_panel_od:
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h \x7fb/include/linux/platform_data/x86/asus-wmi.h
>>> index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU        0x00090019
>>> +
>>>  /* dgpu on/off */
>>>  #define ASUS_WMI_DEVID_DGPU        0x00090020
>>>
>>> -- 
>>> 2.31.1
>>>
>>
> 
> 

Re: [PATCH 3/4] asus-wmi: Add egpu enable method

[Luke Jones]

Oh I see, thanks. I was trying to do the "-v3" on send-email part.

On Sat, Jul 17 2021 at 17:57:54 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi Luke,
> 
> On 7/17/21 10:19 AM, Luke Jones wrote:
>>  Damn. I thought `-v2` on `git send-email` would bump patch version 
>> too. What is the correct way to do that for a full patch sequence?
> 
> You need to split the sending of patches into 2 steps, which 
> generally is a good
> idea anyways, since that will also allow you to easily add a 
> cover-letter to the
> series:
> 
> Lets say you are ready to send v3 and you have the 3 patches as the 
> last 3
> commits in your git tree's current HEAD, then you would do:
> 
> git format-patch --cover-letter -v3 HEAD~3
> $EDITOR v3-0000*.patch
> # Edit the cover letter, say something like:
> # Hi All here is v3 of my ... series, which does foobar
> # new in v3 is ...
> # And don't forget to set the Subject
> git send-email v3-00*.patch
> 
> And you're done. I hope this helps.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>> 
>>  On Sat, Jul 17 2021 at 20:15:30 +1200, Luke Jones <luke@ljones.dev> 
>> wrote:
>>>  Apologies, I forgot that the patches contain sequence. There is 
>>> actually no 4th patch (the patch itself was already upstreamed).
>>> 
>>>  Sincere regards,
>>>  Luke.
>>> 
>>>  On Sat, Jul 17 2021 at 20:13:23 +1200, Luke D. Jones 
>>> <luke@ljones.dev> wrote:
>>>>  The X13 Flow laptops can utilise an external GPU. This requires
>>>>  toggling an ACPI method which will first disable the internal
>>>>  dGPU, and then enable the eGPU.
>>>> 
>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>  ---
>>>>   drivers/platform/x86/asus-wmi.c            | 91 
>>>> \x7f++++++++++++++++++++++
>>>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>>>   2 files changed, 94 insertions(+)
>>>> 
>>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>>> \x7fb/drivers/platform/x86/asus-wmi.c
>>>>  index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
>>>>  +    bool egpu_enable;
>>>>  +
>>>>       bool dgpu_disable_available;
>>>>       bool dgpu_disable;
>>>> 
>>>>  @@ -430,6 +433,86 @@ static void 
>>>> \x7flid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>>>>       }
>>>>   }
>>>> 
>>>>  +/* eGPU 
>>>> \x7f********************************************************************/
>>>>  +static int egpu_enable_check_present(struct asus_wmi *asus)
>>>>  +{
>>>>  +    u32 result;
>>>>  +    int err;
>>>>  +
>>>>  +    asus->egpu_enable_available = false;
>>>>  +
>>>>  +    err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, 
>>>> &result);
>>>>  +    if (err) {
>>>>  +        if (err == -ENODEV)
>>>>  +            return 0;
>>>>  +        return err;
>>>>  +    }
>>>>  +
>>>>  +    if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
>>>>  +        asus->egpu_enable_available = true;
>>>>  +        asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
>>>>  +    }
>>>>  +
>>>>  +    return 0;
>>>>  +}
>>>>  +
>>>>  +static int egpu_enable_write(struct asus_wmi *asus)
>>>>  +{
>>>>  +    int err;
>>>>  +    u8 value;
>>>>  +    u32 retval;
>>>>  +
>>>>  +    value = asus->egpu_enable;
>>>>  +
>>>>  +    err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, 
>>>> &retval);
>>>>  +
>>>>  +    if (err) {
>>>>  +        pr_warn("Failed to set egpu disable: %d\n", err);
>>>>  +        return err;
>>>>  +    }
>>>>  +
>>>>  +    if (retval > 1 || retval < 0) {
>>>>  +        pr_warn("Failed to set egpu disable (retval): 0x%x\n", 
>>>> retval);
>>>>  +        return -EIO;
>>>>  +    }
>>>>  +
>>>>  +    sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
>>>> "egpu_enable");
>>>>  +
>>>>  +    return 0;
>>>>  +}
>>>>  +
>>>>  +static ssize_t egpu_enable_show(struct device *dev,
>>>>  +                   struct device_attribute *attr, char *buf)
>>>>  +{
>>>>  +    struct asus_wmi *asus = dev_get_drvdata(dev);
>>>>  +    bool mode = asus->egpu_enable;
>>>>  +
>>>>  +    return sysfs_emit(buf, "%d\n", mode);
>>>>  +}
>>>>  +
>>>>  +static ssize_t egpu_enable_store(struct device *dev,
>>>>  +                    struct device_attribute *attr,
>>>>  +                    const char *buf, size_t count)
>>>>  +{
>>>>  +    int result;
>>>>  +    bool disable;
>>>>  +    struct asus_wmi *asus = dev_get_drvdata(dev);
>>>>  +
>>>>  +    result = kstrtobool(buf, &disable);
>>>>  +    if (result == -EINVAL)
>>>>  +        return result;
>>>>  +
>>>>  +    asus->egpu_enable = disable;
>>>>  +
>>>>  +    result = egpu_enable_write(asus);
>>>>  +    if (result != 0)
>>>>  +        return result;
>>>>  +
>>>>  +    return count;
>>>>  +}
>>>>  +
>>>>  +static DEVICE_ATTR_RW(egpu_enable);
>>>>  +
>>>>   /* dGPU 
>>>> \x7f********************************************************************/
>>>>   static int dgpu_disable_check_present(struct asus_wmi *asus)
>>>>   {
>>>>  @@ -2502,6 +2585,7 @@ static struct attribute 
>>>> *platform_attributes[] \x7f= {
>>>>       &dev_attr_camera.attr,
>>>>       &dev_attr_cardr.attr,
>>>>       &dev_attr_touchpad.attr,
>>>>  +    &dev_attr_egpu_enable.attr,
>>>>       &dev_attr_dgpu_disable.attr,
>>>>       &dev_attr_lid_resume.attr,
>>>>       &dev_attr_als_enable.attr,
>>>>  @@ -2529,6 +2613,8 @@ static umode_t asus_sysfs_is_visible(struct 
>>>> \x7fkobject *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_egpu_enable.attr)
>>>>  +        ok = asus->egpu_enable_available;
>>>>       else if (attr == &dev_attr_dgpu_disable.attr)
>>>>           ok = asus->dgpu_disable_available;
>>>>       else if (attr == &dev_attr_fan_boost_mode.attr)
>>>>  @@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct 
>>>> platform_device \x7f*pdev)
>>>>       if (err)
>>>>           goto fail_platform;
>>>> 
>>>>  +    err = egpu_enable_check_present(asus);
>>>>  +    if (err)
>>>>  +        goto fail_egpu_enable;
>>>>  +
>>>>       err = dgpu_disable_check_present(asus);
>>>>       if (err)
>>>>           goto fail_dgpu_disable;
>>>>  @@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct 
>>>> platform_device \x7f*pdev)
>>>>   fail_sysfs:
>>>>   fail_throttle_thermal_policy:
>>>>   fail_fan_boost_mode:
>>>>  +fail_egpu_enable:
>>>>   fail_dgpu_disable:
>>>>   fail_platform:
>>>>   fail_panel_od:
>>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>>> \x7fb/include/linux/platform_data/x86/asus-wmi.h
>>>>  index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU        0x00090019
>>>>  +
>>>>   /* dgpu on/off */
>>>>   #define ASUS_WMI_DEVID_DGPU        0x00090020
>>>> 
>>>>  --
>>>>  2.31.1
>>>> 
>>> 
>> 
>> 
> 

Re: [PATCH 3/4] asus-wmi: Add egpu enable method

[Hans de Goede]

Hi,

On 7/17/21 10:13 AM, Luke D. Jones wrote:
> The X13 Flow laptops can utilise an external GPU. This requires
> toggling an ACPI method which will first disable the internal
> dGPU, and then enable the eGPU.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 91 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 94 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 02762a60d27a..ee5d8656641e 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 egpu_enable_available; // 0 = enable
> +	bool egpu_enable;
> +
>  	bool dgpu_disable_available;
>  	bool dgpu_disable;
>  
> @@ -430,6 +433,86 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>  	}
>  }
>  
> +/* eGPU ********************************************************************/
> +static int egpu_enable_check_present(struct asus_wmi *asus)
> +{
> +	u32 result;
> +	int err;
> +
> +	asus->egpu_enable_available = false;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
> +	if (err) {
> +		if (err == -ENODEV)
> +			return 0;
> +		return err;
> +	}
> +
> +	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> +		asus->egpu_enable_available = true;
> +		asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int egpu_enable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;

Upside-down-christmastree declaration order please.

> +
> +	value = asus->egpu_enable;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
> +
> +	if (err) {
> +		pr_warn("Failed to set egpu disable: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
> +		return -EIO;
> +	}
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
> +
> +	return 0;
> +}
> +
> +static ssize_t egpu_enable_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	bool mode = asus->egpu_enable;

Drop the unnecessary "mode" local variable please.

> +
> +	return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t egpu_enable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	bool disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);

Upside-down-christmastree declaration order please.

> +
> +	result = kstrtobool(buf, &disable);
> +	if (result == -EINVAL)
> +		return result;

Just "if (result) ..." please.

> +
> +	asus->egpu_enable = disable;
> +
> +	result = egpu_enable_write(asus);
> +	if (result != 0)
> +		return result;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(egpu_enable);
> +
>  /* dGPU ********************************************************************/
>  static int dgpu_disable_check_present(struct asus_wmi *asus)
>  {
> @@ -2502,6 +2585,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_camera.attr,
>  	&dev_attr_cardr.attr,
>  	&dev_attr_touchpad.attr,
> +	&dev_attr_egpu_enable.attr,
>  	&dev_attr_dgpu_disable.attr,
>  	&dev_attr_lid_resume.attr,
>  	&dev_attr_als_enable.attr,
> @@ -2529,6 +2613,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_egpu_enable.attr)
> +		ok = asus->egpu_enable_available;
>  	else if (attr == &dev_attr_dgpu_disable.attr)
>  		ok = asus->dgpu_disable_available;
>  	else if (attr == &dev_attr_fan_boost_mode.attr)
> @@ -2792,6 +2878,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_platform;
>  
> +	err = egpu_enable_check_present(asus);
> +	if (err)
> +		goto fail_egpu_enable;
> +
>  	err = dgpu_disable_check_present(asus);
>  	if (err)
>  		goto fail_dgpu_disable;
> @@ -2896,6 +2986,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_sysfs:
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
> +fail_egpu_enable:
>  fail_dgpu_disable:
>  fail_platform:
>  fail_panel_od:
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a528f9d0e4b7..17dc5cb6f3f2 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_EGPU		0x00090019
> +
>  /* dgpu on/off */
>  #define ASUS_WMI_DEVID_DGPU		0x00090020
>  
> 

Regards,

Hans

Re: [PATCH 1/4] asus-wmi: Add panel overdrive functionality

[Hans de Goede]

Hi,

On 7/17/21 10:13 AM, Luke D. Jones wrote:
> Some ASUS ROG laptops have the ability to drive the display panel
> a higher rate to eliminate or reduce ghosting.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Looks good, almost there some small remarks inline.


> ---
>  drivers/platform/x86/asus-wmi.c            | 91 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  1 +
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ebaeb7bb80f5..cd881443bc2f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -216,6 +216,9 @@ struct asus_wmi {
>  	// The RSOC controls the maximum charging percentage.
>  	bool battery_rsoc_available;
>  
> +	bool panel_overdrive_available;
> +	bool panel_overdrive;
> +
>  	struct hotplug_slot hotplug_slot;
>  	struct mutex hotplug_lock;
>  	struct mutex wmi_lock;
> @@ -1221,6 +1224,86 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
>  	return result;
>  }
>  
> +/* Panel Overdrive ************************************************************/
> +static int panel_od_check_present(struct asus_wmi *asus)
> +{
> +	u32 result;
> +	int err;
> +
> +	asus->panel_overdrive_available = false;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_PANEL_OD, &result);
> +	if (err) {
> +		if (err == -ENODEV)
> +			return 0;
> +		return err;
> +	}
> +
> +	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> +		asus->panel_overdrive_available = true;
> +		asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int panel_od_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;

nitpick: Please use upside-down-christmastree style for variable-declarations,
what that means is put the longest line first, so in this case:

	u32 retval;
	u8 value;
	int err;

> +
> +	value = asus->panel_overdrive;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, value, &retval);
> +
> +	if (err) {
> +		pr_warn("Failed to set panel overdrive: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set panel overdrive (retval): 0x%x\n", retval);
> +		return -EIO;
> +	}
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "panel_od");
> +
> +	return 0;
> +}
> +
> +static ssize_t panel_od_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	bool mode = asus->panel_overdrive;
> +
> +	return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t panel_od_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	bool overdrive;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);

Nitpick again, please make this:

	struct asus_wmi *asus = dev_get_drvdata(dev);
	bool overdrive;
	int result;

> +
> +	result = kstrtobool(buf, &overdrive);
> +	if (result == -EINVAL)

Any return from kstrtobool() other then 0 is an error,
for v3 please just make this:

	if (result)
		return result;


> +		return result;
> +
> +	asus->panel_overdrive = overdrive;
> +	result = panel_od_write(asus);
> +
> +	if (result != 0)
> +		return result;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(panel_od);
> +
>  /* Quirks *********************************************************************/
>  
>  static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
> @@ -2332,6 +2415,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_als_enable.attr,
>  	&dev_attr_fan_boost_mode.attr,
>  	&dev_attr_throttle_thermal_policy.attr,
> +	&dev_attr_panel_od.attr,
>  	NULL
>  };
>  
> @@ -2357,6 +2441,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		ok = asus->fan_boost_mode_available;
>  	else if (attr == &dev_attr_throttle_thermal_policy.attr)
>  		ok = asus->throttle_thermal_policy_available;
> +	else if (attr == &dev_attr_panel_od.attr)
> +		ok = asus->panel_overdrive_available;
>  
>  	if (devid != -1)
>  		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
> @@ -2622,6 +2708,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	else
>  		throttle_thermal_policy_set_default(asus);
>  
> +	err = panel_od_check_present(asus);
> +	if (err)
> +		goto fail_panel_od;
> +
>  	err = asus_wmi_sysfs_init(asus->platform_device);
>  	if (err)
>  		goto fail_sysfs;
> @@ -2709,6 +2799,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
>  fail_platform:
> +fail_panel_od:
>  	kfree(asus);
>  	return err;
>  }
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 2f274cf52805..428aea701c7b 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -61,6 +61,7 @@
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
>  
>  /* Misc */
> +#define ASUS_WMI_DEVID_PANEL_OD		0x00050019
>  #define ASUS_WMI_DEVID_CAMERA		0x00060013
>  #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
>  
> 

Regards,

Hans