[PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke D. Jones]

Adds support for the screenpad(-plus) found on a few ASUS laptops that have a main 16:9 or 16:10 screen and a shorter screen below the main but above the keyboard.
The support consists of:
- On off control
- Setting brightness from 0-255

There are some small quirks with this device when considering only the raw WMI methods:
1. The Off method can only switch the device off
2. Changing the brightness turns the device back on
3. To turn the device back on the brightness must be > 1
4. When the device is off the brightness can't be changed (so it is stored by the driver if device is off).
5. Booting with a value of 0 brightness (retained by bios) means the bios will set a value of > 0, < 15 which is far too dim and was unexpected by testers. The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15.
6. When the device is off it is "unplugged"

All of the above points are addressed within the patch to create a good user experience and keep within user expectations.

Changelog:
- V2
  - Complete refactor to use as a backlight device

Luke D. Jones (1):
  platform/x86: asus-wmi: add support for ASUS screenpad

 .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
 drivers/platform/x86/asus-wmi.c               | 132 ++++++++++++++++++
 drivers/platform/x86/asus-wmi.h               |   1 +
 include/linux/platform_data/x86/asus-wmi.h    |   4 +
 4 files changed, 138 insertions(+), 1 deletion(-)

-- 
2.40.0

[PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke D. Jones]

Add support for the WMI methods used to turn off and adjust the
brightness of the secondary "screenpad" device found on some high-end
ASUS laptops like the GX650P series and others.

These methods are utilised in a new backlight device named:
- asus_screenpad

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

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index a77a004a1baa..df9817c6233a 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -97,4 +97,4 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Enable an LCD response-time boost to reduce or remove ghosting:
 			* 0 - Disable,
-			* 1 - Enable
+			* 1 - Enable
\ No newline at end of file
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1038dfdcdd32..0528eef02ef7 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -200,6 +200,7 @@ struct asus_wmi {
 
 	struct input_dev *inputdev;
 	struct backlight_device *backlight_device;
+	struct backlight_device *screenpad_backlight_device;
 	struct platform_device *platform_device;
 
 	struct led_classdev wlan_led;
@@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
 	return 0;
 }
 
+/* Screenpad backlight */
+
+static int read_screenpad_backlight_power(struct asus_wmi *asus)
+{
+	int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
+
+	if (ret < 0)
+		return ret;
+	/* 1 == powered */
+	return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
+}
+
+static int read_screenpad_brightness(struct backlight_device *bd)
+{
+	struct asus_wmi *asus = bl_get_data(bd);
+	u32 retval;
+	int err;
+
+	err = read_screenpad_backlight_power(asus);
+	if (err < 0)
+		return err;
+	/* The device brightness can only be read if powered, so return stored */
+	if (err == FB_BLANK_POWERDOWN)
+		return asus->driver->screenpad_brightness;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
+	if (err < 0)
+		return err;
+
+	return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
+}
+
+static int update_screenpad_bl_status(struct backlight_device *bd)
+{
+	struct asus_wmi *asus = bl_get_data(bd);
+	int power, err = 0;
+	u32 ctrl_param;
+
+	power = read_screenpad_backlight_power(asus);
+	if (power == -ENODEV)
+		return err;
+	else if (power < 0)
+		return power;
+
+	if (bd->props.power != power) {
+		if (power != FB_BLANK_UNBLANK) {
+			/* Only brightness can power it back on */
+			ctrl_param = asus->driver->screenpad_brightness;
+			/* Min 1 or the screen won't turn on */
+			if (ctrl_param == 0)
+				ctrl_param = 1;
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
+							ctrl_param, NULL);
+		} else {
+			/* Ensure brightness is stored to turn back on with */
+			asus->driver->screenpad_brightness = bd->props.brightness;
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
+		}
+	} else if (power == FB_BLANK_UNBLANK) {
+		/* Only set brightness if powered on or we get invalid/unsync state */
+		ctrl_param = bd->props.brightness;
+		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
+	}
+
+	return err;
+}
+
+static const struct backlight_ops asus_screenpad_bl_ops = {
+	.get_brightness = read_screenpad_brightness,
+	.update_status = update_screenpad_bl_status,
+	.options = BL_CORE_SUSPENDRESUME,
+};
+
+static int asus_screenpad_init(struct asus_wmi *asus)
+{
+	struct backlight_device *bd;
+	struct backlight_properties props;
+	int power, brightness;
+
+	power = read_screenpad_backlight_power(asus);
+	if (power == -ENODEV)
+		power = FB_BLANK_UNBLANK;
+	else if (power < 0)
+		return power;
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
+	props.max_brightness = 255;
+	bd = backlight_device_register("asus_screenpad",
+				       &asus->platform_device->dev, asus,
+				       &asus_screenpad_bl_ops, &props);
+	if (IS_ERR(bd)) {
+		pr_err("Could not register backlight device\n");
+		return PTR_ERR(bd);
+	}
+
+	asus->screenpad_backlight_device = bd;
+
+	brightness = read_screenpad_brightness(bd);
+	if (brightness < 0)
+		return brightness;
+	/*
+	 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
+	 * 60 is subjective, but accepted as a good compromise to retain visibility.
+	 */
+	else if (brightness < 60)
+		brightness = 60;
+
+	asus->driver->screenpad_brightness = brightness;
+	bd->props.brightness = brightness;
+	bd->props.power = power;
+	backlight_update_status(bd);
+
+	return 0;
+}
+
+static void asus_screenpad_exit(struct asus_wmi *asus)
+{
+	backlight_device_unregister(asus->screenpad_backlight_device);
+
+	asus->screenpad_backlight_device = NULL;
+}
+
 /* Fn-lock ********************************************************************/
 
 static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
@@ -3823,6 +3947,13 @@ static int asus_wmi_add(struct platform_device *pdev)
 	} else if (asus->driver->quirks->wmi_backlight_set_devstate)
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
+		pr_warn("Begin asus_screenpad_init");
+		err = asus_screenpad_init(asus);
+		if (err && err != -ENODEV)
+			goto fail_backlight;
+	}
+
 	if (asus_wmi_has_fnlock_key(asus)) {
 		asus->fnlock_locked = fnlock_default;
 		asus_wmi_fnlock_update(asus);
@@ -3844,6 +3975,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 
 fail_wmi_handler:
 	asus_wmi_backlight_exit(asus);
+	asus_screenpad_exit(asus);
 fail_backlight:
 	asus_wmi_rfkill_exit(asus);
 fail_rfkill:
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index a478ebfd34df..5fbdd0eafa02 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -57,6 +57,7 @@ struct quirk_entry {
 struct asus_wmi_driver {
 	int			brightness;
 	int			panel_power;
+	int			screenpad_brightness;
 	int			wlan_ctrl_by_user;
 
 	const char		*name;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 28234dc9fa6a..a2d94adb5c80 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -58,6 +58,10 @@
 #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
 #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
+/* This can only be used to disable the screen, not re-enable */
+#define ASUS_WMI_DEVID_SCREENPAD_POWER	0x00050031
+/* Writing a brightness re-enables the screen if disabled */
+#define ASUS_WMI_DEVID_SCREENPAD_LIGHT	0x00050032
 #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
 
-- 
2.40.0

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Ilpo Järvinen]

On Fri, 5 May 2023, Luke D. Jones wrote:

> Add support for the WMI methods used to turn off and adjust the
> brightness of the secondary "screenpad" device found on some high-end
> ASUS laptops like the GX650P series and others.
> 
> These methods are utilised in a new backlight device named:
> - asus_screenpad
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>  drivers/platform/x86/asus-wmi.c               | 132 ++++++++++++++++++
>  drivers/platform/x86/asus-wmi.h               |   1 +
>  include/linux/platform_data/x86/asus-wmi.h    |   4 +
>  4 files changed, 138 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index a77a004a1baa..df9817c6233a 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -97,4 +97,4 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Enable an LCD response-time boost to reduce or remove ghosting:
>  			* 0 - Disable,
> -			* 1 - Enable
> +			* 1 - Enable
> \ No newline at end of file

Spurious change?

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1038dfdcdd32..0528eef02ef7 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -200,6 +200,7 @@ struct asus_wmi {
>  
>  	struct input_dev *inputdev;
>  	struct backlight_device *backlight_device;
> +	struct backlight_device *screenpad_backlight_device;
>  	struct platform_device *platform_device;
>  
>  	struct led_classdev wlan_led;
> @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>  	return 0;
>  }
>  
> +/* Screenpad backlight */
> +
> +static int read_screenpad_backlight_power(struct asus_wmi *asus)
> +{
> +	int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);

Please move this to own line because now you have the extra newline 
in between the call and error handling.

> +
> +	if (ret < 0)
> +		return ret;
> +	/* 1 == powered */
> +	return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> +}
> +
> +static int read_screenpad_brightness(struct backlight_device *bd)
> +{
> +	struct asus_wmi *asus = bl_get_data(bd);
> +	u32 retval;
> +	int err;
> +
> +	err = read_screenpad_backlight_power(asus);
> +	if (err < 0)
> +		return err;
> +	/* The device brightness can only be read if powered, so return stored */
> +	if (err == FB_BLANK_POWERDOWN)
> +		return asus->driver->screenpad_brightness;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
> +	if (err < 0)
> +		return err;
> +
> +	return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
> +}
> +
> +static int update_screenpad_bl_status(struct backlight_device *bd)
> +{
> +	struct asus_wmi *asus = bl_get_data(bd);
> +	int power, err = 0;
> +	u32 ctrl_param;
> +
> +	power = read_screenpad_backlight_power(asus);
> +	if (power == -ENODEV)
> +		return err;

Just return 0. Or is there perhaps something wrong/missing here?

> +	else if (power < 0)
> +		return power;
> +
> +	if (bd->props.power != power) {
> +		if (power != FB_BLANK_UNBLANK) {
> +			/* Only brightness can power it back on */

Only brightness > 0 can power the screen back on

> +			ctrl_param = asus->driver->screenpad_brightness;

max(1, asus->driver->screenpad_brightness);

Don't forget to add the #include for it.

> +			/* Min 1 or the screen won't turn on */
> +			if (ctrl_param == 0)
> +				ctrl_param = 1;

Drop this.

> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
> +							ctrl_param, NULL);

Align param.

> +		} else {
> +			/* Ensure brightness is stored to turn back on with */
> +			asus->driver->screenpad_brightness = bd->props.brightness;
> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
> +		}
> +	} else if (power == FB_BLANK_UNBLANK) {
> +		/* Only set brightness if powered on or we get invalid/unsync state */
> +		ctrl_param = bd->props.brightness;
> +		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);

Why not store the brightness if powered off?

> +	}
> +
> +	return err;
> +}
> +
> +static const struct backlight_ops asus_screenpad_bl_ops = {
> +	.get_brightness = read_screenpad_brightness,
> +	.update_status = update_screenpad_bl_status,
> +	.options = BL_CORE_SUSPENDRESUME,
> +};
> +
> +static int asus_screenpad_init(struct asus_wmi *asus)
> +{
> +	struct backlight_device *bd;
> +	struct backlight_properties props;
> +	int power, brightness;
> +
> +	power = read_screenpad_backlight_power(asus);
> +	if (power == -ENODEV)
> +		power = FB_BLANK_UNBLANK;
> +	else if (power < 0)
> +		return power;
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
> +	props.max_brightness = 255;
> +	bd = backlight_device_register("asus_screenpad",
> +				       &asus->platform_device->dev, asus,
> +				       &asus_screenpad_bl_ops, &props);
> +	if (IS_ERR(bd)) {
> +		pr_err("Could not register backlight device\n");
> +		return PTR_ERR(bd);
> +	}
> +
> +	asus->screenpad_backlight_device = bd;
> +
> +	brightness = read_screenpad_brightness(bd);
> +	if (brightness < 0)
> +		return brightness;
> +	/*
> +	 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
> +	 * 60 is subjective, but accepted as a good compromise to retain visibility.
> +	 */
> +	else if (brightness < 60)

Since the other branch returns, else is unnecessary.

-- 
 i.

> +		brightness = 60;
> +
> +	asus->driver->screenpad_brightness = brightness;
> +	bd->props.brightness = brightness;
> +	bd->props.power = power;
> +	backlight_update_status(bd);
> +
> +	return 0;
> +}
> +
> +static void asus_screenpad_exit(struct asus_wmi *asus)
> +{
> +	backlight_device_unregister(asus->screenpad_backlight_device);
> +
> +	asus->screenpad_backlight_device = NULL;
> +}
> +
>  /* Fn-lock ********************************************************************/
>  
>  static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
> @@ -3823,6 +3947,13 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	} else if (asus->driver->quirks->wmi_backlight_set_devstate)
>  		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
>  
> +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
> +		pr_warn("Begin asus_screenpad_init");
> +		err = asus_screenpad_init(asus);
> +		if (err && err != -ENODEV)
> +			goto fail_backlight;
> +	}
> +
>  	if (asus_wmi_has_fnlock_key(asus)) {
>  		asus->fnlock_locked = fnlock_default;
>  		asus_wmi_fnlock_update(asus);
> @@ -3844,6 +3975,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  
>  fail_wmi_handler:
>  	asus_wmi_backlight_exit(asus);
> +	asus_screenpad_exit(asus);
>  fail_backlight:
>  	asus_wmi_rfkill_exit(asus);
>  fail_rfkill:
> diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
> index a478ebfd34df..5fbdd0eafa02 100644
> --- a/drivers/platform/x86/asus-wmi.h
> +++ b/drivers/platform/x86/asus-wmi.h
> @@ -57,6 +57,7 @@ struct quirk_entry {
>  struct asus_wmi_driver {
>  	int			brightness;
>  	int			panel_power;
> +	int			screenpad_brightness;
>  	int			wlan_ctrl_by_user;
>  
>  	const char		*name;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 28234dc9fa6a..a2d94adb5c80 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -58,6 +58,10 @@
>  #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
>  #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
>  #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
> +/* This can only be used to disable the screen, not re-enable */
> +#define ASUS_WMI_DEVID_SCREENPAD_POWER	0x00050031
> +/* Writing a brightness re-enables the screen if disabled */
> +#define ASUS_WMI_DEVID_SCREENPAD_LIGHT	0x00050032
>  #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
>  
> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen 
<ilpo.jarvinen@linux.intel.com> wrote:
> On Fri, 5 May 2023, Luke D. Jones wrote:
> 
>>  Add support for the WMI methods used to turn off and adjust the
>>  brightness of the secondary "screenpad" device found on some 
>> high-end
>>  ASUS laptops like the GX650P series and others.
>> 
>>  These methods are utilised in a new backlight device named:
>>  - asus_screenpad
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>   drivers/platform/x86/asus-wmi.c               | 132 
>> ++++++++++++++++++
>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>   4 files changed, 138 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi 
>> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>  index a77a004a1baa..df9817c6233a 100644
>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>  @@ -97,4 +97,4 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>>   Description:
>>   		Enable an LCD response-time boost to reduce or remove ghosting:
>>   			* 0 - Disable,
>>  -			* 1 - Enable
>>  +			* 1 - Enable
>>  \ No newline at end of file
> 
> Spurious change?

Indeed it is. Not sure how that occurred.

> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 1038dfdcdd32..0528eef02ef7 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>> 
>>   	struct input_dev *inputdev;
>>   	struct backlight_device *backlight_device;
>>  +	struct backlight_device *screenpad_backlight_device;
>>   	struct platform_device *platform_device;
>> 
>>   	struct led_classdev wlan_led;
>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>   	return 0;
>>   }
>> 
>>  +/* Screenpad backlight */
>>  +
>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>  +{
>>  +	int ret = asus_wmi_get_devstate_simple(asus, 
>> ASUS_WMI_DEVID_SCREENPAD_POWER);
> 
> Please move this to own line because now you have the extra newline
> in between the call and error handling.

I don't understand what you mean sorry. Remove the new line or:
int ret;
ret = asus_wmi_get_devstate_simple(asus, 
ASUS_WMI_DEVID_SCREENPAD_POWER);

> 
>>  +
>>  +	if (ret < 0)
>>  +		return ret;
>>  +	/* 1 == powered */
>>  +	return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>  +}
>>  +
>>  +static int read_screenpad_brightness(struct backlight_device *bd)
>>  +{
>>  +	struct asus_wmi *asus = bl_get_data(bd);
>>  +	u32 retval;
>>  +	int err;
>>  +
>>  +	err = read_screenpad_backlight_power(asus);
>>  +	if (err < 0)
>>  +		return err;
>>  +	/* The device brightness can only be read if powered, so return 
>> stored */
>>  +	if (err == FB_BLANK_POWERDOWN)
>>  +		return asus->driver->screenpad_brightness;
>>  +
>>  +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, 
>> &retval);
>>  +	if (err < 0)
>>  +		return err;
>>  +
>>  +	return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>  +}
>>  +
>>  +static int update_screenpad_bl_status(struct backlight_device *bd)
>>  +{
>>  +	struct asus_wmi *asus = bl_get_data(bd);
>>  +	int power, err = 0;
>>  +	u32 ctrl_param;
>>  +
>>  +	power = read_screenpad_backlight_power(asus);
>>  +	if (power == -ENODEV)
>>  +		return err;
> 
> Just return 0. Or is there perhaps something wrong/missing here?

I thought the correct thing was to return any possible error state 
(here, anything less than 0 would be an error, right?)

> 
>>  +	else if (power < 0)
>>  +		return power;
>>  +
>>  +	if (bd->props.power != power) {
>>  +		if (power != FB_BLANK_UNBLANK) {
>>  +			/* Only brightness can power it back on */
> 
> Only brightness > 0 can power the screen back on
> 
>>  +			ctrl_param = asus->driver->screenpad_brightness;
> 
> max(1, asus->driver->screenpad_brightness);
> 
> Don't forget to add the #include for it.

Oh, that's handy! Thank you.

> 
>>  +			/* Min 1 or the screen won't turn on */
>>  +			if (ctrl_param == 0)
>>  +				ctrl_param = 1;
> 
> Drop this.

Thanks to minmax.

> 
>>  +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
>>  +							ctrl_param, NULL);
> 
> Align param.

Done.

> 
>>  +		} else {
>>  +			/* Ensure brightness is stored to turn back on with */
>>  +			asus->driver->screenpad_brightness = bd->props.brightness;
>>  +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, 
>> NULL);
>>  +		}
>>  +	} else if (power == FB_BLANK_UNBLANK) {
>>  +		/* Only set brightness if powered on or we get invalid/unsync 
>> state */
>>  +		ctrl_param = bd->props.brightness;
>>  +		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, 
>> ctrl_param, NULL);
> 
> Why not store the brightness if powered off?

That's me being literal and short sighted. I've now moved:
```
/* Ensure brightness is stored to turn back on with */
asus->driver->screenpad_brightness = bd->props.brightness;
```
to below the conditional blocks.

> 
>>  +	}
>>  +
>>  +	return err;
>>  +}
>>  +
>>  +static const struct backlight_ops asus_screenpad_bl_ops = {
>>  +	.get_brightness = read_screenpad_brightness,
>>  +	.update_status = update_screenpad_bl_status,
>>  +	.options = BL_CORE_SUSPENDRESUME,
>>  +};
>>  +
>>  +static int asus_screenpad_init(struct asus_wmi *asus)
>>  +{
>>  +	struct backlight_device *bd;
>>  +	struct backlight_properties props;
>>  +	int power, brightness;
>>  +
>>  +	power = read_screenpad_backlight_power(asus);
>>  +	if (power == -ENODEV)
>>  +		power = FB_BLANK_UNBLANK;
>>  +	else if (power < 0)
>>  +		return power;
>>  +
>>  +	memset(&props, 0, sizeof(struct backlight_properties));
>>  +	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be 
>> picked */
>>  +	props.max_brightness = 255;
>>  +	bd = backlight_device_register("asus_screenpad",
>>  +				       &asus->platform_device->dev, asus,
>>  +				       &asus_screenpad_bl_ops, &props);
>>  +	if (IS_ERR(bd)) {
>>  +		pr_err("Could not register backlight device\n");
>>  +		return PTR_ERR(bd);
>>  +	}
>>  +
>>  +	asus->screenpad_backlight_device = bd;
>>  +
>>  +	brightness = read_screenpad_brightness(bd);
>>  +	if (brightness < 0)
>>  +		return brightness;
>>  +	/*
>>  +	 * Counter an odd behaviour where default is set to < 13 if it 
>> was 0 on boot.
>>  +	 * 60 is subjective, but accepted as a good compromise to retain 
>> visibility.
>>  +	 */
>>  +	else if (brightness < 60)
> 
> Since the other branch returns, else is unnecessary.

Good catch, thank you.

I'll submit V3 after we clarify the two points above that I'm confused 
by :)

Thank you for taking the time to review.

> 

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Guenter Roeck]

On 5/5/23 16:43, Luke Jones wrote:
> 
> 
> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>
>>>  Add support for the WMI methods used to turn off and adjust the
>>>  brightness of the secondary "screenpad" device found on some high-end
>>>  ASUS laptops like the GX650P series and others.
>>>
>>>  These methods are utilised in a new backlight device named:
>>>  - asus_screenpad
>>>
>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>  ---
>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>   drivers/platform/x86/asus-wmi.c               | 132 ++++++++++++++++++
>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>
>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>  index a77a004a1baa..df9817c6233a 100644
>>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>  @@ -97,4 +97,4 @@ Contact:    "Luke Jones" <luke@ljones.dev>
>>>   Description:
>>>           Enable an LCD response-time boost to reduce or remove ghosting:
>>>               * 0 - Disable,
>>>  -            * 1 - Enable
>>>  +            * 1 - Enable
>>>  \ No newline at end of file
>>
>> Spurious change?
> 
> Indeed it is. Not sure how that occurred.
> 
>>
>>>  diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>  index 1038dfdcdd32..0528eef02ef7 100644
>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>>>
>>>       struct input_dev *inputdev;
>>>       struct backlight_device *backlight_device;
>>>  +    struct backlight_device *screenpad_backlight_device;
>>>       struct platform_device *platform_device;
>>>
>>>       struct led_classdev wlan_led;
>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>       return 0;
>>>   }
>>>
>>>  +/* Screenpad backlight */
>>>  +
>>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>  +{
>>>  +    int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>>
>> Please move this to own line because now you have the extra newline
>> in between the call and error handling.
> 
> I don't understand what you mean sorry. Remove the new line or:
> int ret;
> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
> 
>>
>>>  +
>>>  +    if (ret < 0)
>>>  +        return ret;
>>>  +    /* 1 == powered */
>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>  +}
>>>  +
>>>  +static int read_screenpad_brightness(struct backlight_device *bd)
>>>  +{
>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>  +    u32 retval;
>>>  +    int err;
>>>  +
>>>  +    err = read_screenpad_backlight_power(asus);
>>>  +    if (err < 0)
>>>  +        return err;
>>>  +    /* The device brightness can only be read if powered, so return stored */
>>>  +    if (err == FB_BLANK_POWERDOWN)
>>>  +        return asus->driver->screenpad_brightness;
>>>  +
>>>  +    err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>  +    if (err < 0)
>>>  +        return err;
>>>  +
>>>  +    return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>  +}
>>>  +
>>>  +static int update_screenpad_bl_status(struct backlight_device *bd)
>>>  +{
>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>  +    int power, err = 0;
>>>  +    u32 ctrl_param;
>>>  +
>>>  +    power = read_screenpad_backlight_power(asus);
>>>  +    if (power == -ENODEV)
>>>  +        return err;
>>
>> Just return 0. Or is there perhaps something wrong/missing here?
> 
> I thought the correct thing was to return any possible error state (here, anything less than 0 would be an error, right?)
> 

Well, yes, but you are not returning an error. You are returning 'err'
which is 0 at this point. So, at the very least, this code is (very)
misleading since it suggests that it would return some error
(as saved in the 'err' variable) when it doesn't.

Guenter

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck 
<linux@roeck-us.net> wrote:
> On 5/5/23 16:43, Luke Jones wrote:
>> 
>> 
>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen 
>> <ilpo.jarvinen@linux.intel.com> wrote:
>>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>> 
>>>>  Add support for the WMI methods used to turn off and adjust the
>>>>  brightness of the secondary "screenpad" device found on some 
>>>> high-end
>>>>  ASUS laptops like the GX650P series and others.
>>>> 
>>>>  These methods are utilised in a new backlight device named:
>>>>  - asus_screenpad
>>>> 
>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>  ---
>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>   drivers/platform/x86/asus-wmi.c               | 132 
>>>> ++++++++++++++++++
>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>> 
>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi 
>>>> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>  index a77a004a1baa..df9817c6233a 100644
>>>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>  @@ -97,4 +97,4 @@ Contact:    "Luke Jones" <luke@ljones.dev>
>>>>   Description:
>>>>           Enable an LCD response-time boost to reduce or remove 
>>>> ghosting:
>>>>               * 0 - Disable,
>>>>  -            * 1 - Enable
>>>>  +            * 1 - Enable
>>>>  \ No newline at end of file
>>> 
>>> Spurious change?
>> 
>> Indeed it is. Not sure how that occurred.
>> 
>>> 
>>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>>> b/drivers/platform/x86/asus-wmi.c
>>>>  index 1038dfdcdd32..0528eef02ef7 100644
>>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>>>> 
>>>>       struct input_dev *inputdev;
>>>>       struct backlight_device *backlight_device;
>>>>  +    struct backlight_device *screenpad_backlight_device;
>>>>       struct platform_device *platform_device;
>>>> 
>>>>       struct led_classdev wlan_led;
>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>       return 0;
>>>>   }
>>>> 
>>>>  +/* Screenpad backlight */
>>>>  +
>>>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>>  +{
>>>>  +    int ret = asus_wmi_get_devstate_simple(asus, 
>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>> 
>>> Please move this to own line because now you have the extra newline
>>> in between the call and error handling.
>> 
>> I don't understand what you mean sorry. Remove the new line or:
>> int ret;
>> ret = asus_wmi_get_devstate_simple(asus, 
>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>> 
>>> 
>>>>  +
>>>>  +    if (ret < 0)
>>>>  +        return ret;
>>>>  +    /* 1 == powered */
>>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>>  +}
>>>>  +
>>>>  +static int read_screenpad_brightness(struct backlight_device *bd)
>>>>  +{
>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>  +    u32 retval;
>>>>  +    int err;
>>>>  +
>>>>  +    err = read_screenpad_backlight_power(asus);
>>>>  +    if (err < 0)
>>>>  +        return err;
>>>>  +    /* The device brightness can only be read if powered, so 
>>>> return stored */
>>>>  +    if (err == FB_BLANK_POWERDOWN)
>>>>  +        return asus->driver->screenpad_brightness;
>>>>  +
>>>>  +    err = asus_wmi_get_devstate(asus, 
>>>> ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>>  +    if (err < 0)
>>>>  +        return err;
>>>>  +
>>>>  +    return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>>  +}
>>>>  +
>>>>  +static int update_screenpad_bl_status(struct backlight_device 
>>>> *bd)
>>>>  +{
>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>  +    int power, err = 0;
>>>>  +    u32 ctrl_param;
>>>>  +
>>>>  +    power = read_screenpad_backlight_power(asus);
>>>>  +    if (power == -ENODEV)
>>>>  +        return err;
>>> 
>>> Just return 0. Or is there perhaps something wrong/missing here?
>> 
>> I thought the correct thing was to return any possible error state 
>> (here, anything less than 0 would be an error, right?)
>> 
> 
> Well, yes, but you are not returning an error. You are returning 'err'
> which is 0 at this point. So, at the very least, this code is (very)
> misleading since it suggests that it would return some error
> (as saved in the 'err' variable) when it doesn't.
> 
> Guenter
> 

Oh! Right I see it now, I'm sorry, I just kept skipping over it somehow.

So I should change to:
	power = read_screenpad_backlight_power(asus);
	if (power < 0)
		return power;

Is that acceptable?

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Guenter Roeck]

On 5/5/23 20:48, Luke Jones wrote:
> 
> 
> On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 5/5/23 16:43, Luke Jones wrote:
>>>
>>>
>>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
>>>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>>>
>>>>>  Add support for the WMI methods used to turn off and adjust the
>>>>>  brightness of the secondary "screenpad" device found on some high-end
>>>>>  ASUS laptops like the GX650P series and others.
>>>>>
>>>>>  These methods are utilised in a new backlight device named:
>>>>>  - asus_screenpad
>>>>>
>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>  ---
>>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>>   drivers/platform/x86/asus-wmi.c               | 132 ++++++++++++++++++
>>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>
>>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>  index a77a004a1baa..df9817c6233a 100644
>>>>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>  @@ -97,4 +97,4 @@ Contact:    "Luke Jones" <luke@ljones.dev>
>>>>>   Description:
>>>>>           Enable an LCD response-time boost to reduce or remove ghosting:
>>>>>               * 0 - Disable,
>>>>>  -            * 1 - Enable
>>>>>  +            * 1 - Enable
>>>>>  \ No newline at end of file
>>>>
>>>> Spurious change?
>>>
>>> Indeed it is. Not sure how that occurred.
>>>
>>>>
>>>>>  diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>>  index 1038dfdcdd32..0528eef02ef7 100644
>>>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>>>>>
>>>>>       struct input_dev *inputdev;
>>>>>       struct backlight_device *backlight_device;
>>>>>  +    struct backlight_device *screenpad_backlight_device;
>>>>>       struct platform_device *platform_device;
>>>>>
>>>>>       struct led_classdev wlan_led;
>>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>>  +/* Screenpad backlight */
>>>>>  +
>>>>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>>>  +{
>>>>>  +    int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>
>>>> Please move this to own line because now you have the extra newline
>>>> in between the call and error handling.
>>>
>>> I don't understand what you mean sorry. Remove the new line or:
>>> int ret;
>>> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>
>>>>
>>>>>  +
>>>>>  +    if (ret < 0)
>>>>>  +        return ret;
>>>>>  +    /* 1 == powered */
>>>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>>>  +}
>>>>>  +
>>>>>  +static int read_screenpad_brightness(struct backlight_device *bd)
>>>>>  +{
>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>  +    u32 retval;
>>>>>  +    int err;
>>>>>  +
>>>>>  +    err = read_screenpad_backlight_power(asus);
>>>>>  +    if (err < 0)
>>>>>  +        return err;
>>>>>  +    /* The device brightness can only be read if powered, so return stored */
>>>>>  +    if (err == FB_BLANK_POWERDOWN)
>>>>>  +        return asus->driver->screenpad_brightness;
>>>>>  +
>>>>>  +    err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>>>  +    if (err < 0)
>>>>>  +        return err;
>>>>>  +
>>>>>  +    return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>>>  +}
>>>>>  +
>>>>>  +static int update_screenpad_bl_status(struct backlight_device *bd)
>>>>>  +{
>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>  +    int power, err = 0;
>>>>>  +    u32 ctrl_param;
>>>>>  +
>>>>>  +    power = read_screenpad_backlight_power(asus);
>>>>>  +    if (power == -ENODEV)
>>>>>  +        return err;
>>>>
>>>> Just return 0. Or is there perhaps something wrong/missing here?
>>>
>>> I thought the correct thing was to return any possible error state (here, anything less than 0 would be an error, right?)
>>>
>>
>> Well, yes, but you are not returning an error. You are returning 'err'
>> which is 0 at this point. So, at the very least, this code is (very)
>> misleading since it suggests that it would return some error
>> (as saved in the 'err' variable) when it doesn't.
>>
>> Guenter
>>
> 
> Oh! Right I see it now, I'm sorry, I just kept skipping over it somehow.
> 
> So I should change to:
>      power = read_screenpad_backlight_power(asus);
>      if (power < 0)
>          return power;
> 
> Is that acceptable?
> 

That depends on what the code is supposed to do. Right now it is
"If read_screenpad_backlight_power() returns -ENODEV, stop and return
no error (let the rest of the code continue). If it returns another error,
return it".
Changing the code as suggested above changes the semantics to
"If read_screenpad_backlight_power() returns an error, always return it".

Looking at the patch, I don't have a definite answer.
asus_screenpad_init() happily registers the backlight if
read_screenpad_backlight_power() returns -ENODEV. One could argue that
the other functions should thus handle that situation gracefully as well,
but read_screenpad_brightness() does return -ENODEV in that situation.
I think you should decide how you want to handle that case and handle
it consistently across all functions.

Either case, there is another problem in asus_screenpad_init():
If read_screenpad_brightness() fails, the function returns an error,
but does not unregister the backlight device.

Guenter

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck 
<linux@roeck-us.net> wrote:
> On 5/5/23 20:48, Luke Jones wrote:
>> 
>> 
>> On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck 
>> <linux@roeck-us.net> wrote:
>>> On 5/5/23 16:43, Luke Jones wrote:
>>>> 
>>>> 
>>>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen 
>>>> <ilpo.jarvinen@linux.intel.com> wrote:
>>>>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>>>> 
>>>>>>  Add support for the WMI methods used to turn off and adjust the
>>>>>>  brightness of the secondary "screenpad" device found on some 
>>>>>> high-end
>>>>>>  ASUS laptops like the GX650P series and others.
>>>>>> 
>>>>>>  These methods are utilised in a new backlight device named:
>>>>>>  - asus_screenpad
>>>>>> 
>>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>>  ---
>>>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>>>   drivers/platform/x86/asus-wmi.c               | 132 
>>>>>> ++++++++++++++++++
>>>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi 
>>>>>> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>  index a77a004a1baa..df9817c6233a 100644
>>>>>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>  @@ -97,4 +97,4 @@ Contact:    "Luke Jones" <luke@ljones.dev>
>>>>>>   Description:
>>>>>>           Enable an LCD response-time boost to reduce or remove 
>>>>>> ghosting:
>>>>>>               * 0 - Disable,
>>>>>>  -            * 1 - Enable
>>>>>>  +            * 1 - Enable
>>>>>>  \ No newline at end of file
>>>>> 
>>>>> Spurious change?
>>>> 
>>>> Indeed it is. Not sure how that occurred.
>>>> 
>>>>> 
>>>>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>>>>> b/drivers/platform/x86/asus-wmi.c
>>>>>>  index 1038dfdcdd32..0528eef02ef7 100644
>>>>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>>>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>>>>>> 
>>>>>>       struct input_dev *inputdev;
>>>>>>       struct backlight_device *backlight_device;
>>>>>>  +    struct backlight_device *screenpad_backlight_device;
>>>>>>       struct platform_device *platform_device;
>>>>>> 
>>>>>>       struct led_classdev wlan_led;
>>>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>>>       return 0;
>>>>>>   }
>>>>>> 
>>>>>>  +/* Screenpad backlight */
>>>>>>  +
>>>>>>  +static int read_screenpad_backlight_power(struct asus_wmi 
>>>>>> *asus)
>>>>>>  +{
>>>>>>  +    int ret = asus_wmi_get_devstate_simple(asus, 
>>>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>> 
>>>>> Please move this to own line because now you have the extra 
>>>>> newline
>>>>> in between the call and error handling.
>>>> 
>>>> I don't understand what you mean sorry. Remove the new line or:
>>>> int ret;
>>>> ret = asus_wmi_get_devstate_simple(asus, 
>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>> 
>>>>> 
>>>>>>  +
>>>>>>  +    if (ret < 0)
>>>>>>  +        return ret;
>>>>>>  +    /* 1 == powered */
>>>>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>>>>  +}
>>>>>>  +
>>>>>>  +static int read_screenpad_brightness(struct backlight_device 
>>>>>> *bd)
>>>>>>  +{
>>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>>  +    u32 retval;
>>>>>>  +    int err;
>>>>>>  +
>>>>>>  +    err = read_screenpad_backlight_power(asus);
>>>>>>  +    if (err < 0)
>>>>>>  +        return err;
>>>>>>  +    /* The device brightness can only be read if powered, so 
>>>>>> return stored */
>>>>>>  +    if (err == FB_BLANK_POWERDOWN)
>>>>>>  +        return asus->driver->screenpad_brightness;
>>>>>>  +
>>>>>>  +    err = asus_wmi_get_devstate(asus, 
>>>>>> ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>>>>  +    if (err < 0)
>>>>>>  +        return err;
>>>>>>  +
>>>>>>  +    return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>>>>  +}
>>>>>>  +
>>>>>>  +static int update_screenpad_bl_status(struct backlight_device 
>>>>>> *bd)
>>>>>>  +{
>>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>>  +    int power, err = 0;
>>>>>>  +    u32 ctrl_param;
>>>>>>  +
>>>>>>  +    power = read_screenpad_backlight_power(asus);
>>>>>>  +    if (power == -ENODEV)
>>>>>>  +        return err;
>>>>> 
>>>>> Just return 0. Or is there perhaps something wrong/missing here?
>>>> 
>>>> I thought the correct thing was to return any possible error state 
>>>> (here, anything less than 0 would be an error, right?)
>>>> 
>>> 
>>> Well, yes, but you are not returning an error. You are returning 
>>> 'err'
>>> which is 0 at this point. So, at the very least, this code is (very)
>>> misleading since it suggests that it would return some error
>>> (as saved in the 'err' variable) when it doesn't.
>>> 
>>> Guenter
>>> 
>> 
>> Oh! Right I see it now, I'm sorry, I just kept skipping over it 
>> somehow.
>> 
>> So I should change to:
>>      power = read_screenpad_backlight_power(asus);
>>      if (power < 0)
>>          return power;
>> 
>> Is that acceptable?
>> 
> 
> That depends on what the code is supposed to do. Right now it is
> "If read_screenpad_backlight_power() returns -ENODEV, stop and return
> no error (let the rest of the code continue). If it returns another 
> error,
> return it".
> Changing the code as suggested above changes the semantics to
> "If read_screenpad_backlight_power() returns an error, always return 
> it".
> 
> Looking at the patch, I don't have a definite answer.
> asus_screenpad_init() happily registers the backlight if
> read_screenpad_backlight_power() returns -ENODEV. One could argue that
> the other functions should thus handle that situation gracefully as 
> well,
> but read_screenpad_brightness() does return -ENODEV in that situation.
> I think you should decide how you want to handle that case and handle
> it consistently across all functions.
> 
> Either case, there is another problem in asus_screenpad_init():
> If read_screenpad_brightness() fails, the function returns an error,
> but does not unregister the backlight device.
> 
> Guenter
> 


Thanks mate. I was working on this between my workload and getting a 
few users to test. My first priority was to get the thing working for 
them and as such I didn't put much thought in to errors and consistency.

I think since `asus_screenpad_init()` is not called unless the 
brightness WMI method exists, then it and the other functions should 
return all errors if the power WMI method fails at all since the device 
functionality is compromised. If there is a hardware change in this 
aspect we could revisit it then.

 > Either case, there is another problem in asus_screenpad_init():
 > If read_screenpad_brightness() fails, the function returns an error,
 > but does not unregister the backlight device.

I wasn't entirely sure how to handle this. Mostly I followed what the 
existing backlight code was doing, I've now added a `goto 
fail_screenpad;` with:
fail_screenpad:
	if (asus->screenpad_backlight_device)
		asus_screenpad_exit(asus);

really I'm relying on others to guide me with this - I know I've got a 
fair few patches in these days but they've been mostly simple things 
except for TUF RGB, and I've had to learn a bunch of stuff as I go.

I'll await your response before I send in a V3.

Cheers,
Luke.

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Hans de Goede]

Hi Luke,

On 5/5/23 06:30, Luke D. Jones wrote:
> Adds support for the screenpad(-plus) found on a few ASUS laptops that have a main 16:9 or 16:10 screen and a shorter screen below the main but above the keyboard.
> The support consists of:
> - On off control
> - Setting brightness from 0-255
> 
> There are some small quirks with this device when considering only the raw WMI methods:
> 1. The Off method can only switch the device off
> 2. Changing the brightness turns the device back on
> 3. To turn the device back on the brightness must be > 1
> 4. When the device is off the brightness can't be changed (so it is stored by the driver if device is off).
> 5. Booting with a value of 0 brightness (retained by bios) means the bios will set a value of > 0, < 15 which is far too dim and was unexpected by testers. The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15.
> 6. When the device is off it is "unplugged"
> 
> All of the above points are addressed within the patch to create a good user experience and keep within user expectations.
> 
> Changelog:
> - V2
>   - Complete refactor to use as a backlight device

Thank you on your work for this.

Unfortunately I did not get a chance to react to the v1 posting and
the remarks to switch to using /sys/class/backlight there before you
posted this v2.

Technically the remark to use /sys/class/backlight for this is
completely correct. But due to the way how userspace uses
/sys/class/backlight this is a problematic.

Userspace basically always assumes there is only 1 LCD panel
and it then looks at /sys/class/backlight and picks 1
/sys/class/backlight entry and uses that for the brightness
slider in the desktop-environment UI / system-menu as well
as to handle brightness up/down keyboard hotkey presses.

In the (recent) past the kernel used to register e.g.
both /sys/class/backlight/acpi_video0 and
/sys/class/backlight/intel_backlight

For ACPI resp. direct hw control of the LCD panel backlight
(so both control the same backlight, sometimes both work
sometimes only 1 works).

Userspace uses the backlight-type to determine which backlight
class to use, using (for GNOME, but I believe everywhere) the
following preference order:

1. First look for "firmware" type backlight devices (like acpi_video0)
2. Then try "platform" type backlight devices
3. Last try "raw" type backlight devices

And to make things work the kernel has been hiding the "acpi_video0"
entry in cases where it is known that we need the "raw" aka native
type backlight.

Luke you seem to already be partly aware of this, because the patch
now has this:

	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */

but almost all modern laptops exclusively use the raw/native type
for backlight control of the main LCD panel.

So now we end up with 2 "raw" type backlight devices and if
e.g. gnome-settings-daemon picks the right one now sort of
is left to luck.

Well that is not entirely true, at least gnome-settings-daemon
prefers raw backlight devices where the parent has an "enabled"
sysfs attribute (it expects the parent to be a drm_connector
object) and where that enabled attribute reads as "enabled".

This is done for hybrid-gfx laptops where there already may
be 2 raw backlight-class devices, 1 for each GPU but only
1 of the 2 drm_connectors going to the main LCD panel should
actually show as enabled.

So typing all this out I guess we could go ahead with using
the backlight class for this after all, but this relies
on userspace preferring raw backlight-class devices
with a drm_connector-object parent which show as being
enabled.

Any userspace code which does not do the parent has
an enabled attr reading "enabled" or a similar check
will end up picking a random backlight class device
as control for the main panel brightness which will not
always end well. So this all is a bit fragile ...

And I'm not sure what is the best thing to do here.

Barnabás, Ilpo, Guenter, any comments on this ?

Regards,

Hans


p.s.

Note I'm working on allowing brightness control for
multiple screens in a sane way, see:

https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

The last few kernel-cycles I have landed a refactor/cleanup
of the existing backlight code so that we only ever
register 1 /sys/class/backlight entry for the main LCD
panel, instead of having e.g. both acpi_video0 + intel_backlight
and relying on userspace preferring acpi_video0 in that case.

And when I can find time for it I plan to implement
the API in the linked RFC, which allows properly
dealing with all this.

Luke, question how does the second/exta panel look
from an outputting video to it pov ?  Does it show
up as an extra screen connected to a drm_connector
on one of the GPUs. IOW can it be used with standard
kernel-modesetting APIs ?

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Guenter Roeck]

On 5/6/23 01:09, Luke Jones wrote:
> 
> 
> On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 5/5/23 20:48, Luke Jones wrote:
>>>
>>>
>>> On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On 5/5/23 16:43, Luke Jones wrote:
>>>>>
>>>>>
>>>>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
>>>>>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>>>>>
>>>>>>>  Add support for the WMI methods used to turn off and adjust the
>>>>>>>  brightness of the secondary "screenpad" device found on some high-end
>>>>>>>  ASUS laptops like the GX650P series and others.
>>>>>>>
>>>>>>>  These methods are utilised in a new backlight device named:
>>>>>>>  - asus_screenpad
>>>>>>>
>>>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>>>  ---
>>>>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>>>>   drivers/platform/x86/asus-wmi.c               | 132 ++++++++++++++++++
>>>>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>  index a77a004a1baa..df9817c6233a 100644
>>>>>>>  --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>  +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>>>  @@ -97,4 +97,4 @@ Contact:    "Luke Jones" <luke@ljones.dev>
>>>>>>>   Description:
>>>>>>>           Enable an LCD response-time boost to reduce or remove ghosting:
>>>>>>>               * 0 - Disable,
>>>>>>>  -            * 1 - Enable
>>>>>>>  +            * 1 - Enable
>>>>>>>  \ No newline at end of file
>>>>>>
>>>>>> Spurious change?
>>>>>
>>>>> Indeed it is. Not sure how that occurred.
>>>>>
>>>>>>
>>>>>>>  diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>>>>>>  index 1038dfdcdd32..0528eef02ef7 100644
>>>>>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>>>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>>>>>  @@ -200,6 +200,7 @@ struct asus_wmi {
>>>>>>>
>>>>>>>       struct input_dev *inputdev;
>>>>>>>       struct backlight_device *backlight_device;
>>>>>>>  +    struct backlight_device *screenpad_backlight_device;
>>>>>>>       struct platform_device *platform_device;
>>>>>>>
>>>>>>>       struct led_classdev wlan_led;
>>>>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>
>>>>>>>  +/* Screenpad backlight */
>>>>>>>  +
>>>>>>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>>>>>  +{
>>>>>>>  +    int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>>>
>>>>>> Please move this to own line because now you have the extra newline
>>>>>> in between the call and error handling.
>>>>>
>>>>> I don't understand what you mean sorry. Remove the new line or:
>>>>> int ret;
>>>>> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>>
>>>>>>
>>>>>>>  +
>>>>>>>  +    if (ret < 0)
>>>>>>>  +        return ret;
>>>>>>>  +    /* 1 == powered */
>>>>>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>>>>>  +}
>>>>>>>  +
>>>>>>>  +static int read_screenpad_brightness(struct backlight_device *bd)
>>>>>>>  +{
>>>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>>>  +    u32 retval;
>>>>>>>  +    int err;
>>>>>>>  +
>>>>>>>  +    err = read_screenpad_backlight_power(asus);
>>>>>>>  +    if (err < 0)
>>>>>>>  +        return err;
>>>>>>>  +    /* The device brightness can only be read if powered, so return stored */
>>>>>>>  +    if (err == FB_BLANK_POWERDOWN)
>>>>>>>  +        return asus->driver->screenpad_brightness;
>>>>>>>  +
>>>>>>>  +    err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>>>>>  +    if (err < 0)
>>>>>>>  +        return err;
>>>>>>>  +
>>>>>>>  +    return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>>>>>  +}
>>>>>>>  +
>>>>>>>  +static int update_screenpad_bl_status(struct backlight_device *bd)
>>>>>>>  +{
>>>>>>>  +    struct asus_wmi *asus = bl_get_data(bd);
>>>>>>>  +    int power, err = 0;
>>>>>>>  +    u32 ctrl_param;
>>>>>>>  +
>>>>>>>  +    power = read_screenpad_backlight_power(asus);
>>>>>>>  +    if (power == -ENODEV)
>>>>>>>  +        return err;
>>>>>>
>>>>>> Just return 0. Or is there perhaps something wrong/missing here?
>>>>>
>>>>> I thought the correct thing was to return any possible error state (here, anything less than 0 would be an error, right?)
>>>>>
>>>>
>>>> Well, yes, but you are not returning an error. You are returning 'err'
>>>> which is 0 at this point. So, at the very least, this code is (very)
>>>> misleading since it suggests that it would return some error
>>>> (as saved in the 'err' variable) when it doesn't.
>>>>
>>>> Guenter
>>>>
>>>
>>> Oh! Right I see it now, I'm sorry, I just kept skipping over it somehow.
>>>
>>> So I should change to:
>>>      power = read_screenpad_backlight_power(asus);
>>>      if (power < 0)
>>>          return power;
>>>
>>> Is that acceptable?
>>>
>>
>> That depends on what the code is supposed to do. Right now it is
>> "If read_screenpad_backlight_power() returns -ENODEV, stop and return
>> no error (let the rest of the code continue). If it returns another error,
>> return it".
>> Changing the code as suggested above changes the semantics to
>> "If read_screenpad_backlight_power() returns an error, always return it".
>>
>> Looking at the patch, I don't have a definite answer.
>> asus_screenpad_init() happily registers the backlight if
>> read_screenpad_backlight_power() returns -ENODEV. One could argue that
>> the other functions should thus handle that situation gracefully as well,
>> but read_screenpad_brightness() does return -ENODEV in that situation.
>> I think you should decide how you want to handle that case and handle
>> it consistently across all functions.
>>
>> Either case, there is another problem in asus_screenpad_init():
>> If read_screenpad_brightness() fails, the function returns an error,
>> but does not unregister the backlight device.
>>
>> Guenter
>>
> 
> 
> Thanks mate. I was working on this between my workload and getting a few users to test. My first priority was to get the thing working for them and as such I didn't put much thought in to errors and consistency.
> 
> I think since `asus_screenpad_init()` is not called unless the brightness WMI method exists, then it and the other functions should return all errors if the power WMI method fails at all since the device functionality is compromised. If there is a hardware change in this aspect we could revisit it then.
> 
That would suggest that asus_screenpad_init() should also abort on all errors
and not assign a power of 'FB_BLANK_UNBLANK' if it is unreadable.

>  > Either case, there is another problem in asus_screenpad_init():
>  > If read_screenpad_brightness() fails, the function returns an error,
>  > but does not unregister the backlight device.
> 
> I wasn't entirely sure how to handle this. Mostly I followed what the existing backlight code was doing, I've now added a `goto fail_screenpad;` with:
> fail_screenpad:
>      if (asus->screenpad_backlight_device)
>          asus_screenpad_exit(asus);
> 
> really I'm relying on others to guide me with this - I know I've got a fair few patches in these days but they've been mostly simple things except for TUF RGB, and I've had to learn a bunch of stuff as I go.
> 
> I'll await your response before I send in a V3.
> 

Personally I'd first try to use devm_backlight_device_register().
As for error handling, the code would normally be something like

fail_screenpad:
	backlight_device_unregister(asus->screenpad_backlight_device);

or better

fail_screenpad:
	backlight_device_unregister(bd);

I don't know why you would want to check if asus->screenpad_backlight
is NULL because the code would never be called with it being NULL.

Note that I don't see why
	asus->screenpad_backlight_device = NULL;
in asus_screenpad_exit() would be necessary.

Guenter

Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]

On Sat, 6 May 2023, Luke Jones wrote:
> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Fri, 5 May 2023, Luke D. Jones wrote:
> > 
> > >  Add support for the WMI methods used to turn off and adjust the
> > >  brightness of the secondary "screenpad" device found on some high-end
> > >  ASUS laptops like the GX650P series and others.
> > > 
> > >  These methods are utilised in a new backlight device named:
> > >  - asus_screenpad
> > > 
> > >  Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > >  ---

> > >  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
> > >   	return 0;
> > >   }
> > > 
> > >  +/* Screenpad backlight */
> > >  +
> > >  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
> > >  +{
> > >  +	int ret = asus_wmi_get_devstate_simple(asus,
> > > ASUS_WMI_DEVID_SCREENPAD_POWER);
> > 
> > Please move this to own line because now you have the extra newline
> > in between the call and error handling.
> 
> I don't understand what you mean sorry. Remove the new line or:
> int ret;
> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);

Don't do:

int func()
{
	int ret = func();

	if (ret < 0)
		return ret;
	...
}

But do:

int func()
{
	int ret;

	ret = func();
	if (ret < 0)
		return ret;
	...
}

This keeps the error handling next to the actual call following the usual 
error handling pattern (natural logic grouping). The added clarity is well 
worth the one extra line required.

> > >  +static int update_screenpad_bl_status(struct backlight_device *bd)
> > >  +{
> > >  +	struct asus_wmi *asus = bl_get_data(bd);
> > >  +	int power, err = 0;
> > >  +	u32 ctrl_param;
> > >  +
> > >  +	power = read_screenpad_backlight_power(asus);
> > >  +	if (power == -ENODEV)
> > >  +		return err;
> > 
> > Just return 0. Or is there perhaps something wrong/missing here?
> 
> I thought the correct thing was to return any possible error state (here,
> anything less than 0 would be an error, right?)

I think this was covered already but here what I meant, use either:
	return 0;
because err is always 0 at that point, or:
	return power;
Depending on which is correct, I wasn't sure because you had this after 
it:

> > >  +	else if (power < 0)
> > >  +		return power;

...So I thought you might really intentionally wanted to return 0 there.


-- 
 i.

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Hans de Goede]

Hi,

On 5/6/23 13:52, Hans de Goede wrote:
> Hi Luke,
> 
> On 5/5/23 06:30, Luke D. Jones wrote:
>> Adds support for the screenpad(-plus) found on a few ASUS laptops that have a main 16:9 or 16:10 screen and a shorter screen below the main but above the keyboard.
>> The support consists of:
>> - On off control
>> - Setting brightness from 0-255
>>
>> There are some small quirks with this device when considering only the raw WMI methods:
>> 1. The Off method can only switch the device off
>> 2. Changing the brightness turns the device back on
>> 3. To turn the device back on the brightness must be > 1
>> 4. When the device is off the brightness can't be changed (so it is stored by the driver if device is off).
>> 5. Booting with a value of 0 brightness (retained by bios) means the bios will set a value of > 0, < 15 which is far too dim and was unexpected by testers. The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15.
>> 6. When the device is off it is "unplugged"
>>
>> All of the above points are addressed within the patch to create a good user experience and keep within user expectations.
>>
>> Changelog:
>> - V2
>>   - Complete refactor to use as a backlight device
> 
> Thank you on your work for this.
> 
> Unfortunately I did not get a chance to react to the v1 posting and
> the remarks to switch to using /sys/class/backlight there before you
> posted this v2.
> 
> Technically the remark to use /sys/class/backlight for this is
> completely correct. But due to the way how userspace uses
> /sys/class/backlight this is a problematic.
> 
> Userspace basically always assumes there is only 1 LCD panel
> and it then looks at /sys/class/backlight and picks 1
> /sys/class/backlight entry and uses that for the brightness
> slider in the desktop-environment UI / system-menu as well
> as to handle brightness up/down keyboard hotkey presses.
> 
> In the (recent) past the kernel used to register e.g.
> both /sys/class/backlight/acpi_video0 and
> /sys/class/backlight/intel_backlight
> 
> For ACPI resp. direct hw control of the LCD panel backlight
> (so both control the same backlight, sometimes both work
> sometimes only 1 works).
> 
> Userspace uses the backlight-type to determine which backlight
> class to use, using (for GNOME, but I believe everywhere) the
> following preference order:
> 
> 1. First look for "firmware" type backlight devices (like acpi_video0)
> 2. Then try "platform" type backlight devices
> 3. Last try "raw" type backlight devices
> 
> And to make things work the kernel has been hiding the "acpi_video0"
> entry in cases where it is known that we need the "raw" aka native
> type backlight.
> 
> Luke you seem to already be partly aware of this, because the patch
> now has this:
> 
> 	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
> 
> but almost all modern laptops exclusively use the raw/native type
> for backlight control of the main LCD panel.
> 
> So now we end up with 2 "raw" type backlight devices and if
> e.g. gnome-settings-daemon picks the right one now sort of
> is left to luck.
> 
> Well that is not entirely true, at least gnome-settings-daemon
> prefers raw backlight devices where the parent has an "enabled"
> sysfs attribute (it expects the parent to be a drm_connector
> object) and where that enabled attribute reads as "enabled".
> 
> This is done for hybrid-gfx laptops where there already may
> be 2 raw backlight-class devices, 1 for each GPU but only
> 1 of the 2 drm_connectors going to the main LCD panel should
> actually show as enabled.
> 
> So typing all this out I guess we could go ahead with using
> the backlight class for this after all, but this relies
> on userspace preferring raw backlight-class devices
> with a drm_connector-object parent which show as being
> enabled.
> 
> Any userspace code which does not do the parent has
> an enabled attr reading "enabled" or a similar check
> will end up picking a random backlight class device
> as control for the main panel brightness which will not
> always end well. So this all is a bit fragile ...
> 
> And I'm not sure what is the best thing to do here.
> 
> Barnabás, Ilpo, Guenter, any comments on this ?

Hmm, no comments from anyone on the potential problems of using
/sys/class/backlight for this causing potential userspace confusion
since normally /sys/class/backlight devices control the main LCD
brightness ?

Luke do you have any thoughts on this yourself ?

And can you answer this question please ?  :

> Luke, question how does the second/exta panel look
> from an outputting video to it pov ?  Does it show
> up as an extra screen connected to a drm_connector
> on one of the GPUs. IOW can it be used with standard
> kernel-modesetting APIs ?

Regards,

Hans

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Mon, May 15 2023 at 14:39:10 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 5/6/23 13:52, Hans de Goede wrote:
>>  Hi Luke,
>> 
>>  On 5/5/23 06:30, Luke D. Jones wrote:
>>>  Adds support for the screenpad(-plus) found on a few ASUS laptops 
>>> that have a main 16:9 or 16:10 screen and a shorter screen below 
>>> the main but above the keyboard.
>>>  The support consists of:
>>>  - On off control
>>>  - Setting brightness from 0-255
>>> 
>>>  There are some small quirks with this device when considering only 
>>> the raw WMI methods:
>>>  1. The Off method can only switch the device off
>>>  2. Changing the brightness turns the device back on
>>>  3. To turn the device back on the brightness must be > 1
>>>  4. When the device is off the brightness can't be changed (so it 
>>> is stored by the driver if device is off).
>>>  5. Booting with a value of 0 brightness (retained by bios) means 
>>> the bios will set a value of > 0, < 15 which is far too dim and was 
>>> unexpected by testers. The compromise was to set the brightness to 
>>> 60 which is a usable brightness if the module init brightness was 
>>> under 15.
>>>  6. When the device is off it is "unplugged"
>>> 
>>>  All of the above points are addressed within the patch to create a 
>>> good user experience and keep within user expectations.
>>> 
>>>  Changelog:
>>>  - V2
>>>    - Complete refactor to use as a backlight device
>> 
>>  Thank you on your work for this.
>> 
>>  Unfortunately I did not get a chance to react to the v1 posting and
>>  the remarks to switch to using /sys/class/backlight there before you
>>  posted this v2.
>> 
>>  Technically the remark to use /sys/class/backlight for this is
>>  completely correct. But due to the way how userspace uses
>>  /sys/class/backlight this is a problematic.
>> 
>>  Userspace basically always assumes there is only 1 LCD panel
>>  and it then looks at /sys/class/backlight and picks 1
>>  /sys/class/backlight entry and uses that for the brightness
>>  slider in the desktop-environment UI / system-menu as well
>>  as to handle brightness up/down keyboard hotkey presses.
>> 

IMO, desktops need to adjust this expectation and start offering 
controls for all possible screens. This would open up the possibility 
of setting brightness of modern external screens also. And then they 
should use the "Primary display" brightness controls, or at least offer 
an option to set which is controlled.

>> 
>>  In the (recent) past the kernel used to register e.g.
>>  both /sys/class/backlight/acpi_video0 and
>>  /sys/class/backlight/intel_backlight
>> 
>>  For ACPI resp. direct hw control of the LCD panel backlight
>>  (so both control the same backlight, sometimes both work
>>  sometimes only 1 works).
>> 
>>  Userspace uses the backlight-type to determine which backlight
>>  class to use, using (for GNOME, but I believe everywhere) the
>>  following preference order:
>> 
>>  1. First look for "firmware" type backlight devices (like 
>> acpi_video0)
>>  2. Then try "platform" type backlight devices
>>  3. Last try "raw" type backlight devices
>> 
>>  And to make things work the kernel has been hiding the "acpi_video0"
>>  entry in cases where it is known that we need the "raw" aka native
>>  type backlight.
>> 
>>  Luke you seem to already be partly aware of this, because the patch
>>  now has this:
>> 
>>  	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked 
>> */
>> 
>>  but almost all modern laptops exclusively use the raw/native type
>>  for backlight control of the main LCD panel.
>> 
>>  So now we end up with 2 "raw" type backlight devices and if
>>  e.g. gnome-settings-daemon picks the right one now sort of
>>  is left to luck.
>> 

In a test KDE at least picked the right one.

>> 
>>  Well that is not entirely true, at least gnome-settings-daemon
>>  prefers raw backlight devices where the parent has an "enabled"
>>  sysfs attribute (it expects the parent to be a drm_connector
>>  object) and where that enabled attribute reads as "enabled".
>> 

Ah I see. Parent for screenpad is "platform", while the main is on igpu 
with parent "pci". I will paste some udev info at the end of this reply.

>> 
>>  This is done for hybrid-gfx laptops where there already may
>>  be 2 raw backlight-class devices, 1 for each GPU but only
>>  1 of the 2 drm_connectors going to the main LCD panel should
>>  actually show as enabled.
>> 
>>  So typing all this out I guess we could go ahead with using
>>  the backlight class for this after all, but this relies
>>  on userspace preferring raw backlight-class devices
>>  with a drm_connector-object parent which show as being
>>  enabled.
>> 
>>  Any userspace code which does not do the parent has
>>  an enabled attr reading "enabled" or a similar check
>>  will end up picking a random backlight class device
>>  as control for the main panel brightness which will not
>>  always end well. So this all is a bit fragile ...
>> 
>>  And I'm not sure what is the best thing to do here.
>> 
>>  Barnabás, Ilpo, Guenter, any comments on this ?
> 
> Hmm, no comments from anyone on the potential problems of using
> /sys/class/backlight for this causing potential userspace confusion
> since normally /sys/class/backlight devices control the main LCD
> brightness ?
> 
> Luke do you have any thoughts on this yourself ?
> 
> And can you answer this question please ?  :
> 
>>  Luke, question how does the second/exta panel look
>>  from an outputting video to it pov ?  Does it show
>>  up as an extra screen connected to a drm_connector
>>  on one of the GPUs. IOW can it be used with standard
>>  kernel-modesetting APIs ?
> 

Hi Hans, sorry about delay in response, just been tied up with work.

As I don't actually have this kind of laptop I can't easily get info, 
but I can ask a few people in my discord for information. Is there 
anything in particular you would need to know? From my basic 
understanding some of the points are:

1. It does show as an actual additional screen
2. Internal wiring is unclear, when dispaly MUX is switched to dgpu the 
screen is still detected but not drawn to
3. Point 2 is actually more uncertain as it seems only wayland had this 
issue? I will get more info.

So I think now is probably a good time to raise a particular issue I've 
encountered with the last two years: the display MUX.

As I understand it now, there are two types of new MUX - the manual 
switch, and the newer "Advanced Optimus" automatic switch. The issues I 
have are with the manual switch since I've not encountered the advanced 
optimus yet.

When the switch is. uh. switched. the dgpu drives the internal display, 
and I expect that since the display is now detected through the dgpu, 
this is why the dgpu is kept awake to drive it. But, the igpu is also 
still active, and because of this the initial boot from grub to 
display-manager is a black screen including tty. This means anyone with 
an encrypted drive will never see the prompt and they believe they have 
a failed boot. I don't know what to do about this?

What I would love is somehow to either disable the igpu in kernel if 
the MUX is toggled, or to change which device is the primary. Do you 
have any thoughts on where I should start on this?

An additional problem: `boot_vga` property of display adaptors. I've 
been using this as a first-stage check in supergfxctl to determine if 
there was a switch, but it is never ever reliable - sometimes it 
changes, sometimes it is entirely blank (using udev to fetch 
properties). And then I need to use a combination of checks to 
determine state. So this `boot_vga` seems to always be available but is 
practically unusable.



UDEV info dumps:

Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1",
    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1",
    device_node: None,
    subsystem_name: Some(
        "backlight",
    ),
    system_name: "amdgpu_bl1",
    instance_number: Some(
        1,
    ),
    device_type: None,
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: None,
            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
            device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
            device_node: None,
            subsystem_name: Some(
                "pci",
            ),
            system_name: "0000:09:00.0",
            instance_number: Some(
                0,
            ),
            device_type: None,
            driver: Some(
                "amdgpu",
            ),
            action: None,
            parent: Some(
                Device {
                    initialized: true,
                    device_major_minor_number: None,
                    system_path: "/sys/devices/pci0000:00/0000:00:08.1",
                    device_path: "/devices/pci0000:00/0000:00:08.1",
                    device_node: None,
                    subsystem_name: Some(
                        "pci",
                    ),
                    system_name: "0000:00:08.1",
                    instance_number: Some(
                        1,
                    ),
                    device_type: None,
                    driver: Some(
                        "pcieport",
                    ),
                    action: None,
                    parent: Some(
                        Device {
                            initialized: false,
                            device_major_minor_number: None,
                            system_path: "/sys/devices/pci0000:00",
                            device_path: "/devices/pci0000:00",
                            device_node: None,
                            subsystem_name: None,
                            system_name: "pci0000:00",
                            instance_number: Some(
                                0,
                            ),
                            device_type: None,
                            driver: None,
                            action: None,
                            parent: None,
                        },
                    ),
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":systemd:seat:"
    - "DEVPATH" 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "backlight-pci-0000_09_00_0"
    - "ID_PATH" "pci-0000:09:00.0"
    - "ID_PATH_TAG" "pci-0000_09_00_0"
    - "NVME_HOST_IFACE" "none"
    - "SUBSYSTEM" "backlight"
    - "SYSTEMD_WANTS" "systemd-backlight@backlight:amdgpu_bl1.service"
    - "TAGS" ":systemd:seat:"
    - "USEC_INITIALIZED" "6189554"
  [attributes]
    - "actual_brightness" ""
    - "bl_power" ""
    - "brightness" ""
    - "device" ""
    - "max_brightness" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "scale" ""
    - "subsystem" ""
    - "type" ""
    - "uevent" ""

Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/platform/asus-nb-wmi/backlight/asus_screenpad",
    device_path: 
"/devices/platform/asus-nb-wmi/backlight/asus_screenpad",
    device_node: None,
    subsystem_name: Some(
        "backlight",
    ),
    system_name: "asus_screenpad",
    instance_number: None,
    device_type: None,
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: None,
            system_path: "/sys/devices/platform/asus-nb-wmi",
            device_path: "/devices/platform/asus-nb-wmi",
            device_node: None,
            subsystem_name: Some(
                "platform",
            ),
            system_name: "asus-nb-wmi",
            instance_number: None,
            device_type: None,
            driver: Some(
                "asus-nb-wmi",
            ),
            action: None,
            parent: Some(
                Device {
                    initialized: false,
                    device_major_minor_number: None,
                    system_path: "/sys/devices/platform",
                    device_path: "/devices/platform",
                    device_node: None,
                    subsystem_name: None,
                    system_name: "platform",
                    instance_number: None,
                    device_type: None,
                    driver: None,
                    action: None,
                    parent: None,
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":systemd:seat:"
    - "DEVPATH" "/devices/platform/asus-nb-wmi/backlight/asus_screenpad"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "backlight-platform-asus-nb-wmi"
    - "ID_PATH" "platform-asus-nb-wmi"
    - "ID_PATH_TAG" "platform-asus-nb-wmi"
    - "NVME_HOST_IFACE" "none"
    - "SUBSYSTEM" "backlight"
    - "SYSTEMD_WANTS" 
"systemd-backlight@backlight:asus_screenpad.service"
    - "TAGS" ":systemd:seat:"
    - "USEC_INITIALIZED" "6323833"
  [attributes]
    - "actual_brightness" ""
    - "bl_power" ""
    - "brightness" ""
    - "device" ""
    - "max_brightness" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "scale" ""
    - "subsystem" ""
    - "type" ""
    - "uevent" ""


Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1",
    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1",
    device_node: None,
    subsystem_name: Some(
        "drm",
    ),
    system_name: "card1-DP-1",
    instance_number: Some(
        1,
    ),
    device_type: Some(
        "drm_connector",
    ),
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: Some(
                57857,
            ),
            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1",
            device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1",
            device_node: Some(
                "/dev/dri/card1",
            ),
            subsystem_name: Some(
                "drm",
            ),
            system_name: "card1",
            instance_number: Some(
                1,
            ),
            device_type: Some(
                "drm_minor",
            ),
            driver: None,
            action: None,
            parent: Some(
                Device {
                    initialized: true,
                    device_major_minor_number: None,
                    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
                    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
                    device_node: None,
                    subsystem_name: Some(
                        "pci",
                    ),
                    system_name: "0000:09:00.0",
                    instance_number: Some(
                        0,
                    ),
                    device_type: None,
                    driver: Some(
                        "amdgpu",
                    ),
                    action: None,
                    parent: Some(
                        Device {
                            initialized: true,
                            device_major_minor_number: None,
                            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1",
                            device_path: 
"/devices/pci0000:00/0000:00:08.1",
                            device_node: None,
                            subsystem_name: Some(
                                "pci",
                            ),
                            system_name: "0000:00:08.1",
                            instance_number: Some(
                                1,
                            ),
                            device_type: None,
                            driver: Some(
                                "pcieport",
                            ),
                            action: None,
                            parent: Some(
                                Device {
                                    initialized: false,
                                    device_major_minor_number: None,
                                    system_path: 
"/sys/devices/pci0000:00",
                                    device_path: "/devices/pci0000:00",
                                    device_node: None,
                                    subsystem_name: None,
                                    system_name: "pci0000:00",
                                    instance_number: Some(
                                        0,
                                    ),
                                    device_type: None,
                                    driver: None,
                                    action: None,
                                    parent: None,
                                },
                            ),
                        },
                    ),
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":master-of-seat:seat:"
    - "DEVPATH" 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1"
    - "DEVTYPE" "drm_connector"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "drm-pci-0000_09_00_0"
    - "ID_PATH" "pci-0000:09:00.0"
    - "ID_PATH_TAG" "pci-0000_09_00_0"
    - "SUBSYSTEM" "drm"
    - "TAGS" ":master-of-seat:seat:"
    - "USEC_INITIALIZED" "6196005"
  [attributes]
    - "ddc" ""
    - "device" ""
    - "dpms" ""
    - "edid" ""
    - "enabled" ""
    - "modes" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "status" ""
    - "subsystem" ""
    - "uevent" ""
> 

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Hans de Goede]

Hi Luke,

On 5/16/23 00:34, Luke Jones wrote:
> On Mon, May 15 2023 at 14:39:10 +0200, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>>>  Thank you on your work for this.
>>>
>>>  Unfortunately I did not get a chance to react to the v1 posting and
>>>  the remarks to switch to using /sys/class/backlight there before you
>>>  posted this v2.
>>>
>>>  Technically the remark to use /sys/class/backlight for this is
>>>  completely correct. But due to the way how userspace uses
>>>  /sys/class/backlight this is a problematic.
>>>
>>>  Userspace basically always assumes there is only 1 LCD panel
>>>  and it then looks at /sys/class/backlight and picks 1
>>>  /sys/class/backlight entry and uses that for the brightness
>>>  slider in the desktop-environment UI / system-menu as well
>>>  as to handle brightness up/down keyboard hotkey presses.
>>>
> 
> IMO, desktops need to adjust this expectation and start offering controls for all possible screens. This would open up the possibility of setting brightness of modern external screens also. And then they should use the "Primary display" brightness controls, or at least offer an option to set which is controlled.

Right this is what the proposal at:

https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

is about. ATM userspace cannot reliably determine which
/sys/class/backlight device belongs to which screen /
video-output . So before desktops can offer this functionality
we first need to fix the kernel <-> userspace APIs for this.

<snip>

>>>  So now we end up with 2 "raw" type backlight devices and if
>>>  e.g. gnome-settings-daemon picks the right one now sort of
>>>  is left to luck.
>>>
> 
> In a test KDE at least picked the right one.

That is good to know I'm still not entirely convinced using
/sys/class/backlight for this is a good idea though. 

See below.

>>>  Well that is not entirely true, at least gnome-settings-daemon
>>>  prefers raw backlight devices where the parent has an "enabled"
>>>  sysfs attribute (it expects the parent to be a drm_connector
>>>  object) and where that enabled attribute reads as "enabled".
>>>
> 
> Ah I see. Parent for screenpad is "platform", while the main is on igpu with parent "pci". I will paste some udev info at the end of this reply.

Actually for the backlight-device on the iGPU the parent
should be a drm-connector for the eDP (and the parent of
that is the iGPU PCI device). Note I did not check
the udev dump.

<snip>

>>>  Luke, question how does the second/exta panel look
>>>  from an outputting video to it pov ?  Does it show
>>>  up as an extra screen connected to a drm_connector
>>>  on one of the GPUs. IOW can it be used with standard
>>>  kernel-modesetting APIs ?
>>
> 
> Hi Hans, sorry about delay in response, just been tied up with work.
> 
> As I don't actually have this kind of laptop I can't easily get info, but I can ask a few people in my discord for information. Is there anything in particular you would need to know? From my basic understanding some of the points are:
> 
> 1. It does show as an actual additional screen
> 2. Internal wiring is unclear, when dispaly MUX is switched to dgpu the screen is still detected but not drawn to
> 3. Point 2 is actually more uncertain as it seems only wayland had this issue? I will get more info.

Right, so I think we first need to better understand the interactions between the WMI calls you are making and the drm/kms interface.

Question 1:

If you turn the second screen off through WMI, does it get seen as disconnected by the drm/kms driver then. Or does the drm/kms driver just go on treating it as an extra connected display, still drawing now no longer visible content to it ?

IOW does the desktop environment's monitor-config panel no longer show the extra display after disabling it through WMI?

The best way to check this is look under /sys/class/drm and find out which /sys/class/drm/card#-<conn-type>-# entry belongs to the extra panel. Step 1 check for all card#-<conn-type>-# entries
where status returns connected, e.g. :

[hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/status 
connected

Step 2: for the connected ones cat the modes, e.g.:

[hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/modes
1920x1080
1600x1200
...

And find the one which matches with the resolution of the extra panel (the one which does not match with the resolution of the main panel).

Then turn the extra panel of through WMI and cat the status attribute again. If that still reads connected then that means the desktop environment keeps seeing an extra display output which is not ideal. This will e.g. cause any windows which were on the extra panel to stay there, even though they are no longer visible.


Question 2:

If you turn the second screen off through drm/kms, using the desktop environments monitor config panel does this also turn off the backlight ?

After disabling the screen in the desktop environments monitor config check that the enabled attribute, e.g. cat /sys/class/drm/card1-DP-1/enabled shows disabled and after verifying this look at the extra screen in a dark room, do you see any backlight bleed indicating the backlight is still  on?


We really want the backlight on/off state and the drm-connector enabled state to match. My proposal from above will allow this once implemented. Until we can hook this all up nicely I think it might be better to just go with the custom sysfs attributes from your v1 patch rather then adding a /sys/class/backlight device for this.


> So I think now is probably a good time to raise a particular issue I've encountered with the last two years: the display MUX.
> 
> As I understand it now, there are two types of new MUX - the manual switch, and the newer "Advanced Optimus" automatic switch. The issues I have are with the manual switch since I've not encountered the advanced optimus yet.
> 
> When the switch is. uh. switched. the dgpu drives the internal display, and I expect that since the display is now detected through the dgpu, this is why the dgpu is kept awake to drive it. But, the igpu is also still active, and because of this the initial boot from grub to display-manager is a black screen including tty. This means anyone with an encrypted drive will never see the prompt and they believe they have a failed boot. I don't know what to do about this?

Is this with EFI booting or with classic BIOS boot? With EFI booting the EFIFB should be put on the right GPU by the firmware. So I suspect this is with classic BIOS boot?

I think the best thing to do here is to just use EFI on machines like this. That or put grub in text mode so that it makes BIOS calls to display text. Using GRUB_TERMINAL_OUTPUT=gfxterm combined with classic BIOS booting will make grub try to directly drive the gfx card itself and I'm not surprised that it gets that wrong in this case.

Note I think that just using EFI is prefered over switching grub to GRUB_TERMINAL_OUTPUT=console. I would expect GRUB_TERMINAL_OUTPUT=console to also work but I'm not sure. I don't think that the classic BIOS boot stuff is still tested by laptop vendors and esp. not tested with non standard BIOS settings ...


> What I would love is somehow to either disable the igpu in kernel if the MUX is toggled, or to change which device is the primary. Do you have any thoughts on where I should start on this?

Not really, on the hybrid gfx devices which I have when optimus is disabled in the BIOS, muxing the main LCD to the Nvidia GPU the iGPU gets disabled by the BIOS.

If the iGPU in these models does not get disabled then I guess it is still needed for some connectors, e.g. maybe for displays connected to one of the thunderbolt ports ?

Or maybe it is left on now a days for things like Intel quicksync ?

> An additional problem: `boot_vga` property of display adaptors. I've been using this as a first-stage check in supergfxctl to determine if there was a switch, but it is never ever reliable - sometimes it changes, sometimes it is entirely blank (using udev to fetch properties). And then I need to use a combination of checks to determine state. So this `boot_vga` seems to always be available but is practically unusable.

I'm afraid I cannot really help here. I think most of these laptops ship in Optimus mode (so both GPUs enabled) by default. And the non optimus mode is likely only tested under certain circumstances so there are going to be firmware bugs here. And Linux' code for detecting this likely also has issues of its own. Combine the 2 and you get lots of "fun".

I guess this is also what trips up grub. I wonder if this is better / more reliable under EFI mode though? Esp. since that is what vendors actually QA/test.

Regards,

Hans

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Thu, 2023-05-25 at 13:09 +0200, Hans de Goede wrote:
> Hi Luke,
> 
> On 5/16/23 00:34, Luke Jones wrote:
> > On Mon, May 15 2023 at 14:39:10 +0200, Hans de Goede
> > <hdegoede@redhat.com> wrote:
> 
> <snip>
> 
> > > >  Thank you on your work for this.
> > > > 
> > > >  Unfortunately I did not get a chance to react to the v1
> > > > posting and
> > > >  the remarks to switch to using /sys/class/backlight there
> > > > before you
> > > >  posted this v2.
> > > > 
> > > >  Technically the remark to use /sys/class/backlight for this is
> > > >  completely correct. But due to the way how userspace uses
> > > >  /sys/class/backlight this is a problematic.
> > > > 
> > > >  Userspace basically always assumes there is only 1 LCD panel
> > > >  and it then looks at /sys/class/backlight and picks 1
> > > >  /sys/class/backlight entry and uses that for the brightness
> > > >  slider in the desktop-environment UI / system-menu as well
> > > >  as to handle brightness up/down keyboard hotkey presses.
> > > > 
> > 
> > IMO, desktops need to adjust this expectation and start offering
> > controls for all possible screens. This would open up the
> > possibility of setting brightness of modern external screens also.
> > And then they should use the "Primary display" brightness controls,
> > or at least offer an option to set which is controlled.
> 
> Right this is what the proposal at:
> 
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
> 
> is about. ATM userspace cannot reliably determine which
> /sys/class/backlight device belongs to which screen /
> video-output . So before desktops can offer this functionality
> we first need to fix the kernel <-> userspace APIs for this.
> 
> <snip>
> 
> > > >  So now we end up with 2 "raw" type backlight devices and if
> > > >  e.g. gnome-settings-daemon picks the right one now sort of
> > > >  is left to luck.
> > > > 
> > 
> > In a test KDE at least picked the right one.
> 
> That is good to know I'm still not entirely convinced using
> /sys/class/backlight for this is a good idea though. 
> 
> See below.
> 
> > > >  Well that is not entirely true, at least gnome-settings-daemon
> > > >  prefers raw backlight devices where the parent has an
> > > > "enabled"
> > > >  sysfs attribute (it expects the parent to be a drm_connector
> > > >  object) and where that enabled attribute reads as "enabled".
> > > > 
> > 
> > Ah I see. Parent for screenpad is "platform", while the main is on
> > igpu with parent "pci". I will paste some udev info at the end of
> > this reply.
> 
> Actually for the backlight-device on the iGPU the parent
> should be a drm-connector for the eDP (and the parent of
> that is the iGPU PCI device). Note I did not check
> the udev dump.
> 
> <snip>
> 
> > > >  Luke, question how does the second/exta panel look
> > > >  from an outputting video to it pov ?  Does it show
> > > >  up as an extra screen connected to a drm_connector
> > > >  on one of the GPUs. IOW can it be used with standard
> > > >  kernel-modesetting APIs ?
> > > 
> > 
> > Hi Hans, sorry about delay in response, just been tied up with
> > work.
> > 
> > As I don't actually have this kind of laptop I can't easily get
> > info, but I can ask a few people in my discord for information. Is
> > there anything in particular you would need to know? From my basic
> > understanding some of the points are:
> > 
> > 1. It does show as an actual additional screen
> > 2. Internal wiring is unclear, when dispaly MUX is switched to dgpu
> > the screen is still detected but not drawn to
> > 3. Point 2 is actually more uncertain as it seems only wayland had
> > this issue? I will get more info.
> 
> Right, so I think we first need to better understand the interactions
> between the WMI calls you are making and the drm/kms interface.
> 
> Question 1:
> 
> If you turn the second screen off through WMI, does it get seen as
> disconnected by the drm/kms driver then. Or does the drm/kms driver
> just go on treating it as an extra connected display, still drawing
> now no longer visible content to it ?

It is most certainly viewed as disconnected.

> IOW does the desktop environment's monitor-config panel no longer
> show the extra display after disabling it through WMI?

That is correct, it is no-longer a connected and visible display

> 
> The best way to check this is look under /sys/class/drm and find out
> which /sys/class/drm/card#-<conn-type>-# entry belongs to the extra
> panel. Step 1 check for all card#-<conn-type>-# entries
> where status returns connected, e.g. :
> 
> [hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/status 
> connected
> 

On disable the status does change to:
cat /sys/class/drm/card1-DP-5/enabled
disabled

> Step 2: for the connected ones cat the modes, e.g.:
> 
> [hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/modes
> 1920x1080
> 1600x1200
> ...
> 
> And find the one which matches with the resolution of the extra panel
> (the one which does not match with the resolution of the main panel).
> 
> Then turn the extra panel of through WMI and cat the status attribute
> again. If that still reads connected then that means the desktop
> environment keeps seeing an extra display output which is not ideal.
> This will e.g. cause any windows which were on the extra panel to
> stay there, even though they are no longer visible.
> 
> 
> Question 2:
> 
> If you turn the second screen off through drm/kms, using the desktop
> environments monitor config panel does this also turn off the
> backlight ?

The screen is dark but there is still some backlight coming out of it.
I think this means I need to add a small pre-off to the patch to ensure
backlight is fully off when display is turned off.

> 
> After disabling the screen in the desktop environments monitor config
> check that the enabled attribute, e.g. cat /sys/class/drm/card1-DP-
> 1/enabled shows disabled and after verifying this look at the extra
> screen in a dark room, do you see any backlight bleed indicating the
> backlight is still  on?
> 

Shows as disabled

> 
> We really want the backlight on/off state and the drm-connector
> enabled state to match. My proposal from above will allow this once
> implemented. Until we can hook this all up nicely I think it might be
> better to just go with the custom sysfs attributes from your v1 patch
> rather then adding a /sys/class/backlight device for this.
> 

I would like to go with the backlight patch as it seems more likely I
can adjust this without breaking userspace if required in future. The
WMI controls behave as expected to.

> 
> > So I think now is probably a good time to raise a particular issue
> > I've encountered with the last two years: the display MUX.
> > 
> > As I understand it now, there are two types of new MUX - the manual
> > switch, and the newer "Advanced Optimus" automatic switch. The
> > issues I have are with the manual switch since I've not encountered
> > the advanced optimus yet.
> > 
> > When the switch is. uh. switched. the dgpu drives the internal
> > display, and I expect that since the display is now detected
> > through the dgpu, this is why the dgpu is kept awake to drive it.
> > But, the igpu is also still active, and because of this the initial
> > boot from grub to display-manager is a black screen including tty.
> > This means anyone with an encrypted drive will never see the prompt
> > and they believe they have a failed boot. I don't know what to do
> > about this?
> 
> Is this with EFI booting or with classic BIOS boot? With EFI booting
> the EFIFB should be put on the right GPU by the firmware. So I
> suspect this is with classic BIOS boot?

No this is with EFI boot always. I'm not even sure these machines still
have the ability to boot oldschool bios style.

> 
> I think the best thing to do here is to just use EFI on machines like
> this. That or put grub in text mode so that it makes BIOS calls to
> display text. Using GRUB_TERMINAL_OUTPUT=gfxterm combined with
> classic BIOS booting will make grub try to directly drive the gfx
> card itself and I'm not surprised that it gets that wrong in this
> case.
> Note I think that just using EFI is prefered over switching grub to
> GRUB_TERMINAL_OUTPUT=console. I would expect
> GRUB_TERMINAL_OUTPUT=console to also work but I'm not sure. I don't
> think that the classic BIOS boot stuff is still tested by laptop
> vendors and esp. not tested with non standard BIOS settings ...

The grub gfx mode is GRUB_TERMINAL_OUTPUT="console", fedora default in
all cases here. Grub itself shows fine when the MUX mode is in dgpu
mode (aka, internal display connected to dgpu).

> > What I would love is somehow to either disable the igpu in kernel
> > if the MUX is toggled, or to change which device is the primary. Do
> > you have any thoughts on where I should start on this?
> 
> Not really, on the hybrid gfx devices which I have when optimus is
> disabled in the BIOS, muxing the main LCD to the Nvidia GPU the iGPU
> gets disabled by the BIOS.
> 
> If the iGPU in these models does not get disabled then I guess it is
> still needed for some connectors, e.g. maybe for displays connected
> to one of the thunderbolt ports ?

Yeah seems likely. I'll have to actually check this (I have TB4
connected monitor).

> Or maybe it is left on now a days for things like Intel quicksync ?
> 
> > An additional problem: `boot_vga` property of display adaptors.
> > I've been using this as a first-stage check in supergfxctl to
> > determine if there was a switch, but it is never ever reliable -
> > sometimes it changes, sometimes it is entirely blank (using udev to
> > fetch properties). And then I need to use a combination of checks
> > to determine state. So this `boot_vga` seems to always be available
> > but is practically unusable.
> 
> I'm afraid I cannot really help here. I think most of these laptops
> ship in Optimus mode (so both GPUs enabled) by default. And the non
> optimus mode is likely only tested under certain circumstances so
> there are going to be firmware bugs here. And Linux' code for
> detecting this likely also has issues of its own. Combine the 2 and
> you get lots of "fun".
> 
> I guess this is also what trips up grub. I wonder if this is better /
> more reliable under EFI mode though? Esp. since that is what vendors
> actually QA/test.

As mentioned, the machines are all booting in EFI mode right from the
start - the old mode (CMA?) doesn't seem to exist at all.

I really would like to find a solution here as it is tripping up a lot
of new users.

Cheers,
Luke.

P.S> I will go ahead with the screen patch as it looks like this
behaves as expected minus the need to turn off BL before turn off
display. I will submit revision later in the week.

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Hans de Goede]

Hi Luke,

On 6/4/23 06:52, Luke Jones wrote:
> On Thu, 2023-05-25 at 13:09 +0200, Hans de Goede wrote:

<snip>

>> Right, so I think we first need to better understand the interactions
>> between the WMI calls you are making and the drm/kms interface.
>>
>> Question 1:
>>
>> If you turn the second screen off through WMI, does it get seen as
>> disconnected by the drm/kms driver then. Or does the drm/kms driver
>> just go on treating it as an extra connected display, still drawing
>> now no longer visible content to it ?
> 
> It is most certainly viewed as disconnected.

Ok, that is good.

>> IOW does the desktop environment's monitor-config panel no longer
>> show the extra display after disabling it through WMI?
> 
> That is correct, it is no-longer a connected and visible display

Ack.

>> The best way to check this is look under /sys/class/drm and find out
>> which /sys/class/drm/card#-<conn-type>-# entry belongs to the extra
>> panel. Step 1 check for all card#-<conn-type>-# entries
>> where status returns connected, e.g. :
>>
>> [hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/status 
>> connected
>>
> 
> On disable the status does change to:
> cat /sys/class/drm/card1-DP-5/enabled
> disabled

Ok, that is good.


>> Step 2: for the connected ones cat the modes, e.g.:
>>
>> [hans@shalem ~]$ cat /sys/class/drm/card1-DP-1/modes
>> 1920x1080
>> 1600x1200
>> ...
>>
>> And find the one which matches with the resolution of the extra panel
>> (the one which does not match with the resolution of the main panel).
>>
>> Then turn the extra panel of through WMI and cat the status attribute
>> again. If that still reads connected then that means the desktop
>> environment keeps seeing an extra display output which is not ideal.
>> This will e.g. cause any windows which were on the extra panel to
>> stay there, even though they are no longer visible.
>>
>>
>> Question 2:
>>
>> If you turn the second screen off through drm/kms, using the desktop
>> environments monitor config panel does this also turn off the
>> backlight ?
> 
> The screen is dark but there is still some backlight coming out of it.
> I think this means I need to add a small pre-off to the patch to ensure
> backlight is fully off when display is turned off.

I'm afraid that this is not going to be easy to fix at the kernel level,
we first need to tie backlight control to drm-connectors as I proposed
(and plan to implement when I can make time):

https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

Once that is in place we can simply make the drm-code call out to
the backlight driver and have it turn the backlight off when disabling
the output through the drm/kms interface.

>> After disabling the screen in the desktop environments monitor config
>> check that the enabled attribute, e.g. cat /sys/class/drm/card1-DP-
>> 1/enabled shows disabled and after verifying this look at the extra
>> screen in a dark room, do you see any backlight bleed indicating the
>> backlight is still  on?
>>
> 
> Shows as disabled
> 
>>
>> We really want the backlight on/off state and the drm-connector
>> enabled state to match. My proposal from above will allow this once
>> implemented. Until we can hook this all up nicely I think it might be
>> better to just go with the custom sysfs attributes from your v1 patch
>> rather then adding a /sys/class/backlight device for this.
>>
> 
> I would like to go with the backlight patch as it seems more likely I
> can adjust this without breaking userspace if required in future. The
> WMI controls behave as expected to.

Ok, lets go with the v2 which adds /sys/class/backlight support then.

I must warn you though if this does turn out to cause issues I'll have
no choice but to revert it.

I must admit I've lost track a bit of the state of v2 during this
discussion.  Can I pick up v2 as is, or were there (other) remarks
which need addressing and should I expect a v3 ?

####### Switch to (off-topic) GPU mux discussion ########

>>> So I think now is probably a good time to raise a particular issue
>>> I've encountered with the last two years: the display MUX.
>>>
>>> As I understand it now, there are two types of new MUX - the manual
>>> switch, and the newer "Advanced Optimus" automatic switch. The
>>> issues I have are with the manual switch since I've not encountered
>>> the advanced optimus yet.
>>>
>>> When the switch is. uh. switched. the dgpu drives the internal
>>> display, and I expect that since the display is now detected
>>> through the dgpu, this is why the dgpu is kept awake to drive it.
>>> But, the igpu is also still active, and because of this the initial
>>> boot from grub to display-manager is a black screen including tty.
>>> This means anyone with an encrypted drive will never see the prompt
>>> and they believe they have a failed boot. I don't know what to do
>>> about this?
>>
>> Is this with EFI booting or with classic BIOS boot? With EFI booting
>> the EFIFB should be put on the right GPU by the firmware. So I
>> suspect this is with classic BIOS boot?
> 
> No this is with EFI boot always. I'm not even sure these machines still
> have the ability to boot oldschool bios style.

Hmm, weird with EFI grub is just using the EFI text output protocol
so what is happening here at the grub level is actually a bug in
the firmware of the laptop(s)...

>> I think the best thing to do here is to just use EFI on machines like
>> this. That or put grub in text mode so that it makes BIOS calls to
>> display text. Using GRUB_TERMINAL_OUTPUT=gfxterm combined with
>> classic BIOS booting will make grub try to directly drive the gfx
>> card itself and I'm not surprised that it gets that wrong in this
>> case.
>> Note I think that just using EFI is prefered over switching grub to
>> GRUB_TERMINAL_OUTPUT=console. I would expect
>> GRUB_TERMINAL_OUTPUT=console to also work but I'm not sure. I don't
>> think that the classic BIOS boot stuff is still tested by laptop
>> vendors and esp. not tested with non standard BIOS settings ...
> 
> The grub gfx mode is GRUB_TERMINAL_OUTPUT="console", fedora default in
> all cases here. Grub itself shows fine when the MUX mode is in dgpu
> mode (aka, internal display connected to dgpu).

Ah ok, so I misunderstood and the problem only happens *after* grub?

Have I understood that correctly?

And this is on Fedora with the nvidia binary driver ?

The problem then likely is that the nvidia binary driver is not in
the initrd (which is by design since it may need to be rebuild on
a driver update while the kernel is kept at the same version,
so the initrd won't be rebuild).

So during the initrd there then is no kms driver to drive the
internal display.

Normally plymouth falls back to using the efifb (through simpledrm)
but that is after a pretty large timeout and I think this may not
be happening here because plymouth maybe thinks there is a kms
capable display connected to the iGPU for some reason
(maybe the extra screenpad panel ?)

As a workaround you can tell plymouth to use the simpledrm kms
device as soon as it becomes available instead of waiting for
a native kms driver. To do this add "plymouth.use-simpledrm" to
the kernel commandline. I think that this will work around
the problem.

> P.S> I will go ahead with the screen patch as it looks like this
> behaves as expected minus the need to turn off BL before turn off
> display. I will submit revision later in the week.

Ok, so that answers my question from above and I should wait for a v3 :)

Regards,

Hans

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

**snip**
> >> Question 2:
> >> 
> >> If you turn the second screen off through drm/kms, using the desktop
> >> environments monitor config panel does this also turn off the
> >> backlight ?
> > 
> > The screen is dark but there is still some backlight coming out of it.
> > I think this means I need to add a small pre-off to the patch to ensure
> > backlight is fully off when display is turned off.
> 
> I'm afraid that this is not going to be easy to fix at the kernel level,
> we first need to tie backlight control to drm-connectors as I proposed
> (and plan to implement when I can make time):
> 
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redha
> t.com/
> 
> Once that is in place we can simply make the drm-code call out to
> the backlight driver and have it turn the backlight off when disabling
> the output through the drm/kms interface.

Okay cool. But until then I can set the screenpad to turn brightness off when 
it does the call to:
err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
here I can also do before that call:
err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,ctrl_param, NULL);

Then when the patch you mention is done this can be removed.

**snip** 
> > I would like to go with the backlight patch as it seems more likely I
> > can adjust this without breaking userspace if required in future. The
> > WMI controls behave as expected to.
> 
> Ok, lets go with the v2 which adds /sys/class/backlight support then.
> 
> I must warn you though if this does turn out to cause issues I'll have
> no choice but to revert it.
> 
> I must admit I've lost track a bit of the state of v2 during this
> discussion.  Can I pick up v2 as is, or were there (other) remarks
> which need addressing and should I expect a v3 ?

There will be a V3. I don't anticipate any issues at all with this, and some 
folks have been using this patch with Gnome and KDE since V2 was submitted.

> 
> ####### Switch to (off-topic) GPU mux discussion ########
**snip**
> >> I think the best thing to do here is to just use EFI on machines like
> >> this. That or put grub in text mode so that it makes BIOS calls to
> >> display text. Using GRUB_TERMINAL_OUTPUT=gfxterm combined with
> >> classic BIOS booting will make grub try to directly drive the gfx
> >> card itself and I'm not surprised that it gets that wrong in this
> >> case.
> >> Note I think that just using EFI is prefered over switching grub to
> >> GRUB_TERMINAL_OUTPUT=console. I would expect
> >> GRUB_TERMINAL_OUTPUT=console to also work but I'm not sure. I don't
> >> think that the classic BIOS boot stuff is still tested by laptop
> >> vendors and esp. not tested with non standard BIOS settings ...
> > 
> > The grub gfx mode is GRUB_TERMINAL_OUTPUT="console", fedora default in
> > all cases here. Grub itself shows fine when the MUX mode is in dgpu
> > mode (aka, internal display connected to dgpu).
> 
> Ah ok, so I misunderstood and the problem only happens *after* grub?
> 
> Have I understood that correctly?
> 
> And this is on Fedora with the nvidia binary driver ?
> 
> The problem then likely is that the nvidia binary driver is not in
> the initrd (which is by design since it may need to be rebuild on
> a driver update while the kernel is kept at the same version,
> so the initrd won't be rebuild).

That was indeed the issue. It also creates new problems for when a user wants 
to use iGPU only via the plain (and frankly not adequate or good) method of 
simply removing the dGPU from the device tree.

Currently I maintain the supergfxd tool which is a lot more advanced than the 
other gpu switchers around, and I expose both the above method, and also PCI 
hotplug, and the ASUS WMI method (which I now think is used by other vendors 
also). Hotplug and Asus method can force the device off, and it can't be 
brought back by a PCI rescan - but to do so safely the Nvidia drivers must be 
unused and unloaded. I guess I'll need to tweak the boot process of supergfxd 
and block things until this is done.

Maybe we can move this to a new topic, because there looks to be a few things 
to discuss in relation to hybrid laptops, and specifically Nvidia with MUX, 
and Advanced Optimus.

Cheers,
Luke.



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Hans de Goede]

Hi,

On 6/7/23 00:45, Luke Jones wrote:

<snip>

>> I must admit I've lost track a bit of the state of v2 during this
>> discussion.  Can I pick up v2 as is, or were there (other) remarks
>> which need addressing and should I expect a v3 ?
> 
> There will be a V3. I don't anticipate any issues at all with this, and some 
> folks have been using this patch with Gnome and KDE since V2 was submitted.

Ok.


>> ####### Switch to (off-topic) GPU mux discussion ########
> **snip**
>>>> I think the best thing to do here is to just use EFI on machines like
>>>> this. That or put grub in text mode so that it makes BIOS calls to
>>>> display text. Using GRUB_TERMINAL_OUTPUT=gfxterm combined with
>>>> classic BIOS booting will make grub try to directly drive the gfx
>>>> card itself and I'm not surprised that it gets that wrong in this
>>>> case.
>>>> Note I think that just using EFI is prefered over switching grub to
>>>> GRUB_TERMINAL_OUTPUT=console. I would expect
>>>> GRUB_TERMINAL_OUTPUT=console to also work but I'm not sure. I don't
>>>> think that the classic BIOS boot stuff is still tested by laptop
>>>> vendors and esp. not tested with non standard BIOS settings ...
>>>
>>> The grub gfx mode is GRUB_TERMINAL_OUTPUT="console", fedora default in
>>> all cases here. Grub itself shows fine when the MUX mode is in dgpu
>>> mode (aka, internal display connected to dgpu).
>>
>> Ah ok, so I misunderstood and the problem only happens *after* grub?
>>
>> Have I understood that correctly?
>>
>> And this is on Fedora with the nvidia binary driver ?
>>
>> The problem then likely is that the nvidia binary driver is not in
>> the initrd (which is by design since it may need to be rebuild on
>> a driver update while the kernel is kept at the same version,
>> so the initrd won't be rebuild).
> 
> That was indeed the issue. It also creates new problems for when a user wants 
> to use iGPU only via the plain (and frankly not adequate or good) method of 
> simply removing the dGPU from the device tree.
> 
> Currently I maintain the supergfxd tool which is a lot more advanced than the 
> other gpu switchers around, and I expose both the above method, and also PCI 
> hotplug, and the ASUS WMI method (which I now think is used by other vendors 
> also). Hotplug and Asus method can force the device off, and it can't be 
> brought back by a PCI rescan - but to do so safely the Nvidia drivers must be 
> unused and unloaded. I guess I'll need to tweak the boot process of supergfxd 
> and block things until this is done.
> 
> Maybe we can move this to a new topic, because there looks to be a few things 
> to discuss in relation to hybrid laptops, and specifically Nvidia with MUX, 
> and Advanced Optimus.

Yes if you want to discuss this further please start a new mailinglist
thread for this and also please put the dri-devel list on the Cc:

dri-devel@lists.freedesktop.org

Regards,

Hans