[PATCH v3 0/1] HID backlight driver

[PATCH v3 0/1] HID backlight driver

[Julius Zint]

This patch adds support for HID backlight devices, found in the Apple
Studio Display.

Changes in v1 [1]:
  - Add USB backlight driver for Studio Display

Changes in v2 [2]:
  - Rewrite from a USB driver to a HID driver

Changes in v3:
  - Added missing hid_bl prefix for some functions
  - Early exit in probe when HID parsing fails
  - Add return code to error logs
  - Adding HID Maintainers for review

[1] https://lore.kernel.org/dri-devel/20230701120806.11812-1-julius@zint.sh/
[2] https://lore.kernel.org/dri-devel/20230806091403.10002-1-julius@zint.sh/

Julius Zint (1):
  backlight: hid_bl: Add VESA VCP HID backlight driver

 drivers/video/backlight/Kconfig  |   8 +
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/hid_bl.c | 269 +++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/video/backlight/hid_bl.c


base-commit: dfd122fe8591d513b5e51313601217b67ae98d13
-- 
2.41.0

[PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

The HID spec defines the following Usage IDs (p. 345 ff):

- Monitor Page (0x80) -> Monitor Control (0x01)
- VESA Virtual Controls Page (0x82) -> Brightness (0x10)

Apple made use of them in their Apple Studio Display and most likely on
other external displays (LG UltraFine 5k, Pro Display XDR).

The driver will work for any HID device with a report, where the
application matches the Monitor Control Usage ID and:

1. An Input field in this report with the Brightness Usage ID (to get
   the current brightness)
2. A Feature field in this report with the Brightness Usage ID (to
   set the current brightness)

This driver has been developed and tested with the Apple Studio Display.
Here is a small excerpt from the decoded HID descriptor showing the
feature field for setting the brightness:

  Usage Page (Monitor VESA VCP),  ; Monitor VESA VPC (82h, monitor page)
  Usage (10h, Brightness),
  Logical Minimum (400),
  Logical Maximum (60000),
  Unit (Centimeter^-2 * Candela),
  Unit Exponent (14),
  Report Size (32),
  Report Count (1),
  Feature (Variable, Null State),

The full HID descriptor dump is available as a comment in the source
code.

Signed-off-by: Julius Zint <julius@zint.sh>
---
 drivers/video/backlight/Kconfig  |   8 +
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/hid_bl.c | 269 +++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/video/backlight/hid_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 51387b1ef012..b964a820956d 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -472,6 +472,14 @@ config BACKLIGHT_LED
 	  If you have a LCD backlight adjustable by LED class driver, say Y
 	  to enable this driver.
 
+config BACKLIGHT_HID
+	tristate "VESA VCP HID Backlight Driver"
+	depends on HID
+	help
+	  If you have an external display with VESA compliant HID brightness
+	  controls then say Y to enable this backlight driver. Currently the
+	  only supported device is the Apple Studio Display.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endmenu
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index f72e1c3c59e9..835f9b8772c7 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -58,3 +58,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
 obj-$(CONFIG_BACKLIGHT_ARCXCNN) 	+= arcxcnn_bl.o
 obj-$(CONFIG_BACKLIGHT_RAVE_SP)		+= rave-sp-backlight.o
 obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
+obj-$(CONFIG_BACKLIGHT_HID)		+= hid_bl.o
diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
new file mode 100644
index 000000000000..b40f8f412ee2
--- /dev/null
+++ b/drivers/video/backlight/hid_bl.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/backlight.h>
+
+#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
+#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
+
+#define HID_USAGE_MONITOR_CTRL			0x800001
+#define HID_USAGE_VESA_VCP_BRIGHTNESS		0x820010
+
+/*
+ * Apple Studio Display HID report descriptor
+ *
+ * Usage Page (Monitor),               ; USB monitor (80h, monitor page)
+ * Usage (01h),
+ * Collection (Application),
+ *     Report ID (1),
+ *
+ *     Usage Page (Monitor VESA VCP),  ; Monitor VESA virtual control panel (82h, monitor page)
+ *     Usage (10h, Brightness),
+ *     Logical Minimum (400),
+ *     Logical Maximum (60000),
+ *     Unit (Centimeter^-2 * Candela),
+ *     Unit Exponent (14),
+ *     Report Size (32),
+ *     Report Count (1),
+ *     Feature (Variable, Null State),
+ *
+ *     Usage Page (PID),               ; Physical interface device (0Fh)
+ *     Usage (50h),
+ *     Logical Minimum (0),
+ *     Logical Maximum (20000),
+ *     Unit (1001h),
+ *     Unit Exponent (13),
+ *     Report Size (16),
+ *     Feature (Variable, Null State),
+ *
+ *     Usage Page (Monitor VESA VCP),  ; Monitor VESA virtual control panel (82h, monitor page)
+ *     Usage (10h, Brightness),
+ *     Logical Minimum (400),
+ *     Logical Maximum (60000),
+ *     Unit (Centimeter^-2 * Candela),
+ *     Unit Exponent (14),
+ *     Report Size (32),
+ *     Report Count (1),
+ *     Input (Variable),
+ * End Collection
+ */
+
+struct hid_bl_data {
+	struct hid_device *hdev;
+	unsigned int min_brightness;
+	unsigned int max_brightness;
+	struct hid_field *input_field;
+	struct hid_field *feature_field;
+};
+
+static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
+					     unsigned int report_type,
+					     unsigned int application, unsigned int usage)
+{
+	struct hid_report_enum *re = &hdev->report_enum[report_type];
+	struct hid_report *report;
+	int i, j;
+
+	list_for_each_entry(report, &re->report_list, list) {
+		if (report->application != application)
+			continue;
+
+		for (i = 0; i < report->maxfield; i++) {
+			struct hid_field *field = report->field[i];
+
+			for (j = 0; j < field->maxusage; j++)
+				if (field->usage[j].hid == usage)
+					return field;
+		}
+	}
+	return NULL;
+}
+
+static void hid_bl_remove(struct hid_device *hdev)
+{
+	struct backlight_device *bl;
+	struct hid_bl_data *data;
+
+	hid_dbg(hdev, "remove\n");
+	bl = hid_get_drvdata(hdev);
+	data = bl_get_data(bl);
+
+	devm_backlight_device_unregister(&hdev->dev, bl);
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+	hid_set_drvdata(hdev, NULL);
+	devm_kfree(&hdev->dev, data);
+}
+
+static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
+{
+	struct hid_field *field;
+	int result;
+
+	field = data->input_field;
+	hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
+	hid_hw_wait(data->hdev);
+	result = *field->new_value;
+	hid_dbg(data->hdev, "get brightness: %d\n", result);
+
+	return result;
+}
+
+static int hid_bl_get_brightness(struct backlight_device *bl)
+{
+	struct hid_bl_data *data;
+	int brightness;
+
+	data = bl_get_data(bl);
+	brightness = hid_bl_get_brightness_raw(data);
+	return brightness - data->min_brightness;
+}
+
+static void hid_bl_set_brightness_raw(struct hid_bl_data *data, int brightness)
+{
+	struct hid_field *field;
+
+	field = data->feature_field;
+	*field->value = brightness;
+	hid_hw_request(data->hdev, field->report, HID_REQ_SET_REPORT);
+	hid_hw_wait(data->hdev);
+	hid_dbg(data->hdev, "set brightness: %d\n", brightness);
+}
+
+static int hid_bl_update_status(struct backlight_device *bl)
+{
+	struct hid_bl_data *data;
+	int brightness;
+
+	data = bl_get_data(bl);
+	brightness = backlight_get_brightness(bl);
+	brightness += data->min_brightness;
+	hid_bl_set_brightness_raw(data, brightness);
+	return 0;
+}
+
+static const struct backlight_ops hid_bl_ops = {
+	.update_status  = hid_bl_update_status,
+	.get_brightness = hid_bl_get_brightness,
+};
+
+static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	struct hid_field *input_field;
+	struct hid_field *feature_field;
+	struct hid_bl_data *data;
+	struct backlight_properties props;
+	struct backlight_device *bl;
+
+	hid_dbg(hdev, "probe\n");
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "parse failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+	if (ret) {
+		hid_err(hdev, "hw start failed: %d\n", ret);
+		return ret;
+	}
+
+	input_field = hid_get_usage_field(hdev, HID_INPUT_REPORT,
+					  HID_USAGE_MONITOR_CTRL,
+					  HID_USAGE_VESA_VCP_BRIGHTNESS);
+	if (input_field == NULL) {
+		ret = -ENODEV;
+		goto exit_stop;
+	}
+
+	feature_field = hid_get_usage_field(hdev, HID_FEATURE_REPORT,
+					    HID_USAGE_MONITOR_CTRL,
+					    HID_USAGE_VESA_VCP_BRIGHTNESS);
+	if (feature_field == NULL) {
+		ret = -ENODEV;
+		goto exit_stop;
+	}
+
+	if (input_field->logical_minimum != feature_field->logical_minimum) {
+		hid_warn(hdev, "minimums do not match: %d / %d\n",
+			 input_field->logical_minimum,
+			 feature_field->logical_minimum);
+		ret = -ENODEV;
+		goto exit_stop;
+	}
+
+	if (input_field->logical_maximum != feature_field->logical_maximum) {
+		hid_warn(hdev, "maximums do not match: %d / %d\n",
+			 input_field->logical_maximum,
+			 feature_field->logical_maximum);
+		ret = -ENODEV;
+		goto exit_stop;
+	}
+
+	hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "hw open failed: %d\n", ret);
+		goto exit_stop;
+	}
+
+	data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto exit_stop;
+	}
+	data->hdev = hdev;
+	data->min_brightness = input_field->logical_minimum;
+	data->max_brightness = input_field->logical_maximum;
+	data->input_field = input_field;
+	data->feature_field = feature_field;
+
+	memset(&props, 0, sizeof(props));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = data->max_brightness - data->min_brightness;
+
+	bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
+					    &hdev->dev, data,
+					    &hid_bl_ops,
+					    &props);
+	if (IS_ERR(bl)) {
+		ret = PTR_ERR(bl);
+		hid_err(hdev, "failed to register backlight: %d\n", ret);
+		goto exit_free;
+	}
+
+	hid_set_drvdata(hdev, bl);
+
+	return 0;
+
+exit_free:
+	hid_hw_close(hdev);
+	devm_kfree(&hdev->dev, data);
+
+exit_stop:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static const struct hid_device_id hid_bl_devices[] = {
+	{ HID_USB_DEVICE(APPLE_STUDIO_DISPLAY_VENDOR_ID,
+			 APPLE_STUDIO_DISPLAY_PRODUCT_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, hid_bl_devices);
+
+static struct hid_driver hid_bl_driver = {
+	.name = "hid_backlight",
+	.id_table = hid_bl_devices,
+	.probe = hid_bl_probe,
+	.remove = hid_bl_remove,
+};
+module_hid_driver(hid_bl_driver);
+
+MODULE_AUTHOR("Julius Zint <julius@zint.sh>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Backlight driver for HID devices");
-- 
2.41.0

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Christophe JAILLET]

Le 20/08/2023 à 11:41, Julius Zint a écrit :
> The HID spec defines the following Usage IDs (p. 345 ff):
> 
> - Monitor Page (0x80) -> Monitor Control (0x01)
> - VESA Virtual Controls Page (0x82) -> Brightness (0x10)
> 
> Apple made use of them in their Apple Studio Display and most likely on
> other external displays (LG UltraFine 5k, Pro Display XDR).
> 
> The driver will work for any HID device with a report, where the
> application matches the Monitor Control Usage ID and:
> 
> 1. An Input field in this report with the Brightness Usage ID (to get
>     the current brightness)
> 2. A Feature field in this report with the Brightness Usage ID (to
>     set the current brightness)
> 
> This driver has been developed and tested with the Apple Studio Display.
> Here is a small excerpt from the decoded HID descriptor showing the
> feature field for setting the brightness:
> 
>    Usage Page (Monitor VESA VCP),  ; Monitor VESA VPC (82h, monitor page)
>    Usage (10h, Brightness),
>    Logical Minimum (400),
>    Logical Maximum (60000),
>    Unit (Centimeter^-2 * Candela),
>    Unit Exponent (14),
>    Report Size (32),
>    Report Count (1),
>    Feature (Variable, Null State),
> 
> The full HID descriptor dump is available as a comment in the source
> code.
> 
> Signed-off-by: Julius Zint <julius@zint.sh>
> ---

[...]

> +static void hid_bl_remove(struct hid_device *hdev)
> +{
> +	struct backlight_device *bl;
> +	struct hid_bl_data *data;
> +
> +	hid_dbg(hdev, "remove\n");
> +	bl = hid_get_drvdata(hdev);
> +	data = bl_get_data(bl);
> +
> +	devm_backlight_device_unregister(&hdev->dev, bl);

Hi,

If this need to be called here, before the hid_() calls below, there 
seems to be no real point in using devm_backlight_device_register() / 
devm_backlight_device_unregister().

The non-devm_ version should be enough.

> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +	hid_set_drvdata(hdev, NULL);
> +	devm_kfree(&hdev->dev, data);

'data' is devm_kzalloc()'ed.
It is likely that this explicit devm_kfree() could be saved. It will be 
freed by the framework.

> +}

[...]

> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +	struct hid_field *input_field;
> +	struct hid_field *feature_field;
> +	struct hid_bl_data *data;
> +	struct backlight_properties props;
> +	struct backlight_device *bl;
> +
> +	hid_dbg(hdev, "probe\n");
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "parse failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	input_field = hid_get_usage_field(hdev, HID_INPUT_REPORT,
> +					  HID_USAGE_MONITOR_CTRL,
> +					  HID_USAGE_VESA_VCP_BRIGHTNESS);
> +	if (input_field == NULL) {
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	feature_field = hid_get_usage_field(hdev, HID_FEATURE_REPORT,
> +					    HID_USAGE_MONITOR_CTRL,
> +					    HID_USAGE_VESA_VCP_BRIGHTNESS);
> +	if (feature_field == NULL) {
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	if (input_field->logical_minimum != feature_field->logical_minimum) {
> +		hid_warn(hdev, "minimums do not match: %d / %d\n",
> +			 input_field->logical_minimum,
> +			 feature_field->logical_minimum);
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	if (input_field->logical_maximum != feature_field->logical_maximum) {
> +		hid_warn(hdev, "maximums do not match: %d / %d\n",
> +			 input_field->logical_maximum,
> +			 feature_field->logical_maximum);
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw open failed: %d\n", ret);
> +		goto exit_stop;
> +	}
> +
> +	data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (data == NULL) {
> +		ret = -ENOMEM;
> +		goto exit_stop;

exit_free?
in order to undo the hid_hw_open() just above.

> +	}
> +	data->hdev = hdev;
> +	data->min_brightness = input_field->logical_minimum;
> +	data->max_brightness = input_field->logical_maximum;
> +	data->input_field = input_field;
> +	data->feature_field = feature_field;
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = data->max_brightness - data->min_brightness;
> +
> +	bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
> +					    &hdev->dev, data,
> +					    &hid_bl_ops,
> +					    &props);
> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		hid_err(hdev, "failed to register backlight: %d\n", ret);
> +		goto exit_free;
> +	}
> +
> +	hid_set_drvdata(hdev, bl);
> +
> +	return 0;
> +
> +exit_free:
> +	hid_hw_close(hdev);
> +	devm_kfree(&hdev->dev, data);

'data' is devm_kzalloc()'ed.
It is likely that this explicit devm_kfree() could be saved. It will be 
freed by the framework.

> +
> +exit_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}

[...]

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Daniel Thompson]

On Sun, Aug 20, 2023 at 11:41:18AM +0200, Julius Zint wrote:
> The HID spec defines the following Usage IDs (p. 345 ff):
>
> - Monitor Page (0x80) -> Monitor Control (0x01)
> - VESA Virtual Controls Page (0x82) -> Brightness (0x10)
>
> Apple made use of them in their Apple Studio Display and most likely on
> other external displays (LG UltraFine 5k, Pro Display XDR).
>
> The driver will work for any HID device with a report, where the
> application matches the Monitor Control Usage ID and:
>
> 1. An Input field in this report with the Brightness Usage ID (to get
>    the current brightness)
> 2. A Feature field in this report with the Brightness Usage ID (to
>    set the current brightness)
>
> This driver has been developed and tested with the Apple Studio Display.
> Here is a small excerpt from the decoded HID descriptor showing the
> feature field for setting the brightness:
>
>   Usage Page (Monitor VESA VCP),  ; Monitor VESA VPC (82h, monitor page)
>   Usage (10h, Brightness),
>   Logical Minimum (400),
>   Logical Maximum (60000),
>   Unit (Centimeter^-2 * Candela),
>   Unit Exponent (14),
>   Report Size (32),
>   Report Count (1),
>   Feature (Variable, Null State),
>
> The full HID descriptor dump is available as a comment in the source
> code.
>
> Signed-off-by: Julius Zint <julius@zint.sh>

I saw Christophe's review (thanks) and won't repeat anything from
there...

> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>  	  If you have a LCD backlight adjustable by LED class driver, say Y
>  	  to enable this driver.
>
> +config BACKLIGHT_HID
> +	tristate "VESA VCP HID Backlight Driver"
> +	depends on HID
> +	help
> +	  If you have an external display with VESA compliant HID brightness
> +	  controls then say Y to enable this backlight driver. Currently the
> +	  only supported device is the Apple Studio Display.

This contradicts the description which says you write the driver to the
standard but only tested on Apple Studio Display. There is no need to
spell what has been tested in the Kconfig text. Remove the final
sentence!

> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
> new file mode 100644
> index 000000000000..b40f8f412ee2
> --- /dev/null
> +++ b/drivers/video/backlight/hid_bl.c
> <snip>
> +static void hid_bl_remove(struct hid_device *hdev)
> +{
> +	struct backlight_device *bl;
> +	struct hid_bl_data *data;
> +
> +	hid_dbg(hdev, "remove\n");

This message probably should be removed (if you want to know if a function was
executed use ftrace).


> +	bl = hid_get_drvdata(hdev);
> +	data = bl_get_data(bl);
> +
> +	devm_backlight_device_unregister(&hdev->dev, bl);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +	hid_set_drvdata(hdev, NULL);
> +	devm_kfree(&hdev->dev, data);
> +}
> +
> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
> +{
> +	struct hid_field *field;
> +	int result;
> +
> +	field = data->input_field;
> +	hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(data->hdev);
> +	result = *field->new_value;
> +	hid_dbg(data->hdev, "get brightness: %d\n", result);

To be honest I'm a little dubious about *all* the hid_dbg() calls. They
add very little value (e.g. they are useful to get the driver working
but not that important to keeping it working). As such I don't think
they are worth the clutter in a CONFIG_DYNAMIC_DEBUG kernel.

Note this is strictly for the hid_dbg() stuff... the hid_err() stuff in
the probe error paths are much more useful!


Daniel.

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

On 21.08.23 18:36, Daniel Thompson wrote:
>> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>>   	  If you have a LCD backlight adjustable by LED class driver, say Y
>>   	  to enable this driver.
>>
>> +config BACKLIGHT_HID
>> +	tristate "VESA VCP HID Backlight Driver"
>> +	depends on HID
>> +	help
>> +	  If you have an external display with VESA compliant HID brightness
>> +	  controls then say Y to enable this backlight driver. Currently the
>> +	  only supported device is the Apple Studio Display.
> This contradicts the description which says you write the driver to the
> standard but only tested on Apple Studio Display. There is no need to
> spell what has been tested in the Kconfig text. Remove the final
> sentence!
Will remove it in v4.
>> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
>> new file mode 100644
>> index 000000000000..b40f8f412ee2
>> --- /dev/null
>> +++ b/drivers/video/backlight/hid_bl.c
>> <snip>
>> +static void hid_bl_remove(struct hid_device *hdev)
>> +{
>> +	struct backlight_device *bl;
>> +	struct hid_bl_data *data;
>> +
>> +	hid_dbg(hdev, "remove\n");
> This message probably should be removed (if you want to know if a function was
> executed use ftrace).
>
>
>> +	bl = hid_get_drvdata(hdev);
>> +	data = bl_get_data(bl);
>> +
>> +	devm_backlight_device_unregister(&hdev->dev, bl);
>> +	hid_hw_close(hdev);
>> +	hid_hw_stop(hdev);
>> +	hid_set_drvdata(hdev, NULL);
>> +	devm_kfree(&hdev->dev, data);
>> +}
>> +
>> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
>> +{
>> +	struct hid_field *field;
>> +	int result;
>> +
>> +	field = data->input_field;
>> +	hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
>> +	hid_hw_wait(data->hdev);
>> +	result = *field->new_value;
>> +	hid_dbg(data->hdev, "get brightness: %d\n", result);
> To be honest I'm a little dubious about *all* the hid_dbg() calls. They
> add very little value (e.g. they are useful to get the driver working
> but not that important to keeping it working). As such I don't think
> they are worth the clutter in a CONFIG_DYNAMIC_DEBUG kernel.
>
> Note this is strictly for the hid_dbg() stuff... the hid_err() stuff in
> the probe error paths are much more useful!
You are right, I will remove all hid_dbg calls in v4.

Thank you very much for the review.

Julius

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

On 20.08.23 12:06, Christophe JAILLET wrote:
> [...]
>
>> +static void hid_bl_remove(struct hid_device *hdev)
>> +{
>> +    struct backlight_device *bl;
>> +    struct hid_bl_data *data;
>> +
>> +    hid_dbg(hdev, "remove\n");
>> +    bl = hid_get_drvdata(hdev);
>> +    data = bl_get_data(bl);
>> +
>> +    devm_backlight_device_unregister(&hdev->dev, bl);
>
> Hi,
>
> If this need to be called here, before the hid_() calls below, there 
> seems to be no real point in using devm_backlight_device_register() / 
> devm_backlight_device_unregister().
>
> The non-devm_ version should be enough.
The non-devm_ version is marked deprecated. As for the order, I am not 
really sure. I am
concerned about someone updating the brightness while its being removed. 
So the HID device
would already have been stopped and then I would issue a request and 
wait for completion.

If hid_hw_request and hid_hw_wait can handle this situation we are fine.
>
>> +    hid_hw_close(hdev);
>> +    hid_hw_stop(hdev);
>> +    hid_set_drvdata(hdev, NULL);
>> +    devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will 
> be freed by the framework.
>
>> +}
>
> [...]
>
>> +    if (input_field->logical_maximum != 
>> feature_field->logical_maximum) {
>> +        hid_warn(hdev, "maximums do not match: %d / %d\n",
>> +             input_field->logical_maximum,
>> +             feature_field->logical_maximum);
>> +        ret = -ENODEV;
>> +        goto exit_stop;
>> +    }
>> +
>> +    hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
>> +
>> +    ret = hid_hw_open(hdev);
>> +    if (ret) {
>> +        hid_err(hdev, "hw open failed: %d\n", ret);
>> +        goto exit_stop;
>> +    }
>> +
>> +    data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
>> +    if (data == NULL) {
>> +        ret = -ENOMEM;
>> +        goto exit_stop;
>
> exit_free?
> in order to undo the hid_hw_open() just above.
Yes, my mistake. This does not call hid_hw_close(). I will fix it in v4.
>
>> +    }
>> +    data->hdev = hdev;
>> +    data->min_brightness = input_field->logical_minimum;
>> +    data->max_brightness = input_field->logical_maximum;
>> +    data->input_field = input_field;
>> +    data->feature_field = feature_field;
>> +
>> +    memset(&props, 0, sizeof(props));
>> +    props.type = BACKLIGHT_RAW;
>> +    props.max_brightness = data->max_brightness - data->min_brightness;
>> +
>> +    bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
>> +                        &hdev->dev, data,
>> +                        &hid_bl_ops,
>> +                        &props);
>> +    if (IS_ERR(bl)) {
>> +        ret = PTR_ERR(bl);
>> +        hid_err(hdev, "failed to register backlight: %d\n", ret);
>> +        goto exit_free;
>> +    }
>> +
>> +    hid_set_drvdata(hdev, bl);
>> +
>> +    return 0;
>> +
>> +exit_free:
>> +    hid_hw_close(hdev);
>> +    devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will 
> be freed by the framework.
>
>> +
>> +exit_stop:
>> +    hid_hw_stop(hdev);
>> +    return ret;
>> +}
>
> [...]
I will remove all of the explicit calls to devm_kfree (and others) in v4 
(and test it thoroughly).

Thank you for the helpful review.

Julius

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Thomas Weißschuh]

On 2023-08-20 11:41:18+0200, Julius Zint wrote:
> [..]

> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..b964a820956d 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>  	  If you have a LCD backlight adjustable by LED class driver, say Y
>  	  to enable this driver.
>  
> +config BACKLIGHT_HID
> +	tristate "VESA VCP HID Backlight Driver"
> +	depends on HID
> +	help
> +	  If you have an external display with VESA compliant HID brightness
> +	  controls then say Y to enable this backlight driver. Currently the
> +	  only supported device is the Apple Studio Display.

Is the last sentence needed?
It will go out of date soon, requiring updates to the Kconfig.

> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endmenu

> [..]

> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
> new file mode 100644
> index 000000000000..b40f8f412ee2
> --- /dev/null
> +++ b/drivers/video/backlight/hid_bl.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/backlight.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114

Use hid-ids.h.  The vendor ID already has an entry.

> +
> +#define HID_USAGE_MONITOR_CTRL			0x800001
> +#define HID_USAGE_VESA_VCP_BRIGHTNESS		0x820010

> [..]

> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{

> [..]

> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;

Wouldn't this be more a BACKLIGHT_FIRMWARE?

> +	props.max_brightness = data->max_brightness - data->min_brightness;
> +
> +	bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",

It's non-obvious that the "vesa_vcp" backlight comes from the
"hid_backlight" driver. Maybe align the names.

What happens when multiple compatible devices are used?
That seems to be possible with external monitors.

Can existing userspace figure out which display the backlight device
belongs to?
(I don't know either)

> +					    &hdev->dev, data,
> +					    &hid_bl_ops,
> +					    &props);

> [..]

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Thomas Weißschuh]

+Cc Hans who ins involved with the backlight subsystem

Hi Julius,

today I stumbled upon a mail from Hans [0], which explains that the
backlight subsystem is not actually a good fit (yet?) for external
displays.

It seems a new API is in the works that would better fit, but I'm not
sure about the state of this API. Maybe Hans can clarify.

This also ties back to my review question how userspace can figure out
to which display a backlight devices applies. So far it can not.

[0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/

Below the original PATCH for Hans' reference.

On 2023-08-20 11:41:18+0200, Julius Zint wrote:
> The HID spec defines the following Usage IDs (p. 345 ff):
> 
> - Monitor Page (0x80) -> Monitor Control (0x01)
> - VESA Virtual Controls Page (0x82) -> Brightness (0x10)
> 
> Apple made use of them in their Apple Studio Display and most likely on
> other external displays (LG UltraFine 5k, Pro Display XDR).
> 
> The driver will work for any HID device with a report, where the
> application matches the Monitor Control Usage ID and:
> 
> 1. An Input field in this report with the Brightness Usage ID (to get
>    the current brightness)
> 2. A Feature field in this report with the Brightness Usage ID (to
>    set the current brightness)
> 
> This driver has been developed and tested with the Apple Studio Display.
> Here is a small excerpt from the decoded HID descriptor showing the
> feature field for setting the brightness:
> 
>   Usage Page (Monitor VESA VCP),  ; Monitor VESA VPC (82h, monitor page)
>   Usage (10h, Brightness),
>   Logical Minimum (400),
>   Logical Maximum (60000),
>   Unit (Centimeter^-2 * Candela),
>   Unit Exponent (14),
>   Report Size (32),
>   Report Count (1),
>   Feature (Variable, Null State),
> 
> The full HID descriptor dump is available as a comment in the source
> code.
> 
> Signed-off-by: Julius Zint <julius@zint.sh>
> ---
>  drivers/video/backlight/Kconfig  |   8 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/hid_bl.c | 269 +++++++++++++++++++++++++++++++
>  3 files changed, 278 insertions(+)
>  create mode 100644 drivers/video/backlight/hid_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..b964a820956d 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>  	  If you have a LCD backlight adjustable by LED class driver, say Y
>  	  to enable this driver.
>  
> +config BACKLIGHT_HID
> +	tristate "VESA VCP HID Backlight Driver"
> +	depends on HID
> +	help
> +	  If you have an external display with VESA compliant HID brightness
> +	  controls then say Y to enable this backlight driver. Currently the
> +	  only supported device is the Apple Studio Display.
> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endmenu
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index f72e1c3c59e9..835f9b8772c7 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
>  obj-$(CONFIG_BACKLIGHT_ARCXCNN) 	+= arcxcnn_bl.o
>  obj-$(CONFIG_BACKLIGHT_RAVE_SP)		+= rave-sp-backlight.o
>  obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
> +obj-$(CONFIG_BACKLIGHT_HID)		+= hid_bl.o
> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
> new file mode 100644
> index 000000000000..b40f8f412ee2
> --- /dev/null
> +++ b/drivers/video/backlight/hid_bl.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/backlight.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
> +
> +#define HID_USAGE_MONITOR_CTRL			0x800001
> +#define HID_USAGE_VESA_VCP_BRIGHTNESS		0x820010
> +
> +/*
> + * Apple Studio Display HID report descriptor
> + *
> + * Usage Page (Monitor),               ; USB monitor (80h, monitor page)
> + * Usage (01h),
> + * Collection (Application),
> + *     Report ID (1),
> + *
> + *     Usage Page (Monitor VESA VCP),  ; Monitor VESA virtual control panel (82h, monitor page)
> + *     Usage (10h, Brightness),
> + *     Logical Minimum (400),
> + *     Logical Maximum (60000),
> + *     Unit (Centimeter^-2 * Candela),
> + *     Unit Exponent (14),
> + *     Report Size (32),
> + *     Report Count (1),
> + *     Feature (Variable, Null State),
> + *
> + *     Usage Page (PID),               ; Physical interface device (0Fh)
> + *     Usage (50h),
> + *     Logical Minimum (0),
> + *     Logical Maximum (20000),
> + *     Unit (1001h),
> + *     Unit Exponent (13),
> + *     Report Size (16),
> + *     Feature (Variable, Null State),
> + *
> + *     Usage Page (Monitor VESA VCP),  ; Monitor VESA virtual control panel (82h, monitor page)
> + *     Usage (10h, Brightness),
> + *     Logical Minimum (400),
> + *     Logical Maximum (60000),
> + *     Unit (Centimeter^-2 * Candela),
> + *     Unit Exponent (14),
> + *     Report Size (32),
> + *     Report Count (1),
> + *     Input (Variable),
> + * End Collection
> + */
> +
> +struct hid_bl_data {
> +	struct hid_device *hdev;
> +	unsigned int min_brightness;
> +	unsigned int max_brightness;
> +	struct hid_field *input_field;
> +	struct hid_field *feature_field;
> +};
> +
> +static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
> +					     unsigned int report_type,
> +					     unsigned int application, unsigned int usage)
> +{
> +	struct hid_report_enum *re = &hdev->report_enum[report_type];
> +	struct hid_report *report;
> +	int i, j;
> +
> +	list_for_each_entry(report, &re->report_list, list) {
> +		if (report->application != application)
> +			continue;
> +
> +		for (i = 0; i < report->maxfield; i++) {
> +			struct hid_field *field = report->field[i];
> +
> +			for (j = 0; j < field->maxusage; j++)
> +				if (field->usage[j].hid == usage)
> +					return field;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +static void hid_bl_remove(struct hid_device *hdev)
> +{
> +	struct backlight_device *bl;
> +	struct hid_bl_data *data;
> +
> +	hid_dbg(hdev, "remove\n");
> +	bl = hid_get_drvdata(hdev);
> +	data = bl_get_data(bl);
> +
> +	devm_backlight_device_unregister(&hdev->dev, bl);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +	hid_set_drvdata(hdev, NULL);
> +	devm_kfree(&hdev->dev, data);
> +}
> +
> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
> +{
> +	struct hid_field *field;
> +	int result;
> +
> +	field = data->input_field;
> +	hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(data->hdev);
> +	result = *field->new_value;
> +	hid_dbg(data->hdev, "get brightness: %d\n", result);
> +
> +	return result;
> +}
> +
> +static int hid_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct hid_bl_data *data;
> +	int brightness;
> +
> +	data = bl_get_data(bl);
> +	brightness = hid_bl_get_brightness_raw(data);
> +	return brightness - data->min_brightness;
> +}
> +
> +static void hid_bl_set_brightness_raw(struct hid_bl_data *data, int brightness)
> +{
> +	struct hid_field *field;
> +
> +	field = data->feature_field;
> +	*field->value = brightness;
> +	hid_hw_request(data->hdev, field->report, HID_REQ_SET_REPORT);
> +	hid_hw_wait(data->hdev);
> +	hid_dbg(data->hdev, "set brightness: %d\n", brightness);
> +}
> +
> +static int hid_bl_update_status(struct backlight_device *bl)
> +{
> +	struct hid_bl_data *data;
> +	int brightness;
> +
> +	data = bl_get_data(bl);
> +	brightness = backlight_get_brightness(bl);
> +	brightness += data->min_brightness;
> +	hid_bl_set_brightness_raw(data, brightness);
> +	return 0;
> +}
> +
> +static const struct backlight_ops hid_bl_ops = {
> +	.update_status  = hid_bl_update_status,
> +	.get_brightness = hid_bl_get_brightness,
> +};
> +
> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +	struct hid_field *input_field;
> +	struct hid_field *feature_field;
> +	struct hid_bl_data *data;
> +	struct backlight_properties props;
> +	struct backlight_device *bl;
> +
> +	hid_dbg(hdev, "probe\n");
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "parse failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	input_field = hid_get_usage_field(hdev, HID_INPUT_REPORT,
> +					  HID_USAGE_MONITOR_CTRL,
> +					  HID_USAGE_VESA_VCP_BRIGHTNESS);
> +	if (input_field == NULL) {
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	feature_field = hid_get_usage_field(hdev, HID_FEATURE_REPORT,
> +					    HID_USAGE_MONITOR_CTRL,
> +					    HID_USAGE_VESA_VCP_BRIGHTNESS);
> +	if (feature_field == NULL) {
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	if (input_field->logical_minimum != feature_field->logical_minimum) {
> +		hid_warn(hdev, "minimums do not match: %d / %d\n",
> +			 input_field->logical_minimum,
> +			 feature_field->logical_minimum);
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	if (input_field->logical_maximum != feature_field->logical_maximum) {
> +		hid_warn(hdev, "maximums do not match: %d / %d\n",
> +			 input_field->logical_maximum,
> +			 feature_field->logical_maximum);
> +		ret = -ENODEV;
> +		goto exit_stop;
> +	}
> +
> +	hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw open failed: %d\n", ret);
> +		goto exit_stop;
> +	}
> +
> +	data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (data == NULL) {
> +		ret = -ENOMEM;
> +		goto exit_stop;
> +	}
> +	data->hdev = hdev;
> +	data->min_brightness = input_field->logical_minimum;
> +	data->max_brightness = input_field->logical_maximum;
> +	data->input_field = input_field;
> +	data->feature_field = feature_field;
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = data->max_brightness - data->min_brightness;
> +
> +	bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
> +					    &hdev->dev, data,
> +					    &hid_bl_ops,
> +					    &props);
> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		hid_err(hdev, "failed to register backlight: %d\n", ret);
> +		goto exit_free;
> +	}
> +
> +	hid_set_drvdata(hdev, bl);
> +
> +	return 0;
> +
> +exit_free:
> +	hid_hw_close(hdev);
> +	devm_kfree(&hdev->dev, data);
> +
> +exit_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static const struct hid_device_id hid_bl_devices[] = {
> +	{ HID_USB_DEVICE(APPLE_STUDIO_DISPLAY_VENDOR_ID,
> +			 APPLE_STUDIO_DISPLAY_PRODUCT_ID) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, hid_bl_devices);
> +
> +static struct hid_driver hid_bl_driver = {
> +	.name = "hid_backlight",
> +	.id_table = hid_bl_devices,
> +	.probe = hid_bl_probe,
> +	.remove = hid_bl_remove,
> +};
> +module_hid_driver(hid_bl_driver);
> +
> +MODULE_AUTHOR("Julius Zint <julius@zint.sh>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Backlight driver for HID devices");
> -- 
> 2.41.0
> 

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

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



On Mon, 4 Sep 2023, Thomas Weißschuh wrote:

> +Cc Hans who ins involved with the backlight subsystem
>
> Hi Julius,
>
> today I stumbled upon a mail from Hans [0], which explains that the
> backlight subsystem is not actually a good fit (yet?) for external
> displays.
>
> It seems a new API is in the works that would better fit, but I'm not
> sure about the state of this API. Maybe Hans can clarify.
>
> This also ties back to my review question how userspace can figure out
> to which display a backlight devices applies. So far it can not.
>
> [0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>

Hi Thomas,

thanks for the hint. I will make sure to give this a proper read and
see, if it fits my use case better then the current backlight subsystem.

Especially since I wasnt able to properly address your other review
comments for now. You are right that the name should align better with
the kernel module and also, that it is possible for multiple displays to
be attached.

In its current state, this would mean that you could only control the
backlight for the first HID device (enough for me :-).

The systemd-backlight@.service uses not only the file name, but also the
full bus path for storing/restoring backlights. I did not yet get around
to see how the desktops handle brightness control, but since the
systemd-backlight@.service already uses the name, its important to stay
the same over multiple boots.

I would be able to get a handle on the underlying USB device and use the
serial to uniquely (and persistently) name the backlight. But it does
feel hacky doing it this way.

Anyways, this is where am at. Thanks again for the support and I will
try my best to come up with something better.

Julius

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Hans de Goede]

Hi Julius,

On 9/4/23 21:02, Julius Zint wrote:
> 
> 
> On Mon, 4 Sep 2023, Thomas Weißschuh wrote:
> 
>> +Cc Hans who ins involved with the backlight subsystem
>>
>> Hi Julius,
>>
>> today I stumbled upon a mail from Hans [0], which explains that the
>> backlight subsystem is not actually a good fit (yet?) for external
>> displays.
>>
>> It seems a new API is in the works that would better fit, but I'm not
>> sure about the state of this API. Maybe Hans can clarify.
>>
>> This also ties back to my review question how userspace can figure out
>> to which display a backlight devices applies. So far it can not.
>>
>> [0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>>
> 
> Hi Thomas,
> 
> thanks for the hint. I will make sure to give this a proper read and
> see, if it fits my use case better then the current backlight subsystem.

Note the actual proposal for the new usespace API for display brightness
control is here:

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

I have finished / stabilized the backlight code refactor which I landed
in 6.1, which is a prerequisite for the above proposal. But I have not
been able to make time to actually implement the above proposal; and
I don't know when I will be able to make time for this.

> Especially since I wasnt able to properly address your other review
> comments for now. You are right that the name should align better with
> the kernel module and also, that it is possible for multiple displays to
> be attached.
> 
> In its current state, this would mean that you could only control the
> backlight for the first HID device (enough for me :-).
> 
> The systemd-backlight@.service uses not only the file name, but also the
> full bus path for storing/restoring backlights. I did not yet get around
> to see how the desktops handle brightness control, but since the
> systemd-backlight@.service already uses the name, its important to stay
> the same over multiple boots.
> 
> I would be able to get a handle on the underlying USB device and use the
> serial to uniquely (and persistently) name the backlight. But it does
> feel hacky doing it this way.

So mutter (gnome-shell compositor library) has a similar issue when saving
monitor layouts and I can tell you beforehand that monitor serial numbers
by themselves are not unique enough. Some models just report 123456789
as serial and if you have a dual-monitor setup with 2 such monitors
and name the backlight class device <serial>-vcp-hid or something like that
you will still end up with 2 identical names.

To avoid this when saving monitor layouts mutter saves both the port
to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber
and on startup / monitor hotplug when it checks to see if it has saved
layout info for the monitor it checks the port+serialnr combination.

So what I think you should do is figure out a way to map which
VCP HID device maps to which drm-connector and then use
the connector-name + serial-nr to generate the backlight device name.

We will need the mapping the a drm-connector object anyway for
the new brightness API proposal from above.

Note this does NOT solve the fact that registering a new backlight
class device for an external monitor on a laptop will hopelessly
confuse userspace, see:

https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/

Regards,

Hans

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

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



On Wed, 6 Sep 2023, Hans de Goede wrote:

> Hi Julius,
> 
> On 9/4/23 21:02, Julius Zint wrote:
> > 
> > 
> > On Mon, 4 Sep 2023, Thomas Weißschuh wrote:
> > 
> >> +Cc Hans who ins involved with the backlight subsystem
> >>
> >> Hi Julius,
> >>
> >> today I stumbled upon a mail from Hans [0], which explains that the
> >> backlight subsystem is not actually a good fit (yet?) for external
> >> displays.
> >>
> >> It seems a new API is in the works that would better fit, but I'm not
> >> sure about the state of this API. Maybe Hans can clarify.
> >>
> >> This also ties back to my review question how userspace can figure out
> >> to which display a backlight devices applies. So far it can not.
> >>
> >> [0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
> >>
> > 
> > Hi Thomas,
> > 
> > thanks for the hint. I will make sure to give this a proper read and
> > see, if it fits my use case better then the current backlight subsystem.
> 
> Note the actual proposal for the new usespace API for display brightness
> control is here:
> 
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
> 
> I have finished / stabilized the backlight code refactor which I landed
> in 6.1, which is a prerequisite for the above proposal. But I have not
> been able to make time to actually implement the above proposal; and
> I don't know when I will be able to make time for this.
> 
> > Especially since I wasnt able to properly address your other review
> > comments for now. You are right that the name should align better with
> > the kernel module and also, that it is possible for multiple displays to
> > be attached.
> > 
> > In its current state, this would mean that you could only control the
> > backlight for the first HID device (enough for me :-).
> > 
> > The systemd-backlight@.service uses not only the file name, but also the
> > full bus path for storing/restoring backlights. I did not yet get around
> > to see how the desktops handle brightness control, but since the
> > systemd-backlight@.service already uses the name, its important to stay
> > the same over multiple boots.
> > 
> > I would be able to get a handle on the underlying USB device and use the
> > serial to uniquely (and persistently) name the backlight. But it does
> > feel hacky doing it this way.
> 
> So mutter (gnome-shell compositor library) has a similar issue when saving
> monitor layouts and I can tell you beforehand that monitor serial numbers
> by themselves are not unique enough. Some models just report 123456789
> as serial and if you have a dual-monitor setup with 2 such monitors
> and name the backlight class device <serial>-vcp-hid or something like that
> you will still end up with 2 identical names.
> 
> To avoid this when saving monitor layouts mutter saves both the port
> to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber
> and on startup / monitor hotplug when it checks to see if it has saved
> layout info for the monitor it checks the port+serialnr combination.
> 
> So what I think you should do is figure out a way to map which
> VCP HID device maps to which drm-connector and then use
> the connector-name + serial-nr to generate the backlight device name.
> 
> We will need the mapping the a drm-connector object anyway for
> the new brightness API proposal from above.
> 
> Note this does NOT solve the fact that registering a new backlight
> class device for an external monitor on a laptop will hopelessly
> confuse userspace, see:
> 
> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
> 
> Regards,
> 
> Hans
> 

Thank you for all this additional information. I have watched the talks
and read up upon the mail threads you`ve linked.

I will see if I can make the mapping to the DRM connector and plan to
update this patchset.

Thanks,

Julius

Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Hans de Goede]

Hi,

On 9/19/23 19:46, Julius Zint wrote:
> 
> 
> On Wed, 6 Sep 2023, Hans de Goede wrote:
> 
>> Hi Julius,
>>
>> On 9/4/23 21:02, Julius Zint wrote:
>>>
>>>
>>> On Mon, 4 Sep 2023, Thomas Weißschuh wrote:
>>>
>>>> +Cc Hans who ins involved with the backlight subsystem
>>>>
>>>> Hi Julius,
>>>>
>>>> today I stumbled upon a mail from Hans [0], which explains that the
>>>> backlight subsystem is not actually a good fit (yet?) for external
>>>> displays.
>>>>
>>>> It seems a new API is in the works that would better fit, but I'm not
>>>> sure about the state of this API. Maybe Hans can clarify.
>>>>
>>>> This also ties back to my review question how userspace can figure out
>>>> to which display a backlight devices applies. So far it can not.
>>>>
>>>> [0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>>>>
>>>
>>> Hi Thomas,
>>>
>>> thanks for the hint. I will make sure to give this a proper read and
>>> see, if it fits my use case better then the current backlight subsystem.
>>
>> Note the actual proposal for the new usespace API for display brightness
>> control is here:
>>
>> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
>>
>> I have finished / stabilized the backlight code refactor which I landed
>> in 6.1, which is a prerequisite for the above proposal. But I have not
>> been able to make time to actually implement the above proposal; and
>> I don't know when I will be able to make time for this.
>>
>>> Especially since I wasnt able to properly address your other review
>>> comments for now. You are right that the name should align better with
>>> the kernel module and also, that it is possible for multiple displays to
>>> be attached.
>>>
>>> In its current state, this would mean that you could only control the
>>> backlight for the first HID device (enough for me :-).
>>>
>>> The systemd-backlight@.service uses not only the file name, but also the
>>> full bus path for storing/restoring backlights. I did not yet get around
>>> to see how the desktops handle brightness control, but since the
>>> systemd-backlight@.service already uses the name, its important to stay
>>> the same over multiple boots.
>>>
>>> I would be able to get a handle on the underlying USB device and use the
>>> serial to uniquely (and persistently) name the backlight. But it does
>>> feel hacky doing it this way.
>>
>> So mutter (gnome-shell compositor library) has a similar issue when saving
>> monitor layouts and I can tell you beforehand that monitor serial numbers
>> by themselves are not unique enough. Some models just report 123456789
>> as serial and if you have a dual-monitor setup with 2 such monitors
>> and name the backlight class device <serial>-vcp-hid or something like that
>> you will still end up with 2 identical names.
>>
>> To avoid this when saving monitor layouts mutter saves both the port
>> to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber
>> and on startup / monitor hotplug when it checks to see if it has saved
>> layout info for the monitor it checks the port+serialnr combination.
>>
>> So what I think you should do is figure out a way to map which
>> VCP HID device maps to which drm-connector and then use
>> the connector-name + serial-nr to generate the backlight device name.
>>
>> We will need the mapping the a drm-connector object anyway for
>> the new brightness API proposal from above.
>>
>> Note this does NOT solve the fact that registering a new backlight
>> class device for an external monitor on a laptop will hopelessly
>> confuse userspace, see:
>>
>> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>>
>> Regards,
>>
>> Hans
>>
> 
> Thank you for all this additional information. I have watched the talks
> and read up upon the mail threads you`ve linked.

Now I wonder which presentation you have watched, did you watch
the old XDC2014 presentation ?  Note I gave a much more up2date
presentation on this at kernel-recipes last year:

https://kernel-recipes.org/en/2022/talks/new-userspace-api-for-display-panel-brightness-control/

> I will see if I can make the mapping to the DRM connector and plan to
> update this patchset.

Sounds good.

Regards,

Hans