From: Christoffer Sandberg <[email protected]>
Implement per-key keyboard backlight in the leds sysfs interface for
several TUXEDO devices using the ite8291 controller.
There are however some known short comings:
- The sysfs leds interface does only allow to write one key at a time. The
controller however can only update row wise or the whole keyboard at once
(whole keyboard update is currently not implemented). This means that even
when you want to updated a whole row, the whole row is actually updated
once for each key. So you actually write up to 18x as much as would be
required.
- When you want to update the brightness of the whole keyboard you have to
write 126 sysfs entries, which inherently is somewhat slow, especially when
using a slider that is live updating the brightness.
- While the controller manages up to 126 leds, not all are actually
populated. However the unused are not grouped at the end but also in
between. To not have dead sysfs entries, this would require manual testing
for each implemented device to see which leds are used and some kind of
bitmap in the driver to sort out the unconnected ones.
- It is not communicated to userspace which led entry controls which key
exactly
Co-developed-by: Werner Sembach <[email protected]>
Signed-off-by: Werner Sembach <[email protected]>
Signed-off-by: Christoffer Sandberg <[email protected]>
---
drivers/leds/rgb/Kconfig | 9 +
drivers/leds/rgb/Makefile | 1 +
drivers/leds/rgb/leds-tuxedo-ite8291.c | 451 +++++++++++++++++++++++++
3 files changed, 461 insertions(+)
create mode 100644 drivers/leds/rgb/leds-tuxedo-ite8291.c
diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 183bccc06cf3..c263ae94c137 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -51,4 +51,13 @@ config LEDS_MT6370_RGB
This driver can also be built as a module. If so, the module
will be called "leds-mt6370-rgb".
+config LEDS_TUXEDO_ITE8291
+ tristate "KBL Support for TUXEDO devices using ite8291"
+ help
+ Say Y here to enable keyboard backlight support for TUXEDO devices
+ using ite8291.
+
+ This driver can also be built as a module. If so, the module
+ will be called "leds-tuxedo-ite8291".
+
endif # LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
index c11cc56384e7..5a7447609732 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o
obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o
obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o
obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.o
+obj-$(CONFIG_LEDS_TUXEDO_ITE8291) += leds-tuxedo-ite8291.o
diff --git a/drivers/leds/rgb/leds-tuxedo-ite8291.c b/drivers/leds/rgb/leds-tuxedo-ite8291.c
new file mode 100644
index 000000000000..b77d45804cd0
--- /dev/null
+++ b/drivers/leds/rgb/leds-tuxedo-ite8291.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020-2022 TUXEDO Computers GmbH <[email protected]>
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/usb.h>
+#include <linux/hid.h>
+#include <linux/dmi.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/of.h>
+
+#define LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_MAX 0x32
+#define LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_DEFAULT 0x32
+
+#define LEDS_TUXEDO_ITE8291_BRIGHTNESS_MAX 0xff
+#define LEDS_TUXEDO_ITE8291_BRIGHTNESS_DEFAULT 0x00
+
+#define LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_RED 0xff
+#define LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_GREEN 0xff
+#define LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_BLUE 0xff
+
+#define LEDS_TUXEDO_ITE8291_ROWS 6
+#define LEDS_TUXEDO_ITE8291_COLUMNS 21
+
+// Data length needs one byte (0x00) initial padding for the sending function and one byte (also
+// seemingly 0x00) before the color data starts
+#define LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING (1 + 1)
+#define LEDS_TUXEDO_ITE8291_ROW_DATA_LENGTH (LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + \
+ (LEDS_TUXEDO_ITE8291_COLUMNS * 3))
+
+#define LEDS_TUXEDO_ITE8291_PARAM_MODE_USER 0x33
+
+typedef u8 row_data_t[LEDS_TUXEDO_ITE8291_ROWS][LEDS_TUXEDO_ITE8291_ROW_DATA_LENGTH];
+
+struct leds_tuxedo_ite8291_driver_data_t {
+ row_data_t row_data;
+ struct led_classdev_mc mcled_cdevs[LEDS_TUXEDO_ITE8291_ROWS][LEDS_TUXEDO_ITE8291_COLUMNS];
+ struct mc_subled mcled_cdevs_subleds[LEDS_TUXEDO_ITE8291_ROWS][LEDS_TUXEDO_ITE8291_COLUMNS][3];
+};
+
+/**
+ * Set color for specified [row, column] in row based data structure
+ *
+ * @param row_data Data structure to fill
+ * @param row Row number 0 - 5
+ * @param column Column number 0 - 20
+ * @param red Red brightness 0x00 - 0xff
+ * @param green Green brightness 0x00 - 0xff
+ * @param blue Blue brightness 0x00 - 0xff
+ *
+ * @returns 0 on success, otherwise error
+ */
+static int leds_tuxedo_ite8291_set_row_data(row_data_t row_data, int row, int column,
+ u8 red, u8 green, u8 blue)
+{
+ int column_index_red, column_index_green, column_index_blue;
+
+ if (row < 0 || row >= LEDS_TUXEDO_ITE8291_ROWS ||
+ column < 0 || column >= LEDS_TUXEDO_ITE8291_COLUMNS)
+ return -EINVAL;
+
+ column_index_red =
+ LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (2 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
+ column_index_green =
+ LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (1 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
+ column_index_blue =
+ LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (0 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
+
+ row_data[row][column_index_red] = red;
+ row_data[row][column_index_green] = green;
+ row_data[row][column_index_blue] = blue;
+
+ return 0;
+}
+
+/**
+ * Just a generic helper function to reduce boilerplate code
+ */
+static int leds_tuxedo_ite8291_hid_feature_report_set(struct hid_device *hdev, u8 *data, size_t len)
+{
+ int result;
+ u8 *buf;
+
+ buf = kmemdup(data, len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ result = hid_hw_raw_request(hdev, buf[0], buf, len, HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+ kfree(buf);
+
+ return result;
+}
+
+/**
+ * Update brightness of the whole keyboard. Only used for initialization as this doesn't allow per
+ * key brightness control. That is implemented with RGB value scaling.
+ */
+static int leds_tuxedo_ite8291_write_brightness(struct hid_device *hdev, u8 brightness)
+{
+ int result;
+ u8 brightness_capped = brightness > LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_MAX ?
+ LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_MAX : brightness;
+ u8 ctrl_set_brightness[8] = {0x08, 0x02, LEDS_TUXEDO_ITE8291_PARAM_MODE_USER, 0x00,
+ brightness_capped, 0x00, 0x00, 0x00};
+
+ result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_set_brightness,
+ sizeof(ctrl_set_brightness));
+ if (result < 0)
+ return result;
+
+ return 0;
+}
+
+/**
+ * Update color of a singular row from row_data. This is the smallest unit this device allows to
+ * write. Changes are applied when the last row (row_index == 5) is written.
+ */
+static int leds_tuxedo_ite8291_write_row(struct hid_device *hdev, row_data_t row_data,
+ int row_index)
+{
+ int result;
+ u8 ctrl_announce_row_data[8] = {0x16, 0x00, row_index, 0x00, 0x00, 0x00, 0x00, 0x00};
+
+ result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_announce_row_data,
+ sizeof(ctrl_announce_row_data));
+ if (result < 0)
+ return result;
+
+ result = hid_hw_output_report(hdev, row_data[row_index], sizeof(row_data[row_index]));
+ if (result < 0)
+ return result;
+
+ return 0;
+}
+
+/**
+ * Write color and brightness to the whole keyboard from row data. Note that per key brightness is
+ * encoded in the RGB values of the row_data and the gobal brightness is a fixed value.
+ */
+static int leds_tuxedo_ite8291_write_all(struct hid_device *hdev, row_data_t row_data)
+{
+ int result, row_index;
+
+ if (hdev == NULL)
+ return -ENODEV;
+
+ result = leds_tuxedo_ite8291_write_brightness(hdev,
+ LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_DEFAULT);
+ if (result < 0)
+ return result;
+
+ for (row_index = 0; row_index < LEDS_TUXEDO_ITE8291_ROWS; ++row_index) {
+ result = leds_tuxedo_ite8291_write_row(hdev, row_data, row_index);
+ if (result < 0)
+ return result;
+ }
+
+ return 0;
+}
+
+static int leds_tuxedo_ite8291_write_state(struct hid_device *hdev)
+{
+ struct leds_tuxedo_ite8291_driver_data_t *driver_data = hid_get_drvdata(hdev);
+
+ return leds_tuxedo_ite8291_write_all(hdev, driver_data->row_data);
+}
+
+static int leds_tuxedo_ite8291_write_off(struct hid_device *hdev)
+{
+ int result;
+ u8 ctrl_write_off[8] = {0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+
+ result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_write_off,
+ sizeof(ctrl_write_off));
+ if (result < 0)
+ return result;
+
+ return 0;
+}
+
+static int leds_tuxedo_ite8291_start_hw(struct hid_device *hdev)
+{
+ int result;
+
+ result = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (result < 0)
+ goto err_start;
+
+ result = hid_hw_power(hdev, PM_HINT_FULLON);
+ if (result < 0)
+ goto err_power;
+
+ result = hid_hw_open(hdev);
+ if (result)
+ goto err_open;
+
+ return 0;
+
+err_open:
+ hid_hw_power(hdev, PM_HINT_NORMAL);
+err_power:
+ hid_hw_stop(hdev);
+err_start:
+ return result;
+}
+
+static void leds_tuxedo_ite8291_stop_hw(struct hid_device *hdev)
+{
+ hid_hw_close(hdev);
+ hid_hw_power(hdev, PM_HINT_NORMAL);
+ hid_hw_stop(hdev);
+}
+
+static void leds_tuxedo_ite8291_leds_set_brightness_mc(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ int result, row, column;
+ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+ struct device *dev = led_cdev->dev->parent;
+ struct hid_device *hdev = to_hid_device(dev);
+ struct leds_tuxedo_ite8291_driver_data_t *driver_data = hid_get_drvdata(hdev);
+
+ led_mc_calc_color_components(mcled_cdev, brightness);
+
+ row = mcled_cdev->subled_info[0].channel / LEDS_TUXEDO_ITE8291_COLUMNS;
+ column = mcled_cdev->subled_info[0].channel % LEDS_TUXEDO_ITE8291_COLUMNS;
+
+ result = leds_tuxedo_ite8291_set_row_data(driver_data->row_data, row, column,
+ mcled_cdev->subled_info[0].brightness,
+ mcled_cdev->subled_info[1].brightness,
+ mcled_cdev->subled_info[2].brightness);
+ if (result < 0)
+ return;
+
+ // As a performance optimization, try to write the smallest required unit
+ result = leds_tuxedo_ite8291_write_row(hdev, driver_data->row_data, row);
+ if (result < 0)
+ return;
+
+ // Changes are applied on every write of the last row. So, if a different row was written,
+ // also write the last row.
+ if (row != LEDS_TUXEDO_ITE8291_ROWS - 1) {
+ result = leds_tuxedo_ite8291_write_row(hdev, driver_data->row_data,
+ LEDS_TUXEDO_ITE8291_ROWS - 1);
+ if (result < 0)
+ return;
+ }
+
+ led_cdev->brightness = brightness;
+}
+
+static int leds_tuxedo_ite8291_register_leds(struct hid_device *hdev)
+{
+ int result, i, j, k, l;
+ struct leds_tuxedo_ite8291_driver_data_t *driver_data = hid_get_drvdata(hdev);
+
+ for (i = 0; i < LEDS_TUXEDO_ITE8291_ROWS; ++i) {
+ for (j = 0; j < LEDS_TUXEDO_ITE8291_COLUMNS; ++j) {
+ driver_data->mcled_cdevs[i][j].led_cdev.name =
+ "rgb:" LED_FUNCTION_KBD_BACKLIGHT;
+ driver_data->mcled_cdevs[i][j].led_cdev.max_brightness =
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_MAX;
+ driver_data->mcled_cdevs[i][j].led_cdev.brightness_set =
+ &leds_tuxedo_ite8291_leds_set_brightness_mc;
+ driver_data->mcled_cdevs[i][j].led_cdev.brightness =
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_DEFAULT;
+ driver_data->mcled_cdevs[i][j].num_colors =
+ 3;
+ driver_data->mcled_cdevs[i][j].subled_info =
+ driver_data->mcled_cdevs_subleds[i][j];
+ driver_data->mcled_cdevs[i][j].subled_info[0].color_index =
+ LED_COLOR_ID_RED;
+ driver_data->mcled_cdevs[i][j].subled_info[0].intensity =
+ LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_RED;
+ driver_data->mcled_cdevs[i][j].subled_info[0].channel =
+ LEDS_TUXEDO_ITE8291_COLUMNS * i + j;
+ driver_data->mcled_cdevs[i][j].subled_info[1].color_index =
+ LED_COLOR_ID_GREEN;
+ driver_data->mcled_cdevs[i][j].subled_info[1].intensity =
+ LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_GREEN;
+ driver_data->mcled_cdevs[i][j].subled_info[1].channel =
+ LEDS_TUXEDO_ITE8291_COLUMNS * i + j;
+ driver_data->mcled_cdevs[i][j].subled_info[2].color_index =
+ LED_COLOR_ID_BLUE;
+ driver_data->mcled_cdevs[i][j].subled_info[2].intensity =
+ LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_BLUE;
+ driver_data->mcled_cdevs[i][j].subled_info[2].channel =
+ LEDS_TUXEDO_ITE8291_COLUMNS * i + j;
+
+ result = devm_led_classdev_multicolor_register(&hdev->dev,
+ &driver_data->mcled_cdevs[i][j]);
+ if (result < 0) {
+ for (k = 0; k <= i; ++k) {
+ for (l = 0; l <= j; ++l) {
+ devm_led_classdev_multicolor_unregister(&hdev->dev,
+ &driver_data->mcled_cdevs[i][j]);
+ }
+ }
+ return result;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static void leds_tuxedo_ite8291_unregister_leds(struct hid_device *hdev)
+{
+ int i, j;
+ struct leds_tuxedo_ite8291_driver_data_t *driver_data = hid_get_drvdata(hdev);
+
+ for (i = 0; i < LEDS_TUXEDO_ITE8291_ROWS; ++i) {
+ for (j = 0; j < LEDS_TUXEDO_ITE8291_COLUMNS; ++j) {
+ devm_led_classdev_multicolor_unregister(&hdev->dev,
+ &driver_data->mcled_cdevs[i][j]);
+ }
+ }
+}
+
+static int leds_tuxedo_ite8291_device_add(struct hid_device *hdev)
+{
+ int result, i, j;
+ u8 default_brightness_red, default_brightness_green, default_brightness_blue;
+ struct leds_tuxedo_ite8291_driver_data_t *driver_data;
+
+ driver_data = devm_kzalloc(&hdev->dev, sizeof(*driver_data), GFP_KERNEL);
+ if (!driver_data)
+ return -ENOMEM;
+ hid_set_drvdata(hdev, driver_data);
+
+ default_brightness_red = LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_RED *
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_DEFAULT /
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_MAX;
+ default_brightness_green = LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_GREEN *
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_DEFAULT /
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_MAX;
+ default_brightness_blue = LEDS_TUXEDO_ITE8291_COLOR_DEFAULT_BLUE *
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_DEFAULT /
+ LEDS_TUXEDO_ITE8291_BRIGHTNESS_MAX;
+ for (i = 0; i < LEDS_TUXEDO_ITE8291_ROWS; ++i) {
+ for (j = 0; j < LEDS_TUXEDO_ITE8291_COLUMNS; ++j) {
+ leds_tuxedo_ite8291_set_row_data(driver_data->row_data, i, j,
+ default_brightness_red,
+ default_brightness_green,
+ default_brightness_blue);
+ }
+ }
+
+ result = leds_tuxedo_ite8291_write_state(hdev);
+ if (result < 0)
+ return result;
+
+ result = leds_tuxedo_ite8291_register_leds(hdev);
+ if (result < 0)
+ return result;
+
+ return 0;
+}
+
+static int leds_tuxedo_ite8291_device_remove(struct hid_device *hdev)
+{
+ leds_tuxedo_ite8291_unregister_leds(hdev);
+ leds_tuxedo_ite8291_write_off(hdev);
+
+ return 0;
+}
+
+static const struct dmi_system_id leds_tuxedo_ite8291_whitelist[] = {
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "TUXEDO"),
+ },
+ },
+ { }
+};
+
+static int leds_tuxedo_ite8291_probe(struct hid_device *hdev,
+ const struct hid_device_id *id __always_unused)
+{
+ int result;
+
+ // The ite8291 can be used quite differently. To ensure that this driver doesn't do bogus
+ // things, limit it to tested devices. Done via DMI matching as for the time being this
+ // driver is for internal keyboard backlights only.
+ if (!dmi_check_system(leds_tuxedo_ite8291_whitelist))
+ return -ENODEV;
+
+ result = hid_parse(hdev);
+ if (result < 0)
+ return result;
+
+ result = leds_tuxedo_ite8291_start_hw(hdev);
+ if (result < 0)
+ return result;
+
+ result = leds_tuxedo_ite8291_device_add(hdev);
+ if (result != 0)
+ return result;
+
+ return 0;
+}
+
+static void leds_tuxedo_ite8291_remove(struct hid_device *hdev)
+{
+ leds_tuxedo_ite8291_device_remove(hdev);
+ leds_tuxedo_ite8291_stop_hw(hdev);
+}
+
+#ifdef CONFIG_PM
+static int leds_tuxedo_ite8291_suspend(struct hid_device *hdev,
+ pm_message_t message __always_unused)
+{
+ return leds_tuxedo_ite8291_write_off(hdev);
+}
+
+static int leds_tuxedo_ite8291_resume(struct hid_device *hdev)
+{
+ return leds_tuxedo_ite8291_write_state(hdev);
+}
+
+static int leds_tuxedo_ite8291_reset_resume(struct hid_device *hdev)
+{
+ return leds_tuxedo_ite8291_write_state(hdev);
+}
+#endif
+
+static const struct hid_device_id leds_tuxedo_ite8291_id_table[] = {
+ // Tongfang internal keyboard backlights
+ { HID_USB_DEVICE(0x048d, 0x6004) },
+ { HID_USB_DEVICE(0x048d, 0x600a) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, leds_tuxedo_ite8291_id_table);
+
+static struct hid_driver leds_tuxedo_ite8291 = {
+ .name = "leds-tuxedo-ite8291",
+ .id_table = leds_tuxedo_ite8291_id_table,
+ .probe = leds_tuxedo_ite8291_probe,
+ .remove = leds_tuxedo_ite8291_remove,
+#ifdef CONFIG_PM
+ .suspend = leds_tuxedo_ite8291_suspend,
+ .resume = leds_tuxedo_ite8291_resume,
+ .reset_resume = leds_tuxedo_ite8291_reset_resume
+#endif
+};
+module_hid_driver(leds_tuxedo_ite8291);
+
+MODULE_AUTHOR("TUXEDO Computers GmbH <[email protected]>");
+MODULE_DESCRIPTION("Driver for ITE Device(8291) RGB LED keyboard backlight.");
+MODULE_LICENSE("GPL");
--
2.34.1
Hi,
Am 11.10.23 um 21:00 schrieb Werner Sembach:
> From: Christoffer Sandberg <[email protected]>
>
> Implement per-key keyboard backlight in the leds sysfs interface for
> several TUXEDO devices using the ite8291 controller.
>
> There are however some known short comings:
> - The sysfs leds interface does only allow to write one key at a time. The
> controller however can only update row wise or the whole keyboard at once
> (whole keyboard update is currently not implemented). This means that even
> when you want to updated a whole row, the whole row is actually updated
> once for each key. So you actually write up to 18x as much as would be
> required.
> - When you want to update the brightness of the whole keyboard you have to
> write 126 sysfs entries, which inherently is somewhat slow, especially when
> using a slider that is live updating the brightness.
> - While the controller manages up to 126 leds, not all are actually
> populated. However the unused are not grouped at the end but also in
> between. To not have dead sysfs entries, this would require manual testing
> for each implemented device to see which leds are used and some kind of
> bitmap in the driver to sort out the unconnected ones.
> - It is not communicated to userspace which led entry controls which key
> exactly
>
> Co-developed-by: Werner Sembach <[email protected]>
> Signed-off-by: Werner Sembach <[email protected]>
> Signed-off-by: Christoffer Sandberg <[email protected]>
The first time I submit a whole module, so please let me know if it made any
mistakes e.g. I'm unsure if I need add myself explicitly as a maintainer, if
MODULE_AUTHOR has to be a human, or if i have to split this up into smaller junks.
Also please let me know if i somehow misinterpreted the current API and the
shortcomings can actually be avoided.
I have not yet looked deeply into triggers, but one idea i had is to only have
one kbd_backlight by default that just makes the whole keyboard the same color
and brightness. In addition to that a trigger per_key_control, that, when set,
adds 125*3 subleds to write the whole keyboard in rainbow colors with a single
echo to multi_intensity.
The keyboard also supports hardware color effects like color cycle, which
continuously and smoothly cycles through up to 7 colors. Could this also be
implemented with a trigger? That trigger would need to add a new entry nr_colors
and also respectively additional subleds or additional multi_intensity_* entries.
An additional though I had was that it would be nice if the driver could somehow
communicate the physical location of the key to the userspace for UIs to
automatically generate a keyboard view to graphically set individual colors.
Kind regards,
Werner Sembach
Hi!
> From: Christoffer Sandberg <[email protected]>
>
> Implement per-key keyboard backlight in the leds sysfs interface for
> several TUXEDO devices using the ite8291 controller.
>
> There are however some known short comings:
> - The sysfs leds interface does only allow to write one key at a time. The
> controller however can only update row wise or the whole keyboard at once
> (whole keyboard update is currently not implemented). This means that even
> when you want to updated a whole row, the whole row is actually updated
> once for each key. So you actually write up to 18x as much as would be
> required.
> - When you want to update the brightness of the whole keyboard you have to
> write 126 sysfs entries, which inherently is somewhat slow, especially when
> using a slider that is live updating the brightness.
> - While the controller manages up to 126 leds, not all are actually
> populated. However the unused are not grouped at the end but also in
> between. To not have dead sysfs entries, this would require manual testing
> for each implemented device to see which leds are used and some kind of
> bitmap in the driver to sort out the unconnected ones.
> - It is not communicated to userspace which led entry controls which key
> exactly
Sooner or later, we'll need to support these keyboards.
But this has way too many shortcomings (and we'd be stuck with the
interface forever).
These days, displays with weird shapes are common. Like rounded
corners and holes in them. Perhaps this should be better modelled as a
weird display?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 12.10.23 um 10:58 schrieb Pavel Machek:
> Hi!
>
>> From: Christoffer Sandberg <[email protected]>
>>
>> Implement per-key keyboard backlight in the leds sysfs interface for
>> several TUXEDO devices using the ite8291 controller.
>>
>> There are however some known short comings:
>> - The sysfs leds interface does only allow to write one key at a time. The
>> controller however can only update row wise or the whole keyboard at once
>> (whole keyboard update is currently not implemented). This means that even
>> when you want to updated a whole row, the whole row is actually updated
>> once for each key. So you actually write up to 18x as much as would be
>> required.
>> - When you want to update the brightness of the whole keyboard you have to
>> write 126 sysfs entries, which inherently is somewhat slow, especially when
>> using a slider that is live updating the brightness.
>> - While the controller manages up to 126 leds, not all are actually
>> populated. However the unused are not grouped at the end but also in
>> between. To not have dead sysfs entries, this would require manual testing
>> for each implemented device to see which leds are used and some kind of
>> bitmap in the driver to sort out the unconnected ones.
>> - It is not communicated to userspace which led entry controls which key
>> exactly
> Sooner or later, we'll need to support these keyboards.
>
> But this has way too many shortcomings (and we'd be stuck with the
> interface forever).
I had some thoughts on how the current userspace api could be expanded to better
reflect the capabilities of RGB keyboards. What would be required for such an
expansion to be considered?
I'm in contact with the KDE folks. Plasma already has a keyboard brightness
slider, that soon
https://gitlab.freedesktop.org/upower/upower/-/merge_requests/203 will work with
multiple kbd_backlight. However the slowness of 126 sysfs entries makes it a
little bit janky still.
They are also thinking about expanding desktop accent colors to the keyboard
backlight when it supports RGB.
I have not reached out to the OpenRGB project yet, but is it alive and well and
under constant development: https://gitlab.com/CalcProgrammer1/OpenRGB. Afaik it
is currently a userspace only driver interacting directly with hidraw mostly and
has not yet implemented the sysfs leds interface.
Just listing this to show that there is definitely interest in this.
>
> These days, displays with weird shapes are common. Like rounded
> corners and holes in them. Perhaps this should be better modelled as a
> weird display?
I'm not sure if I can follow you here. Where would this be implemented? Also I
asume displays asume equal distance between pixels and that columns are straight
lines perpendicular to rows, which the key backlights have and are not.
Kind regards,
Werner
>
> Best regards,
> Pavel
Hi Werner,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-leds/for-leds-next]
[also build test WARNING on linus/master v6.6-rc5 next-20231012]
[cannot apply to pavel-leds/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Werner-Sembach/leds-rgb-Implement-per-key-keyboard-backlight-for-several-TUXEDO-devices/20231012-030206
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next
patch link: https://lore.kernel.org/r/20231011190017.1230898-1-wse%40tuxedocomputers.com
patch subject: [PATCH] leds: rgb: Implement per-key keyboard backlight for several TUXEDO devices
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20231012/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231012/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:44: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Set color for specified [row, column] in row based data structure
drivers/leds/rgb/leds-tuxedo-ite8291.c:79: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Just a generic helper function to reduce boilerplate code
drivers/leds/rgb/leds-tuxedo-ite8291.c:96: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Update brightness of the whole keyboard. Only used for initialization as this doesn't allow per
drivers/leds/rgb/leds-tuxedo-ite8291.c:116: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Update color of a singular row from row_data. This is the smallest unit this device allows to
drivers/leds/rgb/leds-tuxedo-ite8291.c:138: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Write color and brightness to the whole keyboard from row data. Note that per key brightness is
vim +44 drivers/leds/rgb/leds-tuxedo-ite8291.c
42
43 /**
> 44 * Set color for specified [row, column] in row based data structure
45 *
46 * @param row_data Data structure to fill
47 * @param row Row number 0 - 5
48 * @param column Column number 0 - 20
49 * @param red Red brightness 0x00 - 0xff
50 * @param green Green brightness 0x00 - 0xff
51 * @param blue Blue brightness 0x00 - 0xff
52 *
53 * @returns 0 on success, otherwise error
54 */
55 static int leds_tuxedo_ite8291_set_row_data(row_data_t row_data, int row, int column,
56 u8 red, u8 green, u8 blue)
57 {
58 int column_index_red, column_index_green, column_index_blue;
59
60 if (row < 0 || row >= LEDS_TUXEDO_ITE8291_ROWS ||
61 column < 0 || column >= LEDS_TUXEDO_ITE8291_COLUMNS)
62 return -EINVAL;
63
64 column_index_red =
65 LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (2 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
66 column_index_green =
67 LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (1 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
68 column_index_blue =
69 LEDS_TUXEDO_ITE8291_ROW_DATA_PADDING + (0 * LEDS_TUXEDO_ITE8291_COLUMNS) + column;
70
71 row_data[row][column_index_red] = red;
72 row_data[row][column_index_green] = green;
73 row_data[row][column_index_blue] = blue;
74
75 return 0;
76 }
77
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi!
> > > There are however some known short comings:
> > > - The sysfs leds interface does only allow to write one key at a time. The
> > > controller however can only update row wise or the whole keyboard at once
> > > (whole keyboard update is currently not implemented). This means that even
> > > when you want to updated a whole row, the whole row is actually updated
> > > once for each key. So you actually write up to 18x as much as would be
> > > required.
> > > - When you want to update the brightness of the whole keyboard you have to
> > > write 126 sysfs entries, which inherently is somewhat slow, especially when
> > > using a slider that is live updating the brightness.
> > > - While the controller manages up to 126 leds, not all are actually
> > > populated. However the unused are not grouped at the end but also in
> > > between. To not have dead sysfs entries, this would require manual testing
> > > for each implemented device to see which leds are used and some kind of
> > > bitmap in the driver to sort out the unconnected ones.
> > > - It is not communicated to userspace which led entry controls which key
> > > exactly
> > Sooner or later, we'll need to support these keyboards.
> >
> > But this has way too many shortcomings (and we'd be stuck with the
> > interface forever).
>
> I had some thoughts on how the current userspace api could be expanded to
> better reflect the capabilities of RGB keyboards. What would be required for
> such an expansion to be considered?
You submit a proposal.
> I'm in contact with the KDE folks. Plasma already has a keyboard brightness
> slider, that soon
> https://gitlab.freedesktop.org/upower/upower/-/merge_requests/203 will work
> with multiple kbd_backlight. However the slowness of 126 sysfs entries makes
> it a little bit janky still.
>
> They are also thinking about expanding desktop accent colors to the keyboard
> backlight when it supports RGB.
>
> I have not reached out to the OpenRGB project yet, but is it alive and well
> and under constant development: https://gitlab.com/CalcProgrammer1/OpenRGB.
> Afaik it is currently a userspace only driver interacting directly with
> hidraw mostly and has not yet implemented the sysfs leds interface.
>
> Just listing this to show that there is definitely interest in this.
Yep, there's definitely interest.
> > These days, displays with weird shapes are common. Like rounded
> > corners and holes in them. Perhaps this should be better modelled as a
> > weird display?
>
> I'm not sure if I can follow you here. Where would this be implemented? Also
> I asume displays asume equal distance between pixels and that columns are
> straight lines perpendicular to rows, which the key backlights have and are
> not.
Yes, I know displays are a bit different from RGB LEDs. Gamma is
another issue. Yes, it is quite weird display. But 6x22 display may be
better approximation of keyboard than ... 126 unrelated files.
Or you could do 6x66 sparse display, I guess, to express the
shifts. But I believe 6x22 would be better.
It would go to drivers/auxdisplay, most probably.
I checked
https://www.tuxedocomputers.com/en/Linux-Hardware/Zubehoer-USB-Co./USB-Zubehoer.tuxedo
, but you don't seem to have stand-alone keyboard with such RGB capability...?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 12.10.23 um 16:05 schrieb Pavel Machek:
> Hi!
>
>>>> There are however some known short comings:
>>>> - The sysfs leds interface does only allow to write one key at a time. The
>>>> controller however can only update row wise or the whole keyboard at once
>>>> (whole keyboard update is currently not implemented). This means that even
>>>> when you want to updated a whole row, the whole row is actually updated
>>>> once for each key. So you actually write up to 18x as much as would be
>>>> required.
>>>> - When you want to update the brightness of the whole keyboard you have to
>>>> write 126 sysfs entries, which inherently is somewhat slow, especially when
>>>> using a slider that is live updating the brightness.
>>>> - While the controller manages up to 126 leds, not all are actually
>>>> populated. However the unused are not grouped at the end but also in
>>>> between. To not have dead sysfs entries, this would require manual testing
>>>> for each implemented device to see which leds are used and some kind of
>>>> bitmap in the driver to sort out the unconnected ones.
>>>> - It is not communicated to userspace which led entry controls which key
>>>> exactly
>>> Sooner or later, we'll need to support these keyboards.
>>>
>>> But this has way too many shortcomings (and we'd be stuck with the
>>> interface forever).
>> I had some thoughts on how the current userspace api could be expanded to
>> better reflect the capabilities of RGB keyboards. What would be required for
>> such an expansion to be considered?
> You submit a proposal.
My quick writeup:
New sysfs entires:
- mode: single_zone_static, multi_zone_static, single_zone_breathing,
multi_zone_breathing, single_zone_color_cycle, multi_zone_color_cycle, etc.
- single_zone_static is mandatory, all other modes are optional (maybe even
freely definable by the driver)
- single_zone_static is the default and does not add any new sysfs entries
that aren't already present on multicolor leds aka brightness, max_brightness,
multi_index, mulit_intensity
- multi_zone_static adds a new entry zones_count. mulit_intensity has now
colors count * zones_count entries. aka a rgb keyboard with 126 leds would take
378 values for mulit_intensity
- other modes are more device specific e.g.
- multi_zone_breathing could have optional breathing_speed and
max_breathing speed entries depending on whether or not the hardware supports it.
- multi_zone_color_cycle could have a color_count and max_color_count
entry. e.g. with color_count 2 you would then write 756 values to multi
intensity, describing 2 states the driver should alternate between
Every multi_zone_* mode could also output a zones_image. That is a greyscale
bitmap or even a svg containing the information where each zone is located and
which outline it has. For the bitmap the information would be encoded in the
grey value, aka 0 = zone 0 etc with 0xff = no zone (i.e. space between the
keys). For the svg, the name of the paths would indicate the zone they are
describing. Svg would have the advantage that it could be more easily used to
also describe non square devices like mice or headsets that also might have
complex RGB controllers.
This might already be doable with triggers? I'm unsure of triggers allow to
change the length of multi_intensity however.
>
>> I'm in contact with the KDE folks. Plasma already has a keyboard brightness
>> slider, that soon
>> https://gitlab.freedesktop.org/upower/upower/-/merge_requests/203 will work
>> with multiple kbd_backlight. However the slowness of 126 sysfs entries makes
>> it a little bit janky still.
>>
>> They are also thinking about expanding desktop accent colors to the keyboard
>> backlight when it supports RGB.
>>
>> I have not reached out to the OpenRGB project yet, but is it alive and well
>> and under constant development: https://gitlab.com/CalcProgrammer1/OpenRGB.
>> Afaik it is currently a userspace only driver interacting directly with
>> hidraw mostly and has not yet implemented the sysfs leds interface.
>>
>> Just listing this to show that there is definitely interest in this.
> Yep, there's definitely interest.
>
>>> These days, displays with weird shapes are common. Like rounded
>>> corners and holes in them. Perhaps this should be better modelled as a
>>> weird display?
>> I'm not sure if I can follow you here. Where would this be implemented? Also
>> I asume displays asume equal distance between pixels and that columns are
>> straight lines perpendicular to rows, which the key backlights have and are
>> not.
> Yes, I know displays are a bit different from RGB LEDs. Gamma is
> another issue. Yes, it is quite weird display. But 6x22 display may be
> better approximation of keyboard than ... 126 unrelated files.
>
> Or you could do 6x66 sparse display, I guess, to express the
> shifts. But I believe 6x22 would be better.
>
> It would go to drivers/auxdisplay, most probably.
Looking into it, thanks for the direction. But this would come with the downside
that upowers kbd_brightness no longer controls the keyboard.
Another approach could be that i implement what i described under
multi_zone_static above without the zones_count entry. Then there wouldn't be
126 unrelated files, but a multi_intensity that is describing multiple 3 subled
leds. This would at least solve the performance problem and allow the shared
brightness adjust the hardware supports to be implemented.
>
> I checked
> https://www.tuxedocomputers.com/en/Linux-Hardware/Zubehoer-USB-Co./USB-Zubehoer.tuxedo
> , but you don't seem to have stand-alone keyboard with such RGB capability...?
No, it's for the integrated keyboards in some of our devices, this driver
specifically is for the Stellaris line with optomechanical keyboards. The ite
controller is internally connected, but is using the usb protocol.
https://www.tuxedocomputers.com/en/Linux-Hardware/Notebooks/15-16-inch/TUXEDO-Stellaris-15-Gen4.tuxedo
Kind regards,
Werner
>
> Best regards,
> Pavel
Hi!
> Every multi_zone_* mode could also output a zones_image. That is a greyscale
> bitmap or even a svg containing the information where each zone is located
> and which outline it has. For the bitmap the information would be encoded in
> the grey value, aka 0 = zone 0 etc with 0xff = no zone (i.e. space between
> the keys). For the svg, the name of the paths would indicate the
> zone they
This is not really suitable for kernel.
> > It would go to drivers/auxdisplay, most probably.
>
> Looking into it, thanks for the direction. But this would come with the
> downside that upowers kbd_brightness no longer controls the keyboard.
Yep. We could add some kind of kludge to fix that.
Perhaps first question is to ask auxdisplay people if treating
keyboard as a weird display is okay? cc: lkml, leds, drm, input at
least.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 13.10.23 um 14:19 schrieb Pavel Machek:
> Hi!
>
>> Every multi_zone_* mode could also output a zones_image. That is a greyscale
>> bitmap or even a svg containing the information where each zone is located
>> and which outline it has. For the bitmap the information would be encoded in
>> the grey value, aka 0 = zone 0 etc with 0xff = no zone (i.e. space between
>> the keys). For the svg, the name of the paths would indicate the
>> zone they
> This is not really suitable for kernel.
Yeah thought as much
>
>>> It would go to drivers/auxdisplay, most probably.
>> Looking into it, thanks for the direction. But this would come with the
>> downside that upowers kbd_brightness no longer controls the keyboard.
> Yep. We could add some kind of kludge to fix that.
>
> Perhaps first question is to ask auxdisplay people if treating
> keyboard as a weird display is okay? cc: lkml, leds, drm, input at
> least.
On it
But i don't know how to implement the different hardware effect modes then.
>
> Best regards,
> Pavel
Hi,
coming from the leds mailing list I'm writing with Pavel how to best handle
per-key RGB keyboards.
His suggestion was that it could be implemented as an aux display, but he also
suggested that I ask first if this fits.
The specific keyboard RGB controller I want to implement takes 6*21 rgb values.
However not every one is actually mapped to a physical key. e.g. the bottom row
needs less entries because of the space bar. Additionally the keys are ofc not
in a straight line from top to bottom.
Best regards,
Werner
Hi!
> coming from the leds mailing list I'm writing with Pavel how to best handle
> per-key RGB keyboards.
>
> His suggestion was that it could be implemented as an aux display, but he
> also suggested that I ask first if this fits.
Thanks for doing this.
> The specific keyboard RGB controller I want to implement takes 6*21 rgb
> values. However not every one is actually mapped to a physical key. e.g. the
> bottom row needs less entries because of the space bar. Additionally the
> keys are ofc not in a straight line from top to bottom.
So... a bit of rationale. The keyboard does not really fit into the
LED subsystem; LEDs are expected to be independent ("hdd led") and not
a matrix of them.
We do see various strange displays these days -- they commonly have
rounded corners and holes in them. I'm not sure how that's currently
supported, but I believe it is reasonable to view keyboard as a
display with slightly weird placing of pixels.
Plus, I'd really like to play tetris on one of those :-).
So, would presenting them as auxdisplay be acceptable? Or are there
better options?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> > The specific keyboard RGB controller I want to implement takes 6*21 rgb
> > values. However not every one is actually mapped to a physical key. e.g. the
> > bottom row needs less entries because of the space bar. Additionally the
> > keys are ofc not in a straight line from top to bottom.
>
> So... a bit of rationale. The keyboard does not really fit into the
> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> a matrix of them.
>
> We do see various strange displays these days -- they commonly have
> rounded corners and holes in them. I'm not sure how that's currently
> supported, but I believe it is reasonable to view keyboard as a
> display with slightly weird placing of pixels.
>
> Plus, I'd really like to play tetris on one of those :-).
>
> So, would presenting them as auxdisplay be acceptable? Or are there
> better options?
Oh and... My existing keyboard membrane-based Chicony, and it is from
time when PS/2 was still in wide use. I am slowly looking for a new
keyboard. If you know of one with nice mechanical switches, RGB
backlight with known protocol, and hopefully easily available in Czech
republic, let me know.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
On Fri, Oct 13, 2023 at 9:56 PM Pavel Machek <[email protected]> wrote:
>
> So... a bit of rationale. The keyboard does not really fit into the
> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> a matrix of them.
Makes sense.
> We do see various strange displays these days -- they commonly have
> rounded corners and holes in them. I'm not sure how that's currently
> supported, but I believe it is reasonable to view keyboard as a
> display with slightly weird placing of pixels.
>
> Plus, I'd really like to play tetris on one of those :-).
>
> So, would presenting them as auxdisplay be acceptable? Or are there
> better options?
It sounds like a fair use case -- auxdisplay are typically simple
character-based or small graphical displays, e.g. 128x64, that may not
be a "main" / usual screen as typically understood, but the concept is
a bit fuzzy and we are a bit of a catch-all.
And "keyboard backlight display with a pixel/color per-key" does not
sound like a "main" screen, and having some cute effects displayed
there are the kind of thing that one could do in the usual small
graphical ones too. :)
But if somebody prefers to create new categories (or subcategories
within auxdisplay) to hold these, that could be nice too (in the
latter case, I would perhaps suggest reorganizing all of the existing
ones while at it).
Cheers,
Miguel
Hi Werner,
kernel test robot noticed the following build errors:
[auto build test ERROR on lee-leds/for-leds-next]
[also build test ERROR on linus/master v6.6-rc6 next-20231016]
[cannot apply to pavel-leds/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Werner-Sembach/leds-rgb-Implement-per-key-keyboard-backlight-for-several-TUXEDO-devices/20231012-030206
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next
patch link: https://lore.kernel.org/r/20231011190017.1230898-1-wse%40tuxedocomputers.com
patch subject: [PATCH] leds: rgb: Implement per-key keyboard backlight for several TUXEDO devices
config: x86_64-randconfig-122-20231016 (https://download.01.org/0day-ci/archive/20231016/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231016/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
ld: vmlinux.o: in function `leds_tuxedo_ite8291_hid_feature_report_set':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:89: undefined reference to `hid_hw_raw_request'
ld: vmlinux.o: in function `leds_tuxedo_ite8291_write_row':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:130: undefined reference to `hid_hw_output_report'
ld: vmlinux.o: in function `hid_parse':
>> include/linux/hid.h:1091: undefined reference to `hid_open_report'
ld: vmlinux.o: in function `leds_tuxedo_ite8291_start_hw':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:186: undefined reference to `hid_hw_start'
>> ld: drivers/leds/rgb/leds-tuxedo-ite8291.c:194: undefined reference to `hid_hw_open'
>> ld: drivers/leds/rgb/leds-tuxedo-ite8291.c:203: undefined reference to `hid_hw_stop'
>> ld: drivers/leds/rgb/leds-tuxedo-ite8291.c:203: undefined reference to `hid_hw_stop'
ld: vmlinux.o: in function `leds_tuxedo_ite8291_stop_hw':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:210: undefined reference to `hid_hw_close'
ld: drivers/leds/rgb/leds-tuxedo-ite8291.c:212: undefined reference to `hid_hw_stop'
ld: vmlinux.o: in function `leds_tuxedo_ite8291_init':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:447: undefined reference to `__hid_register_driver'
ld: vmlinux.o: in function `leds_tuxedo_ite8291_exit':
>> drivers/leds/rgb/leds-tuxedo-ite8291.c:447: undefined reference to `hid_unregister_driver'
vim +89 drivers/leds/rgb/leds-tuxedo-ite8291.c
77
78 /**
79 * Just a generic helper function to reduce boilerplate code
80 */
81 static int leds_tuxedo_ite8291_hid_feature_report_set(struct hid_device *hdev, u8 *data, size_t len)
82 {
83 int result;
84 u8 *buf;
85
86 buf = kmemdup(data, len, GFP_KERNEL);
87 if (!buf)
88 return -ENOMEM;
> 89 result = hid_hw_raw_request(hdev, buf[0], buf, len, HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
90 kfree(buf);
91
92 return result;
93 }
94
95 /**
96 * Update brightness of the whole keyboard. Only used for initialization as this doesn't allow per
97 * key brightness control. That is implemented with RGB value scaling.
98 */
99 static int leds_tuxedo_ite8291_write_brightness(struct hid_device *hdev, u8 brightness)
100 {
101 int result;
102 u8 brightness_capped = brightness > LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_MAX ?
103 LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_MAX : brightness;
104 u8 ctrl_set_brightness[8] = {0x08, 0x02, LEDS_TUXEDO_ITE8291_PARAM_MODE_USER, 0x00,
105 brightness_capped, 0x00, 0x00, 0x00};
106
107 result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_set_brightness,
108 sizeof(ctrl_set_brightness));
109 if (result < 0)
110 return result;
111
112 return 0;
113 }
114
115 /**
116 * Update color of a singular row from row_data. This is the smallest unit this device allows to
117 * write. Changes are applied when the last row (row_index == 5) is written.
118 */
119 static int leds_tuxedo_ite8291_write_row(struct hid_device *hdev, row_data_t row_data,
120 int row_index)
121 {
122 int result;
123 u8 ctrl_announce_row_data[8] = {0x16, 0x00, row_index, 0x00, 0x00, 0x00, 0x00, 0x00};
124
125 result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_announce_row_data,
126 sizeof(ctrl_announce_row_data));
127 if (result < 0)
128 return result;
129
> 130 result = hid_hw_output_report(hdev, row_data[row_index], sizeof(row_data[row_index]));
131 if (result < 0)
132 return result;
133
134 return 0;
135 }
136
137 /**
138 * Write color and brightness to the whole keyboard from row data. Note that per key brightness is
139 * encoded in the RGB values of the row_data and the gobal brightness is a fixed value.
140 */
141 static int leds_tuxedo_ite8291_write_all(struct hid_device *hdev, row_data_t row_data)
142 {
143 int result, row_index;
144
145 if (hdev == NULL)
146 return -ENODEV;
147
148 result = leds_tuxedo_ite8291_write_brightness(hdev,
149 LEDS_TUXEDO_ITE8291_GLOBAL_BRIGHTNESS_DEFAULT);
150 if (result < 0)
151 return result;
152
153 for (row_index = 0; row_index < LEDS_TUXEDO_ITE8291_ROWS; ++row_index) {
154 result = leds_tuxedo_ite8291_write_row(hdev, row_data, row_index);
155 if (result < 0)
156 return result;
157 }
158
159 return 0;
160 }
161
162 static int leds_tuxedo_ite8291_write_state(struct hid_device *hdev)
163 {
164 struct leds_tuxedo_ite8291_driver_data_t *driver_data = hid_get_drvdata(hdev);
165
166 return leds_tuxedo_ite8291_write_all(hdev, driver_data->row_data);
167 }
168
169 static int leds_tuxedo_ite8291_write_off(struct hid_device *hdev)
170 {
171 int result;
172 u8 ctrl_write_off[8] = {0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
173
174 result = leds_tuxedo_ite8291_hid_feature_report_set(hdev, ctrl_write_off,
175 sizeof(ctrl_write_off));
176 if (result < 0)
177 return result;
178
179 return 0;
180 }
181
182 static int leds_tuxedo_ite8291_start_hw(struct hid_device *hdev)
183 {
184 int result;
185
> 186 result = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
187 if (result < 0)
188 goto err_start;
189
190 result = hid_hw_power(hdev, PM_HINT_FULLON);
191 if (result < 0)
192 goto err_power;
193
> 194 result = hid_hw_open(hdev);
195 if (result)
196 goto err_open;
197
198 return 0;
199
200 err_open:
201 hid_hw_power(hdev, PM_HINT_NORMAL);
202 err_power:
> 203 hid_hw_stop(hdev);
204 err_start:
205 return result;
206 }
207
208 static void leds_tuxedo_ite8291_stop_hw(struct hid_device *hdev)
209 {
> 210 hid_hw_close(hdev);
211 hid_hw_power(hdev, PM_HINT_NORMAL);
212 hid_hw_stop(hdev);
213 }
214
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, 16 Oct 2023, Miguel Ojeda <[email protected]> wrote:
> On Fri, Oct 13, 2023 at 9:56 PM Pavel Machek <[email protected]> wrote:
>>
>> So... a bit of rationale. The keyboard does not really fit into the
>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>> a matrix of them.
>
> Makes sense.
>
>> We do see various strange displays these days -- they commonly have
>> rounded corners and holes in them. I'm not sure how that's currently
>> supported, but I believe it is reasonable to view keyboard as a
>> display with slightly weird placing of pixels.
>>
>> Plus, I'd really like to play tetris on one of those :-).
>>
>> So, would presenting them as auxdisplay be acceptable? Or are there
>> better options?
>
> It sounds like a fair use case -- auxdisplay are typically simple
> character-based or small graphical displays, e.g. 128x64, that may not
> be a "main" / usual screen as typically understood, but the concept is
> a bit fuzzy and we are a bit of a catch-all.
>
> And "keyboard backlight display with a pixel/color per-key" does not
> sound like a "main" screen, and having some cute effects displayed
> there are the kind of thing that one could do in the usual small
> graphical ones too. :)
>
> But if somebody prefers to create new categories (or subcategories
> within auxdisplay) to hold these, that could be nice too (in the
> latter case, I would perhaps suggest reorganizing all of the existing
> ones while at it).
One could also reasonably make the argument that controlling the
individual keyboard key backlights should be part of the input
subsystem. It's not a display per se. (Unless you actually have small
displays on the keycaps, and I think that's a thing too.)
There's force feedback, there could be light feedback? There's also
drivers/input/input-leds.c for the keycaps that have leds, like caps
lock, num lock, etc.
Anyway, just throwing ideas around, no strong opinions, really.
BR,
Jani.
--
Jani Nikula, Intel
On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <[email protected]> wrote:
>
> One could also reasonably make the argument that controlling the
> individual keyboard key backlights should be part of the input
> subsystem. It's not a display per se. (Unless you actually have small
> displays on the keycaps, and I think that's a thing too.)
>
> There's force feedback, there could be light feedback? There's also
> drivers/input/input-leds.c for the keycaps that have leds, like caps
> lock, num lock, etc.
>
> Anyway, just throwing ideas around, no strong opinions, really.
Yeah, sounds quite reasonable too, in fact it may make more sense
there given the LEDs are associated per-key rather than being an
uniform matrix in a rectangle if I understand correctly. If the input
subsystem wants to take it, that would be great.
Cheers,
Miguel
Hi!
> >> So... a bit of rationale. The keyboard does not really fit into the
> >> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> >> a matrix of them.
> >
> > Makes sense.
> >
> >> We do see various strange displays these days -- they commonly have
> >> rounded corners and holes in them. I'm not sure how that's currently
> >> supported, but I believe it is reasonable to view keyboard as a
> >> display with slightly weird placing of pixels.
> >>
> >> Plus, I'd really like to play tetris on one of those :-).
> >>
> >> So, would presenting them as auxdisplay be acceptable? Or are there
> >> better options?
> >
> > It sounds like a fair use case -- auxdisplay are typically simple
> > character-based or small graphical displays, e.g. 128x64, that may not
> > be a "main" / usual screen as typically understood, but the concept is
> > a bit fuzzy and we are a bit of a catch-all.
> >
> > And "keyboard backlight display with a pixel/color per-key" does not
> > sound like a "main" screen, and having some cute effects displayed
> > there are the kind of thing that one could do in the usual small
> > graphical ones too. :)
> >
> > But if somebody prefers to create new categories (or subcategories
> > within auxdisplay) to hold these, that could be nice too (in the
> > latter case, I would perhaps suggest reorganizing all of the existing
> > ones while at it).
>
> One could also reasonably make the argument that controlling the
> individual keyboard key backlights should be part of the input
> subsystem. It's not a display per se. (Unless you actually have small
> displays on the keycaps, and I think that's a thing too.)
While it would not be completely crazy to do that... I believe the
backlight is more of a display and less of a keyboard. Plus input
subystem is very far away from supporting this, and we had no input
from input people here.
I don't think LED subsystem is right place for this, and I believe
auxdisplay makes slightly more sense than input.
Unless someone steps up, I'd suggest Werner tries to implement this as
an auxdisplay. [And yes, this will not be simple task. RGB on LED is
different from RGB on display. But there are other LED displays, so
auxdisplay should handle this. Plus pixels are really funnily
shaped. But displays with missing pixels -- aka holes for camera --
are common in phones, and I believe we'll get variable pixel densities
-- less dense over camera -- too. So displays will have to deal with
these in the end.]
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
On Mon 2023-10-23 13:44:46, Miguel Ojeda wrote:
> On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <[email protected]> wrote:
> >
> > One could also reasonably make the argument that controlling the
> > individual keyboard key backlights should be part of the input
> > subsystem. It's not a display per se. (Unless you actually have small
> > displays on the keycaps, and I think that's a thing too.)
> >
> > There's force feedback, there could be light feedback? There's also
> > drivers/input/input-leds.c for the keycaps that have leds, like caps
> > lock, num lock, etc.
> >
> > Anyway, just throwing ideas around, no strong opinions, really.
>
> Yeah, sounds quite reasonable too, in fact it may make more sense
> there given the LEDs are associated per-key rather than being an
> uniform matrix in a rectangle if I understand correctly. If the input
> subsystem wants to take it, that would be great.
Unfortunately we are getting no input from input subsystem. Question
seems to be more of "is auxdisplay willing to take it if it is done
properly"?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 20.11.23 um 21:52 schrieb Pavel Machek:
> Hi!
>
>>>> So... a bit of rationale. The keyboard does not really fit into the
>>>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>>>> a matrix of them.
>>> Makes sense.
>>>
>>>> We do see various strange displays these days -- they commonly have
>>>> rounded corners and holes in them. I'm not sure how that's currently
>>>> supported, but I believe it is reasonable to view keyboard as a
>>>> display with slightly weird placing of pixels.
>>>>
>>>> Plus, I'd really like to play tetris on one of those :-).
>>>>
>>>> So, would presenting them as auxdisplay be acceptable? Or are there
>>>> better options?
>>> It sounds like a fair use case -- auxdisplay are typically simple
>>> character-based or small graphical displays, e.g. 128x64, that may not
>>> be a "main" / usual screen as typically understood, but the concept is
>>> a bit fuzzy and we are a bit of a catch-all.
>>>
>>> And "keyboard backlight display with a pixel/color per-key" does not
>>> sound like a "main" screen, and having some cute effects displayed
>>> there are the kind of thing that one could do in the usual small
>>> graphical ones too. :)
>>>
>>> But if somebody prefers to create new categories (or subcategories
>>> within auxdisplay) to hold these, that could be nice too (in the
>>> latter case, I would perhaps suggest reorganizing all of the existing
>>> ones while at it).
>> One could also reasonably make the argument that controlling the
>> individual keyboard key backlights should be part of the input
>> subsystem. It's not a display per se. (Unless you actually have small
>> displays on the keycaps, and I think that's a thing too.)
> While it would not be completely crazy to do that... I believe the
> backlight is more of a display and less of a keyboard. Plus input
> subystem is very far away from supporting this, and we had no input
> from input people here.
>
> I don't think LED subsystem is right place for this, and I believe
> auxdisplay makes slightly more sense than input.
>
> Unless someone steps up, I'd suggest Werner tries to implement this as
> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
> different from RGB on display. But there are other LED displays, so
> auxdisplay should handle this. Plus pixels are really funnily
> shaped. But displays with missing pixels -- aka holes for camera --
> are common in phones, and I believe we'll get variable pixel densities
> -- less dense over camera -- too. So displays will have to deal with
> these in the end.]
Another idea I want to throw in the mix:
Maybe the kernel is not the right place to implement this at all. RGB stuff is
not at all standardized and every vendor is doing completely different
interfaces, which does not fit the kernel userpsace apis desire to be uniformal
and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does
not fit the snake-effect mode, or the raindrops mode, or the
4-different-colors-in-the-edges-breathing-and-color-cycling mode.
So my current idea: Implement these keyboards as a single zone RGB kbd_backlight
in the leds interface to have something functional out of the box, but make it
runtime disable-able if something like
https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular
control from userspace via hidraw.
Kind regards,
Werner
>
> Best regards,
> Pavel
Hi Werner,
On 11/21/23 12:33, Werner Sembach wrote:
> Hi,
>
> Am 20.11.23 um 21:52 schrieb Pavel Machek:
>> Hi!
>>
>>>>> So... a bit of rationale. The keyboard does not really fit into the
>>>>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>>>>> a matrix of them.
>>>> Makes sense.
>>>>
>>>>> We do see various strange displays these days -- they commonly have
>>>>> rounded corners and holes in them. I'm not sure how that's currently
>>>>> supported, but I believe it is reasonable to view keyboard as a
>>>>> display with slightly weird placing of pixels.
>>>>>
>>>>> Plus, I'd really like to play tetris on one of those :-).
>>>>>
>>>>> So, would presenting them as auxdisplay be acceptable? Or are there
>>>>> better options?
>>>> It sounds like a fair use case -- auxdisplay are typically simple
>>>> character-based or small graphical displays, e.g. 128x64, that may not
>>>> be a "main" / usual screen as typically understood, but the concept is
>>>> a bit fuzzy and we are a bit of a catch-all.
>>>>
>>>> And "keyboard backlight display with a pixel/color per-key" does not
>>>> sound like a "main" screen, and having some cute effects displayed
>>>> there are the kind of thing that one could do in the usual small
>>>> graphical ones too. :)
>>>>
>>>> But if somebody prefers to create new categories (or subcategories
>>>> within auxdisplay) to hold these, that could be nice too (in the
>>>> latter case, I would perhaps suggest reorganizing all of the existing
>>>> ones while at it).
>>> One could also reasonably make the argument that controlling the
>>> individual keyboard key backlights should be part of the input
>>> subsystem. It's not a display per se. (Unless you actually have small
>>> displays on the keycaps, and I think that's a thing too.)
>> While it would not be completely crazy to do that... I believe the
>> backlight is more of a display and less of a keyboard. Plus input
>> subystem is very far away from supporting this, and we had no input
>> from input people here.
>>
>> I don't think LED subsystem is right place for this, and I believe
>> auxdisplay makes slightly more sense than input.
>>
>> Unless someone steps up, I'd suggest Werner tries to implement this as
>> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
>> different from RGB on display. But there are other LED displays, so
>> auxdisplay should handle this. Plus pixels are really funnily
>> shaped. But displays with missing pixels -- aka holes for camera --
>> are common in phones, and I believe we'll get variable pixel densities
>> -- less dense over camera -- too. So displays will have to deal with
>> these in the end.]
>
> Another idea I want to throw in the mix:
>
> Maybe the kernel is not the right place to implement this at all. RGB stuff is not at all standardized and every vendor is doing completely different interfaces, which does not fit the kernel userpsace apis desire to be uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does not fit the snake-effect mode, or the raindrops mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>
> So my current idea: Implement these keyboards as a single zone RGB kbd_backlight in the leds interface to have something functional out of the box, but make it runtime disable-able if something like https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular control from userspace via hidraw.
That sounds like a good approach to me. We are seeing the same with game controllers where steam and wine/proton also sometimes use hidraw mode to get access to all the crazy^W interesting features.
That would mean that all we need to standardize and the kernel <-> userspace API level is adding a standard way to disable the single zone RGB kbd_backlight support in the kernel.
Regards,
Hans
Am 21.11.23 um 13:20 schrieb Hans de Goede:
> Hi Werner,
>
> On 11/21/23 12:33, Werner Sembach wrote:
>> Hi,
>>
>> Am 20.11.23 um 21:52 schrieb Pavel Machek:
>>> Hi!
>>>
>>>>>> So... a bit of rationale. The keyboard does not really fit into the
>>>>>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>>>>>> a matrix of them.
>>>>> Makes sense.
>>>>>
>>>>>> We do see various strange displays these days -- they commonly have
>>>>>> rounded corners and holes in them. I'm not sure how that's currently
>>>>>> supported, but I believe it is reasonable to view keyboard as a
>>>>>> display with slightly weird placing of pixels.
>>>>>>
>>>>>> Plus, I'd really like to play tetris on one of those :-).
>>>>>>
>>>>>> So, would presenting them as auxdisplay be acceptable? Or are there
>>>>>> better options?
>>>>> It sounds like a fair use case -- auxdisplay are typically simple
>>>>> character-based or small graphical displays, e.g. 128x64, that may not
>>>>> be a "main" / usual screen as typically understood, but the concept is
>>>>> a bit fuzzy and we are a bit of a catch-all.
>>>>>
>>>>> And "keyboard backlight display with a pixel/color per-key" does not
>>>>> sound like a "main" screen, and having some cute effects displayed
>>>>> there are the kind of thing that one could do in the usual small
>>>>> graphical ones too. :)
>>>>>
>>>>> But if somebody prefers to create new categories (or subcategories
>>>>> within auxdisplay) to hold these, that could be nice too (in the
>>>>> latter case, I would perhaps suggest reorganizing all of the existing
>>>>> ones while at it).
>>>> One could also reasonably make the argument that controlling the
>>>> individual keyboard key backlights should be part of the input
>>>> subsystem. It's not a display per se. (Unless you actually have small
>>>> displays on the keycaps, and I think that's a thing too.)
>>> While it would not be completely crazy to do that... I believe the
>>> backlight is more of a display and less of a keyboard. Plus input
>>> subystem is very far away from supporting this, and we had no input
>>> from input people here.
>>>
>>> I don't think LED subsystem is right place for this, and I believe
>>> auxdisplay makes slightly more sense than input.
>>>
>>> Unless someone steps up, I'd suggest Werner tries to implement this as
>>> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
>>> different from RGB on display. But there are other LED displays, so
>>> auxdisplay should handle this. Plus pixels are really funnily
>>> shaped. But displays with missing pixels -- aka holes for camera --
>>> are common in phones, and I believe we'll get variable pixel densities
>>> -- less dense over camera -- too. So displays will have to deal with
>>> these in the end.]
>> Another idea I want to throw in the mix:
>>
>> Maybe the kernel is not the right place to implement this at all. RGB stuff is not at all standardized and every vendor is doing completely different interfaces, which does not fit the kernel userpsace apis desire to be uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does not fit the snake-effect mode, or the raindrops mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>>
>> So my current idea: Implement these keyboards as a single zone RGB kbd_backlight in the leds interface to have something functional out of the box, but make it runtime disable-able if something like https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular control from userspace via hidraw.
> That sounds like a good approach to me. We are seeing the same with game controllers where steam and wine/proton also sometimes use hidraw mode to get access to all the crazy^W interesting features.
>
> That would mean that all we need to standardize and the kernel <-> userspace API level is adding a standard way to disable the single zone RGB kbd_backlight support in the kernel.
I would suggest a simple "enable" entry. Default is 1. When set to 0 the kernel
driver no longer does anything.
Questions:
- Should the driver try to reset the settings to boot default? Or just leave the
device in the current state? With the former I could see issues that they
keyboard is flashing when changing from kernelspace control to userspace
control. With the later the burden on bringing the device to a know state lies
with the userspace driver.
- Should this be a optional entry that only shows up on drivers supporting it,
or could this implemented in a generic way affecting all current led entries?
- I guess UPower integration for the userspace driver could be archived with
https://www.kernel.org/doc/html/latest/leds/uleds.html however this limited to
brightness atm, so when accent colors actually come to UPower this would also
need some expansion to be able to pass a preferred color to the userspace driver
(regardless of what that driver is then doing with that information).
On a different note: This approach does currently not cover the older EC
controlled 3 zone keyboards from clevo. Here only the kernel has access access
to the device so the kernel driver has to expose all functionality somehow.
Should this be done by an arbitrarily designed platform device?
Kind regards,
Werner
>
> Regards,
>
> Hans
>
>
Hi Werner,
On 11/21/23 14:29, Werner Sembach wrote:
>
> Am 21.11.23 um 13:20 schrieb Hans de Goede:
>> Hi Werner,
>>
>> On 11/21/23 12:33, Werner Sembach wrote:
>>> Hi,
>>>
>>> Am 20.11.23 um 21:52 schrieb Pavel Machek:
>>>> Hi!
>>>>
>>>>>>> So... a bit of rationale. The keyboard does not really fit into the
>>>>>>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>>>>>>> a matrix of them.
>>>>>> Makes sense.
>>>>>>
>>>>>>> We do see various strange displays these days -- they commonly have
>>>>>>> rounded corners and holes in them. I'm not sure how that's currently
>>>>>>> supported, but I believe it is reasonable to view keyboard as a
>>>>>>> display with slightly weird placing of pixels.
>>>>>>>
>>>>>>> Plus, I'd really like to play tetris on one of those :-).
>>>>>>>
>>>>>>> So, would presenting them as auxdisplay be acceptable? Or are there
>>>>>>> better options?
>>>>>> It sounds like a fair use case -- auxdisplay are typically simple
>>>>>> character-based or small graphical displays, e.g. 128x64, that may not
>>>>>> be a "main" / usual screen as typically understood, but the concept is
>>>>>> a bit fuzzy and we are a bit of a catch-all.
>>>>>>
>>>>>> And "keyboard backlight display with a pixel/color per-key" does not
>>>>>> sound like a "main" screen, and having some cute effects displayed
>>>>>> there are the kind of thing that one could do in the usual small
>>>>>> graphical ones too. :)
>>>>>>
>>>>>> But if somebody prefers to create new categories (or subcategories
>>>>>> within auxdisplay) to hold these, that could be nice too (in the
>>>>>> latter case, I would perhaps suggest reorganizing all of the existing
>>>>>> ones while at it).
>>>>> One could also reasonably make the argument that controlling the
>>>>> individual keyboard key backlights should be part of the input
>>>>> subsystem. It's not a display per se. (Unless you actually have small
>>>>> displays on the keycaps, and I think that's a thing too.)
>>>> While it would not be completely crazy to do that... I believe the
>>>> backlight is more of a display and less of a keyboard. Plus input
>>>> subystem is very far away from supporting this, and we had no input
>>>> from input people here.
>>>>
>>>> I don't think LED subsystem is right place for this, and I believe
>>>> auxdisplay makes slightly more sense than input.
>>>>
>>>> Unless someone steps up, I'd suggest Werner tries to implement this as
>>>> an auxdisplay. [And yes, this will not be simple task. RGB on LED is
>>>> different from RGB on display. But there are other LED displays, so
>>>> auxdisplay should handle this. Plus pixels are really funnily
>>>> shaped. But displays with missing pixels -- aka holes for camera --
>>>> are common in phones, and I believe we'll get variable pixel densities
>>>> -- less dense over camera -- too. So displays will have to deal with
>>>> these in the end.]
>>> Another idea I want to throw in the mix:
>>>
>>> Maybe the kernel is not the right place to implement this at all. RGB stuff is not at all standardized and every vendor is doing completely different interfaces, which does not fit the kernel userpsace apis desire to be uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does not fit the snake-effect mode, or the raindrops mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>>>
>>> So my current idea: Implement these keyboards as a single zone RGB kbd_backlight in the leds interface to have something functional out of the box, but make it runtime disable-able if something like https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular control from userspace via hidraw.
>> That sounds like a good approach to me. We are seeing the same with game controllers where steam and wine/proton also sometimes use hidraw mode to get access to all the crazy^W interesting features.
>>
>> That would mean that all we need to standardize and the kernel <-> userspace API level is adding a standard way to disable the single zone RGB kbd_backlight support in the kernel.
>
> I would suggest a simple "enable" entry. Default is 1. When set to 0 the kernel driver no longer does anything.
I'm not in favor of using "enable" as sysfs attribute for this,
I would like to see a more descriptive name, how about:
"disable_kernel_kbd_backlight_support"
And then maybe also have the driver actually unregister
the LED class device ?
Or just make the support inactive when writing 1 to
this and allow re-enabling it by writing 0?
> Questions:
>
> - Should the driver try to reset the settings to boot default? Or just leave the device in the current state? With the former I could see issues that they keyboard is flashing when changing from kernelspace control to userspace control. With the later the burden on bringing the device to a know state lies with the userspace driver.
My vote would go to leave the state as is. Even if the hw
does not support state readback, then the userspace code
can readback the state before writing 1 to
"disable_kernel_kbd_backlight_support"
> - Should this be a optional entry that only shows up on drivers supporting it, or could this implemented in a generic way affecting all current led entries?
IMHO this should be optional. If we go with the variant
where writing 1 to "disable_kernel_kbd_backlight_support"
just disables support and 0 re-enables it then I guess
we could have support for this in the LED-core, enabled
by a flag set by the driver.
If we go with unregistering the led class device,
then this needs to be mostly handled in the driver.
Either way the kernel driver should know about this even
if it is mostly handled in the LED core so that e.g.
it does not try to restore settings on resume from suspend.
> - I guess UPower integration for the userspace driver could be archived with https://www.kernel.org/doc/html/latest/leds/uleds.html however this limited to brightness atm, so when accent colors actually come to UPower this would also need some expansion to be able to pass a preferred color to the userspace driver (regardless of what that driver is then doing with that information).
Using uleds is an interesting suggestion, but upower atm
does not support LED class kbd_backlight devices getting
hot-plugged. It only scans for them once at boot.
Jelle van der Waa (a colleague of mine, added to the Cc)
has indicated he is interested in maybe working on fixing
this upower short-coming as a side project, once his
current side-projects are finished.
> On a different note: This approach does currently not cover the older EC controlled 3 zone keyboards from clevo. Here only the kernel has access access to the device so the kernel driver has to expose all functionality somehow. Should this be done by an arbitrarily designed platform device?
Interesting question, this reminds there was a discussion
about how to handle zoned keyboards using plain LED class
APIs here:
https://lore.kernel.org/linux-leds/[email protected]/
Basically the idea discussed there is to create
separate multi-color LED sysfs devices for each zone,
using :rgb:kbd_zoned_backlight-xxx as postfix, e.g. :
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-wasd
As postfixes for the 4 per zone LED class devices
and then teach upower to just treat this as
a single kbd-backlight for the existing upower
DBUS API and maybe later extend the DBUS API.
Would something like this work for the Clevo
case you are describing?
Unfortunately this was never implemented but
I think that for simple zoned backlighting
this still makes sense. Where as for per key
controllable backlighting as mention in
$subject I do believe that just using hidraw
access directly from userspace is best.
Regards,
Hans
Hi Hans,
Am 22.11.23 um 19:34 schrieb Hans de Goede:
> Hi Werner,
[snip]
>>>> Another idea I want to throw in the mix:
>>>>
>>>> Maybe the kernel is not the right place to implement this at all. RGB stuff is not at all standardized and every vendor is doing completely different interfaces, which does not fit the kernel userpsace apis desire to be uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does not fit the snake-effect mode, or the raindrops mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>>>>
>>>> So my current idea: Implement these keyboards as a single zone RGB kbd_backlight in the leds interface to have something functional out of the box, but make it runtime disable-able if something like https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular control from userspace via hidraw.
>>> That sounds like a good approach to me. We are seeing the same with game controllers where steam and wine/proton also sometimes use hidraw mode to get access to all the crazy^W interesting features.
>>>
>>> That would mean that all we need to standardize and the kernel <-> userspace API level is adding a standard way to disable the single zone RGB kbd_backlight support in the kernel.
>> I would suggest a simple "enable" entry. Default is 1. When set to 0 the kernel driver no longer does anything.
> I'm not in favor of using "enable" as sysfs attribute for this,
> I would like to see a more descriptive name, how about:
>
> "disable_kernel_kbd_backlight_support"
>
> And then maybe also have the driver actually unregister
> the LED class device ?
>
> Or just make the support inactive when writing 1 to
> this and allow re-enabling it by writing 0?
Unregistering would mean that it can't be reenabled without module reload/reboot?
I would prefer that the userspace driver could easily give back control to the
leds interface.
>
>> Questions:
>>
>> - Should the driver try to reset the settings to boot default? Or just leave the device in the current state? With the former I could see issues that they keyboard is flashing when changing from kernelspace control to userspace control. With the later the burden on bringing the device to a know state lies with the userspace driver.
> My vote would go to leave the state as is. Even if the hw
> does not support state readback, then the userspace code
> can readback the state before writing 1 to
> "disable_kernel_kbd_backlight_support"
ack
>
>> - Should this be a optional entry that only shows up on drivers supporting it, or could this implemented in a generic way affecting all current led entries?
> IMHO this should be optional. If we go with the variant
> where writing 1 to "disable_kernel_kbd_backlight_support"
> just disables support and 0 re-enables it then I guess
> we could have support for this in the LED-core, enabled
> by a flag set by the driver.
>
> If we go with unregistering the led class device,
> then this needs to be mostly handled in the driver.
>
> Either way the kernel driver should know about this even
> if it is mostly handled in the LED core so that e.g.
> it does not try to restore settings on resume from suspend.
So a generic implementation would still require all current led drivers to be
touched?
For the sake of simplicity I would then prefer the optional variant.
>
>> - I guess UPower integration for the userspace driver could be archived with https://www.kernel.org/doc/html/latest/leds/uleds.html however this limited to brightness atm, so when accent colors actually come to UPower this would also need some expansion to be able to pass a preferred color to the userspace driver (regardless of what that driver is then doing with that information).
> Using uleds is an interesting suggestion, but upower atm
> does not support LED class kbd_backlight devices getting
> hot-plugged. It only scans for them once at boot.
>
> Jelle van der Waa (a colleague of mine, added to the Cc)
> has indicated he is interested in maybe working on fixing
> this upower short-coming as a side project, once his
> current side-projects are finished.
Nice to hear.
>
>> On a different note: This approach does currently not cover the older EC controlled 3 zone keyboards from clevo. Here only the kernel has access access to the device so the kernel driver has to expose all functionality somehow. Should this be done by an arbitrarily designed platform device?
> Interesting question, this reminds there was a discussion
> about how to handle zoned keyboards using plain LED class
> APIs here:
>
> https://lore.kernel.org/linux-leds/[email protected]/
>
> Basically the idea discussed there is to create
> separate multi-color LED sysfs devices for each zone,
> using :rgb:kbd_zoned_backlight-xxx as postfix, e.g. :
>
> :rgb:kbd_zoned_backlight-left
> :rgb:kbd_zoned_backlight-middle
> :rgb:kbd_zoned_backlight-right
> :rgb:kbd_zoned_backlight-wasd
>
> As postfixes for the 4 per zone LED class devices
> and then teach upower to just treat this as
> a single kbd-backlight for the existing upower
> DBUS API and maybe later extend the DBUS API.
>
> Would something like this work for the Clevo
> case you are describing?
Not entirely as some concept for the special modes would still be required.
Also it would be nice to be able to set the whole keyboard with a singular file
access so that the keyboard changes at once and not zone by zone.
>
> Unfortunately this was never implemented but
> I think that for simple zoned backlighting
> this still makes sense. Where as for per key
> controllable backlighting as mention in
> $subject I do believe that just using hidraw
> access directly from userspace is best.
>
> Regards,
>
> Hans
I also stumbled across a new Problem:
We have an upcoming device that has a per-key keyboard backlight, but does the
control completely via a wmi/acpi interface. So no usable hidraw here for a
potential userspace driver implementation ...
So a quick summary for the ideas floating in this thread so far:
1. Expand leds interface allowing arbitrary modes with semi arbitrary optional
attributes:
- Pro:
- Still offers all default attributes for use with UPower
- Fairly simple to implement from the preexisting codebase
- Could be implemented for all (to me) known internal keyboard backlights
- Con:
- Violates the simplicity paradigm of the leds interface (e.g. with
this one leds entry controls possible multiple leds)
2. Implement per-key keyboards as auxdisplay
- Pro:
- Already has a concept for led positions
- Is conceptually closer to "multiple leds forming a singular entity"
- Con:
- No preexisting UPower support
- No concept for special hardware lightning modes
- No support for arbitrary led outlines yet (e.g. ISO style enter-key)
3. Implement in input subsystem
- Pro:
- Preexisting concept for keys and key purpose
- Con:
- Not in scope for subsystem
- No other preexisting light infrastructure
4. Implement a simple leds driver only supporting a small subset of the
capabilities and make it disable-able for a userspace driver to take over
- Pro:
- Most simple to implement basic support
- In scope for led subsystem simplicity paradigm
- Con:
- Not all built in keyboard backlights can be implemented in a
userspace only driver
Kind Regards,
Werner
Hi Hans & the others,
[snip]
> I also stumbled across a new Problem:
>
> We have an upcoming device that has a per-key keyboard backlight, but does the
> control completely via a wmi/acpi interface. So no usable hidraw here for a
> potential userspace driver implementation ...
>
> So a quick summary for the ideas floating in this thread so far:
>
> 1. Expand leds interface allowing arbitrary modes with semi arbitrary optional
> attributes:
>
> - Pro:
>
> - Still offers all default attributes for use with UPower
>
> - Fairly simple to implement from the preexisting codebase
>
> - Could be implemented for all (to me) known internal keyboard backlights
>
> - Con:
>
> - Violates the simplicity paradigm of the leds interface (e.g. with
> this one leds entry controls possible multiple leds)
>
> 2. Implement per-key keyboards as auxdisplay
>
> - Pro:
>
> - Already has a concept for led positions
>
> - Is conceptually closer to "multiple leds forming a singular entity"
>
> - Con:
>
> - No preexisting UPower support
>
> - No concept for special hardware lighting modes
>
> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>
> 3. Implement in input subsystem
>
> - Pro:
>
> - Preexisting concept for keys and key purpose
>
> - Con:
>
> - Not in scope for subsystem
>
> - No other preexisting light infrastructure
>
> 4. Implement a simple leds driver only supporting a small subset of the
> capabilities and make it disable-able for a userspace driver to take over
>
> - Pro:
>
> - Most simple to implement basic support
>
> - In scope for led subsystem simplicity paradigm
>
> - Con:
>
> - Not all built in keyboard backlights can be implemented in a
> userspace only driver
>
> Kind Regards,
>
> Werner
Just a gentle bump and request for comments again. 4. would be better then
nothing but it is not a universal future proof solution so I'm hesitant to put
work into it even though it would be the simplest driver. I still tend towards
1. as the leds interface already got expanded once with the multicolor stuff.
The only other way I see would be to implement a platform driver with no
standardized api or implement a complete new api/subsystem from the ground up.
Kind Regards,
Werner
Hi Werner,
Once again, sorry for the very slow response here.
On 11/27/23 11:59, Werner Sembach wrote:
> Hi Hans,
>
> Am 22.11.23 um 19:34 schrieb Hans de Goede:
>> Hi Werner,
> [snip]
>>>>> Another idea I want to throw in the mix:
>>>>>
>>>>> Maybe the kernel is not the right place to implement this at all. RGB stuff is not at all standardized and every vendor is doing completely different interfaces, which does not fit the kernel userpsace apis desire to be uniformal and fixed. e.g. Auxdisplay might fit static setting of RGB values, but it does not fit the snake-effect mode, or the raindrops mode, or the 4-different-colors-in-the-edges-breathing-and-color-cycling mode.
>>>>>
>>>>> So my current idea: Implement these keyboards as a single zone RGB kbd_backlight in the leds interface to have something functional out of the box, but make it runtime disable-able if something like https://gitlab.com/CalcProgrammer1/OpenRGB wants to take over more fine granular control from userspace via hidraw.
>>>> That sounds like a good approach to me. We are seeing the same with game controllers where steam and wine/proton also sometimes use hidraw mode to get access to all the crazy^W interesting features.
>>>>
>>>> That would mean that all we need to standardize and the kernel <-> userspace API level is adding a standard way to disable the single zone RGB kbd_backlight support in the kernel.
>>> I would suggest a simple "enable" entry. Default is 1. When set to 0 the kernel driver no longer does anything.
>> I'm not in favor of using "enable" as sysfs attribute for this,
>> I would like to see a more descriptive name, how about:
>>
>> "disable_kernel_kbd_backlight_support"
>>
>> And then maybe also have the driver actually unregister
>> the LED class device ?
>>
>> Or just make the support inactive when writing 1 to
>> this and allow re-enabling it by writing 0?
>
> Unregistering would mean that it can't be reenabled without module reload/reboot?
Yes.
> I would prefer that the userspace driver could easily give back control to the leds interface.
Hmm, the problem here is that leaving a non-working LED class device
behind may confuse things like upower.
So maybe the disable_kbd_backlight_support sysfs attr should
not sit under /sys/class/leds/foobar::kbd_backlight, but rather
it should sit under the sysfs dir of the parent-device ?
So if we are talking [USB|I2C]-HID keyboards and userspace
using hidraw to takeover kbd_backlight control through,
then have "disable_kbd_backlight_support" sit under
/sys/bus/hid/devices/0003:xxxx:xxxx.xxxx/disable_kbd_backlight_support
and then re-register the LED class device for the keyboard
when 0 gets written to disable_kbd_backlight_support ?
That seems better to me then leaving a non-working LED
class device behind and this will not require any changes
to the LED subsystem.
>>> Questions:
>>>
>>> - Should the driver try to reset the settings to boot default? Or just leave the device in the current state? With the former I could see issues that they keyboard is flashing when changing from kernelspace control to userspace control. With the later the burden on bringing the device to a know state lies with the userspace driver.
>> My vote would go to leave the state as is. Even if the hw
>> does not support state readback, then the userspace code
>> can readback the state before writing 1 to
>> "disable_kernel_kbd_backlight_support"
> ack
>>
>>> - Should this be a optional entry that only shows up on drivers supporting it, or could this implemented in a generic way affecting all current led entries?
>> IMHO this should be optional. If we go with the variant
>> where writing 1 to "disable_kernel_kbd_backlight_support"
>> just disables support and 0 re-enables it then I guess
>> we could have support for this in the LED-core, enabled
>> by a flag set by the driver.
>>
>> If we go with unregistering the led class device,
>> then this needs to be mostly handled in the driver.
>>
>> Either way the kernel driver should know about this even
>> if it is mostly handled in the LED core so that e.g.
>> it does not try to restore settings on resume from suspend.
>
> So a generic implementation would still require all current led drivers to be touched?
>
> For the sake of simplicity I would then prefer the optional variant.
See above, I think we need to put the sysfs-attr to disable
the kernel's builtin kbd-backlight support in the parents
sysfs-dir and then actually unregister the LED class device,
this way we don't need any LED subsytem changes at all.
>>> - I guess UPower integration for the userspace driver could be archived with https://www.kernel.org/doc/html/latest/leds/uleds.html however this limited to brightness atm, so when accent colors actually come to UPower this would also need some expansion to be able to pass a preferred color to the userspace driver (regardless of what that driver is then doing with that information).
>> Using uleds is an interesting suggestion, but upower atm
>> does not support LED class kbd_backlight devices getting
>> hot-plugged. It only scans for them once at boot.
>>
>> Jelle van der Waa (a colleague of mine, added to the Cc)
>> has indicated he is interested in maybe working on fixing
>> this upower short-coming as a side project, once his
>> current side-projects are finished.
> Nice to hear.
>>
>>> On a different note: This approach does currently not cover the older EC controlled 3 zone keyboards from clevo. Here only the kernel has access access to the device so the kernel driver has to expose all functionality somehow. Should this be done by an arbitrarily designed platform device?
>> Interesting question, this reminds there was a discussion
>> about how to handle zoned keyboards using plain LED class
>> APIs here:
>>
>> https://lore.kernel.org/linux-leds/[email protected]/
>>
>> Basically the idea discussed there is to create
>> separate multi-color LED sysfs devices for each zone,
>> using :rgb:kbd_zoned_backlight-xxx as postfix, e.g. :
>>
>> :rgb:kbd_zoned_backlight-left
>> :rgb:kbd_zoned_backlight-middle
>> :rgb:kbd_zoned_backlight-right
>> :rgb:kbd_zoned_backlight-wasd
>>
>> As postfixes for the 4 per zone LED class devices
>> and then teach upower to just treat this as
>> a single kbd-backlight for the existing upower
>> DBUS API and maybe later extend the DBUS API.
>>
>> Would something like this work for the Clevo
>> case you are describing?
>
> Not entirely as some concept for the special modes would still be required.
Right, that can be done with some custom sysfs attr added
to the LED class device, like how dell-laptop.c sets
the .groups member of the "dell::kbd_backlight"
"struct led_classdev kbd_led" to add some extra
sysfs_attr to configure the timeout after which
the kbd_backlight automatically turns off when
no keys are pressed.
> Also it would be nice to be able to set the whole keyboard with a singular file access so that the keyboard changes at once and not zone by zone.
That is an interesting point. This could be implemented
by adding an "enable_atomic_commit" sysfs attr to
all 4:
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-wasd
LED class devices, which is backed by only
1 variable in the kernel (so changing it
in one place changes it everywhere) and
then also have a "commit" sysfs attr and
writing say "1" to that will then commit
all changes at once.
So normally changes are still applied directly
(for compatibility with the usual sysfs API),
but then when "enable_atomic_commit" is set to 1,
writes only update in kernel variables and then
once "commit" is written all changes are send
out in 1 go.
I think we had the same issue where there was
a single WMI call to change all zones at once
(and having some sort of atomic API was desirable)
the last time the suggestion to use 4 LED class
devices for zoned kbds:
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-wasd
came up, so we could start a new:
Documentation/ABI/testing/sysfs-class-led-zoned-kbd-backlight
document extending the standard:
Documentation/ABI/testing/sysfs-class-led
which documents both using the:
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-wasd
suffixes there, as well as document some sort
of atomically change all 4 zones at once API.
Werner, if this sounds like something which would
work for you, then it would probably be best to
first submit a RFC patch introducing a:
Documentation/ABI/testing/sysfs-class-led-zoned-kbd-backlight
and then first discuss that with the LED subsys
maintainers, so that we have buy-in from the LED
subsys maintainers before you start actually
implementing this.
I'll reply to your "I also stumbled across a new Problem"
in another reply as it seems best to start a separate
thread for this.
Regards,
Hans
Hi All,
On 11/27/23 11:59, Werner Sembach wrote:
<snip>
> I also stumbled across a new Problem:
>
> We have an upcoming device that has a per-key keyboard backlight, but does the control completely via a wmi/acpi interface. So no usable hidraw here for a potential userspace driver implementation ...
>
> So a quick summary for the ideas floating in this thread so far:
>
> 1. Expand leds interface allowing arbitrary modes with semi arbitrary optional attributes:
>
> - Pro:
>
> - Still offers all default attributes for use with UPower
>
> - Fairly simple to implement from the preexisting codebase
>
> - Could be implemented for all (to me) known internal keyboard backlights
>
> - Con:
>
> - Violates the simplicity paradigm of the leds interface (e.g. with this one leds entry controls possible multiple leds)
So what you are suggesting here is having some way (a-z + other sysfs attr?)
to use a single LED class device and then extend that to allow setting all
keys ?
This does not seem like a good idea to me and this will also cause issues
when doing animations in software, since this API will likely be slow.
And if the API is not slow, then it will likely involve some sort
of binary sysfs file for setting multiple keys rather then 1
file per key which would break the normal 1 file per setting sysfs
paradigm.
> 2. Implement per-key keyboards as auxdisplay
>
> - Pro:
>
> - Already has a concept for led positions
With a "concept" you mean simple x,y positioning or is
there something more advanced here that I'm aware of ?
> - Is conceptually closer to "multiple leds forming a singular entity"
>
> - Con:
>
> - No preexisting UPower support
>
> - No concept for special hardware lightning modes
>
> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
Hmm, so there is very little documentation on this and what
docs there is: Documentation/admin-guide/auxdisplay/cfag12864b.rst
as well as the example program how to uses this suggests that
this is using the old /dev/fb# interface which we are sorta
trying to retire.
> 3. Implement in input subsystem
>
> - Pro:
>
> - Preexisting concept for keys and key purpose
>
> - Con:
>
> - Not in scope for subsystem
>
> - No other preexisting light infrastructure
Dmitry actually recently nacked the addition of
a LED_MIC_MUTE define to include/uapi/linux/input-event-codes.h
which was intended to be able to allow the input LED support
with standard HID mic-mute leds (spk-mute is already supported
this way).
Dmitry was very clear that no new LEDs must be added and
that any new LED support should be done through the LED
subsytem, so I do not think that something like this
is going to fly.
> 4. Implement a simple leds driver only supporting a small subset of the capabilities and make it disable-able for a userspace driver to take over
>
> - Pro:
>
> - Most simple to implement basic support
>
> - In scope for led subsystem simplicity paradigm
>
> - Con:
>
> - Not all built in keyboard backlights can be implemented in a userspace only driver
Right, so this is basically what we have been discussing in the other
part of the thread with the:
/sys/bus/hid/devices/0003:xxxx:xxxx.xxxx/disable_kbd_backlight_support
proposal to unregister the kernel's LED class device and then
allow userspace to do whatever it wants through /dev/hidraw
without the kernel also trying to access the backlight
functionality at the same time.
AFAIK there already is a bunch of userspace support for
per key addressable kbd RGB backlights using hidraw support,
so this way we can use the momentum / code of these existing
projects, at least for existing hidraw keyboards and adding
support for:
/sys/bus/hid/devices/0003:xxxx:xxxx.xxxx/disable_kbd_backlight_support
to these existing projects should be simple.
Yet this will not work for your mentioned "control completely
via a wmi/acpi interface". Still I think we should go the same
route for those adding a misc-char device or something like
that to allow making WMI calls from userspace (like Windows
can do). Maybe with an allow list per GUID to only allow
specific calls, so that we can avoid possible dangerous calls.
Armin Wolf recently became the WMI bus maintainer.
Armin, we are discussing how to deal with (laptop) keyboards
which have a separate RGB LED per key and how to control
those LEDs.
So far per key addressable keyboard backlighting has always
been HID based, so any existing support is just userspace
based using /dev/hidraw. In my experience the problem with
supporting gaming peripherals is that there is interest in it,
but not really enough interest to keep a sustained momentum
behind projects, especially not when it comes to taking code
from a fun weekend hack to upstreaming them into bigger
projects like the kernel.
So I would like to offer some sort of easy accessible
API to userspace for accessing this, basically allowing
userspace drivers for the LED part of the keyboard which
in some cases will involve making WMI calls from userspace.
So, Armin, what do you think about a way of allowing
(filtered) WMI calls from userspace through say
a misc-char device + ioctls or something like that?
Werner atm I personally do think that option 4. from
your list is the way to go. Mainly because designing
a generic kernel API for all bells and whistles of gaming
hw is very tricky and would require a large ongoing
effort which I just don't see happening (based on
past experience).
Regards,
Hans
Hi!
> We have an upcoming device that has a per-key keyboard backlight, but does
> the control completely via a wmi/acpi interface. So no usable hidraw here
> for a potential userspace driver implementation ...
>
> So a quick summary for the ideas floating in this thread so far:
>
> 1. Expand leds interface allowing arbitrary modes with semi arbitrary
> optional attributes:
> ??? - Con:
>
> ??? ??? - Violates the simplicity paradigm of the leds interface (e.g. with
> this one leds entry controls possible multiple leds)
Let's not do this.
> 2. Implement per-key keyboards as auxdisplay
>
> ??? - Pro:
>
> ??? ??? - Already has a concept for led positions
>
> ??? ??? - Is conceptually closer to "multiple leds forming a singular entity"
>
> ??? - Con:
>
> ??? ??? - No preexisting UPower support
>
> ??? ??? - No concept for special hardware lightning modes
>
> ??? ??? - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
Please do this one.
> 3. Implement in input subsystem
>
> ??? - Pro:
>
> ??? ??? - Preexisting concept for keys and key purpose
>
> ??? - Con:
>
> ??? ??? - Not in scope for subsystem
>
> ??? ??? - No other preexisting light infrastructure
Or negotiate with input people to do this.
> 4. Implement a simple leds driver only supporting a small subset of the
> capabilities and make it disable-able for a userspace driver to take over
No. Kernel should abstract this away.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi
Am 18.01.24 um 18:45 schrieb Pavel Machek:
> Hi!
>
>> We have an upcoming device that has a per-key keyboard backlight, but does
>> the control completely via a wmi/acpi interface. So no usable hidraw here
>> for a potential userspace driver implementation ...
>>
>> So a quick summary for the ideas floating in this thread so far:
>>
>> 1. Expand leds interface allowing arbitrary modes with semi arbitrary
>> optional attributes:
>> - Con:
>>
>> - Violates the simplicity paradigm of the leds interface (e.g. with
>> this one leds entry controls possible multiple leds)
> Let's not do this.
>
>> 2. Implement per-key keyboards as auxdisplay
>>
>> - Pro:
>>
>> - Already has a concept for led positions
>>
>> - Is conceptually closer to "multiple leds forming a singular entity"
>>
>> - Con:
>>
>> - No preexisting UPower support
>>
>> - No concept for special hardware lightning modes
>>
>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
> Please do this one.
2 more maybe cons to add to this that popped into my mind just now:
- How would lightbars, or mice fit into this?
- How do 3-zone, 4-zone, n-zone keyboards fit into this? aka how many zones to
be considered an aux display?
>
>> 3. Implement in input subsystem
>>
>> - Pro:
>>
>> - Preexisting concept for keys and key purpose
>>
>> - Con:
>>
>> - Not in scope for subsystem
>>
>> - No other preexisting light infrastructure
> Or negotiate with input people to do this.
>
>> 4. Implement a simple leds driver only supporting a small subset of the
>> capabilities and make it disable-able for a userspace driver to take over
> No. Kernel should abstract this away.
>
> Best regards,
> Pavel
Kind regards,
Werner
Hi,
On 1/18/24 18:45, Pavel Machek wrote:
> Hi!
>
>> We have an upcoming device that has a per-key keyboard backlight, but does
>> the control completely via a wmi/acpi interface. So no usable hidraw here
>> for a potential userspace driver implementation ...
>>
>> So a quick summary for the ideas floating in this thread so far:
>>
>> 1. Expand leds interface allowing arbitrary modes with semi arbitrary
>> optional attributes:
>
>> - Con:
>>
>> - Violates the simplicity paradigm of the leds interface (e.g. with
>> this one leds entry controls possible multiple leds)
>
> Let's not do this.
>
>> 2. Implement per-key keyboards as auxdisplay
>>
>> - Pro:
>>
>> - Already has a concept for led positions
>>
>> - Is conceptually closer to "multiple leds forming a singular entity"
>>
>> - Con:
>>
>> - No preexisting UPower support
>>
>> - No concept for special hardware lightning modes
>>
>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>
> Please do this one.
Ok, so based on the discussion so far and Pavel's feedback lets try to
design a custom userspace API for this. I do not believe that auxdisplay
is a good fit because:
- auxdisplay is just a directory name, it does not seem to clearly
define an API
- instead the deprecated /dev/fb API is used which is deprecated
- auxdisplays are very much displays (hence /dev/fb) they are typically
small LCD displays with a straight widthxheight grid of square pixels
- /dev/fb does gives us nothing for effects, zoned keyboard, etc.
So my proposal would be an ioctl interface (ioctl only no r/w)
using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
For per key controllable rgb LEDs we need to discuss a coordinate
system. I propose using a fixed size of 16 rows of 64 keys,
so 64x16 in standard WxH notation.
And then storing RGB in separate bytes, so userspace will then
always send a buffer of 192 bytes per line (64x3) x 14 rows
= 3072 bytes. With the kernel driver ignoring parts of
the buffer where there are no actual keys.
I would then like the map the standard 105 key layout onto this,
starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
the top left). Leaving plenty of space on the left top and right
(and some on the bottom) for extra media key rows, macro keys, etc.
The idea to have the standard layout at a fixed place is to allow
userspace to have a database of preset patterns which will work
everywhere.
Note I say standard 105 key layout, but in reality for
defining the standardized part of the buffer we should
use the maximum amount of keys per row of all the standard layouts,
so for row 6 (the ESC row) and for extra keys on the right outside
the main block we use the standard layout as shown here:
http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
For the main area of the keyboard looking at:
http://bopqehorizon.weebly.com/uploads/1/3/4/3/134337299/913246919_orig.png
We want to max rows per key, so this means that per row we use
(from the above image) :
row 7: 106/109 - JIS
row 8: 101/104 - ANSI
row 9: 102/105 - ISO
row 10: 104/107 - ABNT
row 11: 106/109 - JIS
(with row 7 being the main area top row)
This way we can address all the possible keys in the various
standard layouts in one standard wat and then the drivers can
just skip keys which are not there when preparing the buffer
to send to the hw / fw.
One open question is if we should add padding after the main
area so that the printscreen / ins / del / leftarrow of the
"middle" block of
http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
all start at the same x (say 32) or we just pack these directly
after the main area.
And the same question for the numlock block, do we align
this to an x of say 36, or pack it ?
As for the actual IOCTL API I think there should be
the following ioctls:
1. A get-info ioctl returning a struct with the following members:
{
char name[64] /* Keyboard model name / identifier */
int row_begin[16]; /* The x address of the first available key per row. On a std 105key kbd this will be 16 for rows 6-11, 0 for other rows */
int row_end[16]; /* x+1 for the address of the last available key per row, end - begin gives number of keys in a row */
int rgb_zones; /* number of rgb zones for zoned keyboards. Note both
zones and per key addressing may be available if
effects are applied per zone. */
?
}
2. A set-leds ioctl which takes the earlier discussed 3092 bytes buffer
to set all the LEDs at once, only valid if at least one row has a non 0 lenght.
3. A set-zones ioctl which takes an array of bytes sized 3 * number-of-zones
containing RGB values for each zone
4. A enum_effects ioctl which takes a struct with the following members:
{
long size; /* Size of passed in struct including the size member itself */
long effects_mask[]
}
the idea being that there is an enum with effects, which gets extended
as we encounter more effects and the bitmask in effects_mask has a bit set
for each effects enum value which is supported. effects_mask is an array
so that we don't run out of bits. If older userspace only passes 1 long
(size == (2*sizeof(long)) when 2 are needed at some point in the future
then the kernel will simply only fill the first long.
5. A set_effect ioctl which takes a struct with the following members:
{
long size; /* Size of passed in struct including the size member itself */
int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
int zone; /* zone to apply the effect to */
int speed; /* cycle speed of the effect in milli-hz */
char color1[3]; /* effect dependend may be unused. */
char color2[3]; /* effect dependend may be unused. */
}
Again the idea with the size member is that the struct can be extended with
new members if necessary and the kernel will supply a default value for
older userspaces which provide a smaller struct (note size being smaller
then sizeof(struct-v1) will invalid).
Note this is all just a rough sketch suggestions welcome!
Regards,
Hans
On Fri, 19 Jan 2024, Hans de Goede <[email protected]> wrote:
> For per key controllable rgb LEDs we need to discuss a coordinate
> system. I propose using a fixed size of 16 rows of 64 keys,
> so 64x16 in standard WxH notation.
>
> And then storing RGB in separate bytes, so userspace will then
> always send a buffer of 192 bytes per line (64x3) x 14 rows
> = 3072 bytes. With the kernel driver ignoring parts of
> the buffer where there are no actual keys.
>
> I would then like the map the standard 105 key layout onto this,
> starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
> the top left). Leaving plenty of space on the left top and right
> (and some on the bottom) for extra media key rows, macro keys, etc.
>
> The idea to have the standard layout at a fixed place is to allow
> userspace to have a database of preset patterns which will work
> everywhere.
>
> Note I say standard 105 key layout, but in reality for
> defining the standardized part of the buffer we should
> use the maximum amount of keys per row of all the standard layouts,
> so for row 6 (the ESC row) and for extra keys on the right outside
> the main block we use the standard layout as shown here:
Doesn't the input stack already have to have pretty much all of this
already covered? I can view the keyboard layout in my desktop
environment, and it's a reasonably accurate match, even if unlikely to
be pixel perfect. But crucially, it has to have all the possible layouts
covered already.
And while I would personally hate it, you can imagine a use case where
you'd like a keypress to have a visual effect around the key you
pressed. A kind of force feedback, if you will. I don't actually know,
and correct me if I'm wrong, but feels like implementing that outside of
the input subsystem would be non-trivial.
Cc: Dmitry, could we at least have some input from the input subsystem
POV on this? AFAICT we have received none.
BR,
Jani.
>
> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>
> For the main area of the keyboard looking at:
>
> http://bopqehorizon.weebly.com/uploads/1/3/4/3/134337299/913246919_orig.png
>
> We want to max rows per key, so this means that per row we use
> (from the above image) :
>
> row 7: 106/109 - JIS
> row 8: 101/104 - ANSI
> row 9: 102/105 - ISO
> row 10: 104/107 - ABNT
> row 11: 106/109 - JIS
>
> (with row 7 being the main area top row)
>
> This way we can address all the possible keys in the various
> standard layouts in one standard wat and then the drivers can
> just skip keys which are not there when preparing the buffer
> to send to the hw / fw.
>
> One open question is if we should add padding after the main
> area so that the printscreen / ins / del / leftarrow of the
> "middle" block of
>
> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>
> all start at the same x (say 32) or we just pack these directly
> after the main area.
>
> And the same question for the numlock block, do we align
> this to an x of say 36, or pack it ?
>
>
> As for the actual IOCTL API I think there should be
> the following ioctls:
>
> 1. A get-info ioctl returning a struct with the following members:
>
> {
> char name[64] /* Keyboard model name / identifier */
> int row_begin[16]; /* The x address of the first available key per row. On a std 105key kbd this will be 16 for rows 6-11, 0 for other rows */
> int row_end[16]; /* x+1 for the address of the last available key per row, end - begin gives number of keys in a row */
> int rgb_zones; /* number of rgb zones for zoned keyboards. Note both
> zones and per key addressing may be available if
> effects are applied per zone. */
> ?
> }
>
> 2. A set-leds ioctl which takes the earlier discussed 3092 bytes buffer
> to set all the LEDs at once, only valid if at least one row has a non 0 lenght.
>
> 3. A set-zones ioctl which takes an array of bytes sized 3 * number-of-zones
> containing RGB values for each zone
>
> 4. A enum_effects ioctl which takes a struct with the following members:
>
> {
> long size; /* Size of passed in struct including the size member itself */
> long effects_mask[]
> }
>
> the idea being that there is an enum with effects, which gets extended
> as we encounter more effects and the bitmask in effects_mask has a bit set
> for each effects enum value which is supported. effects_mask is an array
> so that we don't run out of bits. If older userspace only passes 1 long
> (size == (2*sizeof(long)) when 2 are needed at some point in the future
> then the kernel will simply only fill the first long.
>
> 5. A set_effect ioctl which takes a struct with the following members:
>
> {
> long size; /* Size of passed in struct including the size member itself */
> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
> int zone; /* zone to apply the effect to */
> int speed; /* cycle speed of the effect in milli-hz */
> char color1[3]; /* effect dependend may be unused. */
> char color2[3]; /* effect dependend may be unused. */
> }
>
> Again the idea with the size member is that the struct can be extended with
> new members if necessary and the kernel will supply a default value for
> older userspaces which provide a smaller struct (note size being smaller
> then sizeof(struct-v1) will invalid).
>
>
> Note this is all just a rough sketch suggestions welcome!
>
> Regards,
>
> Hans
>
>
>
--
Jani Nikula, Intel
Hi,
sorry have to resend, thunderbird html-ified the mail
Am 19.01.24 um 09:44 schrieb Hans de Goede:
> Hi,
>
> On 1/18/24 18:45, Pavel Machek wrote:
>> Hi!
>>
>>> We have an upcoming device that has a per-key keyboard backlight, but does
>>> the control completely via a wmi/acpi interface. So no usable hidraw here
>>> for a potential userspace driver implementation ...
>>>
>>> So a quick summary for the ideas floating in this thread so far:
>>>
>>> 1. Expand leds interface allowing arbitrary modes with semi arbitrary
>>> optional attributes:
>>> - Con:
>>>
>>> - Violates the simplicity paradigm of the leds interface (e.g. with
>>> this one leds entry controls possible multiple leds)
>> Let's not do this.
>>
>>> 2. Implement per-key keyboards as auxdisplay
>>>
>>> - Pro:
>>>
>>> - Already has a concept for led positions
>>>
>>> - Is conceptually closer to "multiple leds forming a singular entity"
>>>
>>> - Con:
>>>
>>> - No preexisting UPower support
>>>
>>> - No concept for special hardware lightning modes
>>>
>>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>> Please do this one.
> Ok, so based on the discussion so far and Pavel's feedback lets try to
> design a custom userspace API for this. I do not believe that auxdisplay
> is a good fit because:
>
> - auxdisplay is just a directory name, it does not seem to clearly
> define an API
>
> - instead the deprecated /dev/fb API is used which is deprecated
>
> - auxdisplays are very much displays (hence /dev/fb) they are typically
> small LCD displays with a straight widthxheight grid of square pixels
>
> - /dev/fb does gives us nothing for effects, zoned keyboard, etc.
I was just checking this and wanted to write something similar. When I wrote the
pro/con list I was mistaken that aux displays use either one of 2 APIs (charlcd
or fb), but I was mistaken. The 8 devices implemented there are actually using 5
different apis, some of them 2 at a time.
Just for reference the small list I wrote on the side just now:
arm-charlcd.c - own implementation without userspace interaction (just a static
text is displayed)
cfag12864b.c/cfag12864bfb.c - ks0108_isinited or register_framebuffer
hd44780.c - charlcd_register
ht16k33.c - linedisp_register or register_framebuffer
img-ascii-lcd.c - linedisp_register
ks0108.c - own implementetion using parport_register_dev_model
lcd2s.c - charlcd_register
panel.c - charlcd_register
> So my proposal would be an ioctl interface (ioctl only no r/w)
> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>
> For per key controllable rgb LEDs we need to discuss a coordinate
> system. I propose using a fixed size of 16 rows of 64 keys,
> so 64x16 in standard WxH notation.
>
> And then storing RGB in separate bytes, so userspace will then
> always send a buffer of 192 bytes per line (64x3) x 14 rows
> = 3072 bytes. With the kernel driver ignoring parts of
> the buffer where there are no actual keys.
The be sure the "14 rows" is a typo? And should be 16 rows?
> I would then like the map the standard 105 key layout onto this,
> starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
> the top left). Leaving plenty of space on the left top and right
> (and some on the bottom) for extra media key rows, macro keys, etc.
>
> The idea to have the standard layout at a fixed place is to allow
> userspace to have a database of preset patterns which will work
> everywhere.
>
> Note I say standard 105 key layout, but in reality for
> defining the standardized part of the buffer we should
> use the maximum amount of keys per row of all the standard layouts,
> so for row 6 (the ESC row) and for extra keys on the right outside
> the main block we use the standard layout as shown here:
>
> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>
> For the main area of the keyboard looking at:
>
> http://bopqehorizon.weebly.com/uploads/1/3/4/3/134337299/913246919_orig.png
>
> We want to max rows per key, so this means that per row we use
> (from the above image) :
>
> row 7: 106/109 - JIS
> row 8: 101/104 - ANSI
> row 9: 102/105 - ISO
> row 10: 104/107 - ABNT
> row 11: 106/109 - JIS
>
> (with row 7 being the main area top row)
>
> This way we can address all the possible keys in the various
> standard layouts in one standard wat and then the drivers can
> just skip keys which are not there when preparing the buffer
> to send to the hw / fw.
Some remarks here:
- Some keyboards might have two or more leds for big keys like (iso-)enter,
shift, capslock, num+, etc. that in theory are individually controllable by the
firmware. In windows drivers this is usually abstracted away, but could be
interesting for effects (e.g. if the top of iso-enter is separate from the
bottom of iso-enter like with one of our devices).
- In combination with this: The driver might not be able to tell if the actual
physical keyboard is ISO or ANSI, so it might not be able the correctly assign
the leds around enter correctly as being an own key or being part of ANSI- or
ISO-enter.
- Should the interface have different addresses for the different enter and num+
styles (or even the different length shifts and spacebars)?
One idea for this: Actually assign 1 value per line for tall keys per line, 3
(or maybe even 4, to have one spare) values per line for wide keys and 6 (or 8)
values for space. e.g.:
- Right shift would have 3 values in row 10. The first value might be the left
side of shift or the additional ABNT/JIS key. The 2nd Key might be the left side
or middle of shift and the third key might be the right side of shift or the
only value for the whole key. The additional ABNT/JIS key still also has a
dedicated value which is used by drivers which can differentiate between
physical layouts.
- Enter would have 3 values in row 8 and 3 values in row 9. With the same
disambiguation as the additional ABNT/JIS but this time for ansii-/ and iso-#
- Num+ would have 2 values, one row 8 and one in row 9. The one in row 9 might
control the whole key or might just control the lower half. The one in row 8
might be another key or the upper half
For the left half if the main block the leftmost value should be the "might be
the only relevant"-value while the right most value should be the "might be
another key"-value. For the right side of the main block this should be swapped.
Unused values should be adjacent to the "might be another key"-value, e.g.:
| Left shift value 1 | Left shift value 2 | Left shift value 3 | Left shift value 4 | 102nd key value
ISO/ANSI aware | Left shift color | Unused | Unused | Unused | 102nd key color
ISO non aware 1 led under shift | Left shift color | Unused | Unused | 102nd key color | Unused
ANSI non aware 1 led under shift | Left shift color | Unused | Unused | Unused | Unused
ISO non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | 102nd key color | Unused
ANSI non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | Unused | Unused
ISO non aware 3 leds under shift | Left shift left color | Left shift middle color | Left shift right color | 102nd key color | Unused
ANSI non aware 3 leds under shift | Left shift left color | Left shift middle color | Unused | Left shift right color | Unused
ANSI non aware 4 leds under shift | Left shift left color | Left shift middle left color | Left shift middle right color | Left shift right color | Unused
Like this with no information you can still reliable target the ANSI-shift
space, if you know it's an ISO keyboard from user space you can target shift and
102nd key, and if you have even more information you can have multi color shift
if the firmware supports it.
> One open question is if we should add padding after the main
> area so that the printscreen / ins / del / leftarrow of the
> "middle" block of
>
> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>
> all start at the same x (say 32) or we just pack these directly
> after the main area.
>
> And the same question for the numlock block, do we align
> this to an x of say 36, or pack it ?
With all that padding around I think a little padding in the middle wouldn't
hurt. Would even suggest a min padding of 1 to have some reserved space in there.
> As for the actual IOCTL API I think there should be
> the following ioctls:
>
> 1. A get-info ioctl returning a struct with the following members:
>
> {
> char name[64] /* Keyboard model name / identifier */
> int row_begin[16]; /* The x address of the first available key per row. On a std 105key kbd this will be 16 for rows 6-11, 0 for other rows */
> int row_end[16]; /* x+1 for the address of the last available key per row, end - begin gives number of keys in a row */
I guess you meant x-1 for the address, aka row_end[16] points to the address
behind the last value so that you can iterate over the row with: i = row_begin;
i < row_end; ++i
> int rgb_zones; /* number of rgb zones for zoned keyboards. Note both
> zones and per key addressing may be available if
> effects are applied per zone. */
> ?
> }
>
> 2. A set-leds ioctl which takes the earlier discussed 3092 bytes buffer
> to set all the LEDs at once, only valid if at least one row has a non 0 lenght.
>
> 3. A set-zones ioctl which takes an array of bytes sized 3 * number-of-zones
> containing RGB values for each zone
>
> 4. A enum_effects ioctl which takes a struct with the following members:
>
> {
> long size; /* Size of passed in struct including the size member itself */
> long effects_mask[]
> }
>
> the idea being that there is an enum with effects, which gets extended
> as we encounter more effects and the bitmask in effects_mask has a bit set
> for each effects enum value which is supported. effects_mask is an array
> so that we don't run out of bits. If older userspace only passes 1 long
> (size == (2*sizeof(long)) when 2 are needed at some point in the future
> then the kernel will simply only fill the first long.
>
> 5. A set_effect ioctl which takes a struct with the following members:
>
> {
> long size; /* Size of passed in struct including the size member itself */
> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
> int zone; /* zone to apply the effect to */
Don't know if this is necessary, the keyboards I have seen so far apply firmware
effects globally.
> int speed; /* cycle speed of the effect in milli-hz */
I would split this into speed and speed_max and don't specify an actual unit.
The firmwares effects I have seen so far: If they have a speed value, it's some
low number interpreted as a proportional x/n * the max speed of this effect,
with n being some low number like 8 or 10.
But i don't know if such clearly named properties are even sensefull, see below.
> char color1[3]; /* effect dependend may be unused. */
> char color2[3]; /* effect dependend may be unused. */
> }
We can not predetermine how many colors we might need in the future.
Firmware effects can vary vastly in complexity, e.g. breathing can be a single
bit switch that just varies the brightness of whatever color setting is
currently applied. It could have an optional speed argument. It could have nth
additional color arguments to cycle through, it could have an optional randomize
bit that either randomizes the order of the defined colors or means that it is
picking completely random color ignoring the color settings if set.
Like this we could have a very fast explosion of the effects enum e.g.:
breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors,
breathing_speed_controlled, breathing_speed_controlled_2_colors, ...
breathing_speed_controlled_n_colors_random_bit, etc.
Or we give up on generic names and just make something like:
tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2, clevo_breathing_1
Each with an own struct defined in a big .h file.
Otherwise I think the config struct needs to be dynamically created out of
information the driver gives to userspace.
> Again the idea with the size member is that the struct can be extended with
> new members if necessary and the kernel will supply a default value for
> older userspaces which provide a smaller struct (note size being smaller
> then sizeof(struct-v1) will invalid).
>
>
> Note this is all just a rough sketch suggestions welcome!
>
> Regards,
>
> Hans
>
>
>
Regards,
Werner
Hi,
Am 19.01.24 um 11:51 schrieb Jani Nikula:
> On Fri, 19 Jan 2024, Hans de Goede <[email protected]> wrote:
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
>>
>> I would then like the map the standard 105 key layout onto this,
>> starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
>> the top left). Leaving plenty of space on the left top and right
>> (and some on the bottom) for extra media key rows, macro keys, etc.
>>
>> The idea to have the standard layout at a fixed place is to allow
>> userspace to have a database of preset patterns which will work
>> everywhere.
>>
>> Note I say standard 105 key layout, but in reality for
>> defining the standardized part of the buffer we should
>> use the maximum amount of keys per row of all the standard layouts,
>> so for row 6 (the ESC row) and for extra keys on the right outside
>> the main block we use the standard layout as shown here:
> Doesn't the input stack already have to have pretty much all of this
> already covered? I can view the keyboard layout in my desktop
> environment, and it's a reasonably accurate match, even if unlikely to
> be pixel perfect. But crucially, it has to have all the possible layouts
> covered already.
>
> And while I would personally hate it, you can imagine a use case where
> you'd like a keypress to have a visual effect around the key you
> pressed. A kind of force feedback, if you will. I don't actually know,
> and correct me if I'm wrong, but feels like implementing that outside of
> the input subsystem would be non-trivial.
>
> Cc: Dmitry, could we at least have some input from the input subsystem
> POV on this? AFAICT we have received none.
>
>
> BR,
> Jani.
Don't forget: while we are currently discussing keyboards, in the future this
API imho should also be usefull for other RGB devices like mice, lightbars, etc.
Regards,
Werner
>
>
>> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>>
>> For the main area of the keyboard looking at:
>>
>> http://bopqehorizon.weebly.com/uploads/1/3/4/3/134337299/913246919_orig.png
>>
>> We want to max rows per key, so this means that per row we use
>> (from the above image) :
>>
>> row 7: 106/109 - JIS
>> row 8: 101/104 - ANSI
>> row 9: 102/105 - ISO
>> row 10: 104/107 - ABNT
>> row 11: 106/109 - JIS
>>
>> (with row 7 being the main area top row)
>>
>> This way we can address all the possible keys in the various
>> standard layouts in one standard wat and then the drivers can
>> just skip keys which are not there when preparing the buffer
>> to send to the hw / fw.
>>
>> One open question is if we should add padding after the main
>> area so that the printscreen / ins / del / leftarrow of the
>> "middle" block of
>>
>> http://www.maxkeyboard.com/images/105_ISO_6_25_Key_Layout.jpg
>>
>> all start at the same x (say 32) or we just pack these directly
>> after the main area.
>>
>> And the same question for the numlock block, do we align
>> this to an x of say 36, or pack it ?
>>
>>
>> As for the actual IOCTL API I think there should be
>> the following ioctls:
>>
>> 1. A get-info ioctl returning a struct with the following members:
>>
>> {
>> char name[64] /* Keyboard model name / identifier */
>> int row_begin[16]; /* The x address of the first available key per row. On a std 105key kbd this will be 16 for rows 6-11, 0 for other rows */
>> int row_end[16]; /* x+1 for the address of the last available key per row, end - begin gives number of keys in a row */
>> int rgb_zones; /* number of rgb zones for zoned keyboards. Note both
>> zones and per key addressing may be available if
>> effects are applied per zone. */
>> ?
>> }
>>
>> 2. A set-leds ioctl which takes the earlier discussed 3092 bytes buffer
>> to set all the LEDs at once, only valid if at least one row has a non 0 lenght.
>>
>> 3. A set-zones ioctl which takes an array of bytes sized 3 * number-of-zones
>> containing RGB values for each zone
>>
>> 4. A enum_effects ioctl which takes a struct with the following members:
>>
>> {
>> long size; /* Size of passed in struct including the size member itself */
>> long effects_mask[]
>> }
>>
>> the idea being that there is an enum with effects, which gets extended
>> as we encounter more effects and the bitmask in effects_mask has a bit set
>> for each effects enum value which is supported. effects_mask is an array
>> so that we don't run out of bits. If older userspace only passes 1 long
>> (size == (2*sizeof(long)) when 2 are needed at some point in the future
>> then the kernel will simply only fill the first long.
>>
>> 5. A set_effect ioctl which takes a struct with the following members:
>>
>> {
>> long size; /* Size of passed in struct including the size member itself */
>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
>> int zone; /* zone to apply the effect to */
>> int speed; /* cycle speed of the effect in milli-hz */
>> char color1[3]; /* effect dependend may be unused. */
>> char color2[3]; /* effect dependend may be unused. */
>> }
>>
>> Again the idea with the size member is that the struct can be extended with
>> new members if necessary and the kernel will supply a default value for
>> older userspaces which provide a smaller struct (note size being smaller
>> then sizeof(struct-v1) will invalid).
>>
>>
>> Note this is all just a rough sketch suggestions welcome!
>>
>> Regards,
>>
>> Hans
>>
>>
>>
On Fri, Jan 19, 2024 at 12:51:21PM +0200, Jani Nikula wrote:
> On Fri, 19 Jan 2024, Hans de Goede <[email protected]> wrote:
> > For per key controllable rgb LEDs we need to discuss a coordinate
> > system. I propose using a fixed size of 16 rows of 64 keys,
> > so 64x16 in standard WxH notation.
> >
> > And then storing RGB in separate bytes, so userspace will then
> > always send a buffer of 192 bytes per line (64x3) x 14 rows
> > = 3072 bytes. With the kernel driver ignoring parts of
> > the buffer where there are no actual keys.
> >
> > I would then like the map the standard 105 key layout onto this,
> > starting at x.y (column.row) coordinates of 16.6 (with 0.0 being
> > the top left). Leaving plenty of space on the left top and right
> > (and some on the bottom) for extra media key rows, macro keys, etc.
> >
> > The idea to have the standard layout at a fixed place is to allow
> > userspace to have a database of preset patterns which will work
> > everywhere.
> >
> > Note I say standard 105 key layout, but in reality for
> > defining the standardized part of the buffer we should
> > use the maximum amount of keys per row of all the standard layouts,
> > so for row 6 (the ESC row) and for extra keys on the right outside
> > the main block we use the standard layout as shown here:
>
> Doesn't the input stack already have to have pretty much all of this
> already covered? I can view the keyboard layout in my desktop
> environment, and it's a reasonably accurate match, even if unlikely to
> be pixel perfect. But crucially, it has to have all the possible layouts
> covered already.
The kernel actually is not aware of the keyboard geometry, it had no
idea if you are dealing with a standard full 101/102 keys keyboard,
TKL or even smaller one, if it is split or not, maybe something like
Kinesis Advantage360. Arguably, it could potentially know about
101/TLK if vendors would program accurate descriptors into their
devices, but nobody does... And geometry is not a part of HID interface
at all. So your desktop environment makes an [un]educated guess.
>
> And while I would personally hate it, you can imagine a use case where
> you'd like a keypress to have a visual effect around the key you
> pressed. A kind of force feedback, if you will. I don't actually know,
> and correct me if I'm wrong, but feels like implementing that outside of
> the input subsystem would be non-trivial.
Actually I think it does not belong to the input subsystem as it is,
where the goal is to deliver keystrokes and gestures to userspace. The
"force feedback" kind of fits, but not really practical, again because
of lack of geometry info. It is also not really essential to be fully
and automatically handled by the kernel. So I think the best way is to
have an API that is flexible enough for the userspace solution to
control, and that is not restricted by the input core design. The
hardware drivers are not restricted to using a single API, they can
implement both an input device and whatever new "rgbled" and userspace
can associate them by topology/sysfs.
>
> Cc: Dmitry, could we at least have some input from the input subsystem
> POV on this? AFAICT we have received none.
Sorry, I was not CCed and I missed this on the mainling list.
Thanks.
--
Dmitry
Hi!
> >> 2. Implement per-key keyboards as auxdisplay
> >>
> >> ??? - Pro:
> >>
> >> ??? ??? - Already has a concept for led positions
> >>
> >> ??? ??? - Is conceptually closer to "multiple leds forming a singular entity"
> >>
> >> ??? - Con:
> >>
> >> ??? ??? - No preexisting UPower support
> >>
> >> ??? ??? - No concept for special hardware lightning modes
> >>
> >> ??? ??? - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
> >
> > Please do this one.
>
> Ok, so based on the discussion so far and Pavel's feedback lets try to
> design a custom userspace API for this. I do not believe that auxdisplay
> is a good fit because:
Ok, so lets call this a "display". These days, framebuffers and drm
handles displays. My proposal is to use similar API as other displays.
> So my proposal would be an ioctl interface (ioctl only no r/w)
> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>
> For per key controllable rgb LEDs we need to discuss a coordinate
> system. I propose using a fixed size of 16 rows of 64 keys,
> so 64x16 in standard WxH notation.
>
> And then storing RGB in separate bytes, so userspace will then
> always send a buffer of 192 bytes per line (64x3) x 14 rows
> = 3072 bytes. With the kernel driver ignoring parts of
> the buffer where there are no actual keys.
That's really really weird interface. If you are doing RGB888 64x14,
lets make it a ... display? :-).
ioctl always sending 3072 bytes is really a hack.
Small displays exist and are quite common, surely we'd handle this as
a display:
https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini
It is 64x48.
And then there's this:
https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219
and this:
https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219
One of them is 8x8.
Surely those should be displays, too?
And yes, we'd probably want some extra ioctls on top, for example to
map from input device to this and back, and maybe for various effects,
too. And yes, I realize that display with holes in it and with some
pixels bigger than others is weird, but it still looks like a display
to me. (And phones have high-res displays with rounded corners and
holes in them, so... we'll need to deal with weird displays anyway).
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 19.01.24 um 21:15 schrieb Pavel Machek:
> Hi!
>
>>>> 2. Implement per-key keyboards as auxdisplay
>>>>
>>>> - Pro:
>>>>
>>>> - Already has a concept for led positions
>>>>
>>>> - Is conceptually closer to "multiple leds forming a singular entity"
>>>>
>>>> - Con:
>>>>
>>>> - No preexisting UPower support
>>>>
>>>> - No concept for special hardware lightning modes
>>>>
>>>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>>> Please do this one.
>> Ok, so based on the discussion so far and Pavel's feedback lets try to
>> design a custom userspace API for this. I do not believe that auxdisplay
>> is a good fit because:
> Ok, so lets call this a "display". These days, framebuffers and drm
> handles displays. My proposal is to use similar API as other displays.
>
>> So my proposal would be an ioctl interface (ioctl only no r/w)
>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
> That's really really weird interface. If you are doing RGB888 64x14,
> lets make it a ... display? :-).
>
> ioctl always sending 3072 bytes is really a hack.
>
> Small displays exist and are quite common, surely we'd handle this as
> a display:
> https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini
> It is 64x48.
>
> And then there's this:
> https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219
> and this:
> https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219
>
> One of them is 8x8.
>
> Surely those should be displays, too?
But what about a light bar with, lets say, 3 zones. Is that a 3x1 display?
And what about a mouse having lit mousebuttons and a single led light bar at the
wrist: a 2x2 display, but one is thin but long and one is not used?
Regards,
Werner
>
> And yes, we'd probably want some extra ioctls on top, for example to
> map from input device to this and back, and maybe for various effects,
> too. And yes, I realize that display with holes in it and with some
> pixels bigger than others is weird, but it still looks like a display
> to me. (And phones have high-res displays with rounded corners and
> holes in them, so... we'll need to deal with weird displays anyway).
>
> Best regards,
> Pavel
Hi!
> > > And then storing RGB in separate bytes, so userspace will then
> > > always send a buffer of 192 bytes per line (64x3) x 14 rows
> > > = 3072 bytes. With the kernel driver ignoring parts of
> > > the buffer where there are no actual keys.
> > That's really really weird interface. If you are doing RGB888 64x14,
> > lets make it a ... display? :-).
> >
> > ioctl always sending 3072 bytes is really a hack.
> >
> > Small displays exist and are quite common, surely we'd handle this as
> > a display:
> > https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini
> > It is 64x48.
> >
> > And then there's this:
> > https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219
> > and this:
> > https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219
> >
> > One of them is 8x8.
> >
> > Surely those should be displays, too?
>
> But what about a light bar with, lets say, 3 zones. Is that a 3x1 display?
>
> And what about a mouse having lit mousebuttons and a single led light bar at
> the wrist: a 2x2 display, but one is thin but long and one is not used?
So indeed LEDs can arranged into various shapes. Like a ring, or this:
* *
* * *
* *
https://pajenicko.cz/led-moduly?page=2
Dunno. Sounds like a display is still a best match for them. Some of
modules are RGB, some are single-color only, I'm sure there will be
various bit depths.
I guess we can do 3x1 and 2x2 displays. Or we could try to solve
keyboards and ignore those for now.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> > And while I would personally hate it, you can imagine a use case where
> > you'd like a keypress to have a visual effect around the key you
> > pressed. A kind of force feedback, if you will. I don't actually know,
> > and correct me if I'm wrong, but feels like implementing that outside of
> > the input subsystem would be non-trivial.
>
> Actually I think it does not belong to the input subsystem as it is,
> where the goal is to deliver keystrokes and gestures to userspace. The
> "force feedback" kind of fits, but not really practical, again because
> of lack of geometry info. It is also not really essential to be fully
> and automatically handled by the kernel. So I think the best way is
> > to
So that's actually big question.
If the common usage is "run bad apple demo on keyboard" than pretty
clearly it should be display.
If the common usage is "computer is asking yes/no question, so
highlight yes and no buttons", then there are good arguments why input
should handle that (as it does capslock led, for example).
Actually I could imagine "real" use when shift / control /alt
backlight would indicate sticky-shift keys for handicapped.
It seems they are making mice with backlit buttons. If the main use is
highlight this key whereever it is, then it should be input.
But I suspect may use is just fancy colors and it should be display.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Am 19.01.24 um 23:14 schrieb Pavel Machek:
> Hi!
>
>>> And while I would personally hate it, you can imagine a use case where
>>> you'd like a keypress to have a visual effect around the key you
>>> pressed. A kind of force feedback, if you will. I don't actually know,
>>> and correct me if I'm wrong, but feels like implementing that outside of
>>> the input subsystem would be non-trivial.
>> Actually I think it does not belong to the input subsystem as it is,
>> where the goal is to deliver keystrokes and gestures to userspace. The
>> "force feedback" kind of fits, but not really practical, again because
>> of lack of geometry info. It is also not really essential to be fully
>> and automatically handled by the kernel. So I think the best way is
>>> to
> So that's actually big question.
>
> If the common usage is "run bad apple demo on keyboard" than pretty
> clearly it should be display.
>
> If the common usage is "computer is asking yes/no question, so
> highlight yes and no buttons", then there are good arguments why input
> should handle that (as it does capslock led, for example).
The common usage is "make keyboard look flashy", for some a fixed color scheme
is enough, other ones might probably enable one of the built in modes. Most
people I think will be satisfied with these 2 options, albeit both of your
suggestions sound cool.
>
> Actually I could imagine "real" use when shift / control /alt
> backlight would indicate sticky-shift keys for handicapped.
>
> It seems they are making mice with backlit buttons. If the main use is
> highlight this key whereever it is, then it should be input.
>
> But I suspect may use is just fancy colors and it should be display.
>
> Best regards,
> Pavel
Hi,
On 1/19/24 21:15, Pavel Machek wrote:
> Hi!
>
>>>> 2. Implement per-key keyboards as auxdisplay
>>>>
>>>> - Pro:
>>>>
>>>> - Already has a concept for led positions
>>>>
>>>> - Is conceptually closer to "multiple leds forming a singular entity"
>>>>
>>>> - Con:
>>>>
>>>> - No preexisting UPower support
>>>>
>>>> - No concept for special hardware lightning modes
>>>>
>>>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>>>
>>> Please do this one.
>>
>> Ok, so based on the discussion so far and Pavel's feedback lets try to
>> design a custom userspace API for this. I do not believe that auxdisplay
>> is a good fit because:
>
> Ok, so lets call this a "display". These days, framebuffers and drm
> handles displays. My proposal is to use similar API as other displays.
>
>> So my proposal would be an ioctl interface (ioctl only no r/w)
>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
>
> That's really really weird interface. If you are doing RGB888 64x14,
> lets make it a ... display? :-).
>
> ioctl always sending 3072 bytes is really a hack.
>
> Small displays exist and are quite common, surely we'd handle this as
> a display:
> https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini
> It is 64x48.
This is indeed a display and should use display APIs
> And then there's this:
> https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219
> and this:
> https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219
>
> One of them is 8x8.
>
> Surely those should be displays, too?
The 8x8 one not really, the other one could be used to scroll
some text one but cannot display images, so not really displays
IMHO.
Anyways we are talking about keyboards here and those do not have
a regular x-y grid like your example above, so they certainly do
not count as displays. See the long discussion earlier in the thread.
Regards,
Hans
Hi Werner,
On 1/19/24 17:04, Werner Sembach wrote:
> Am 19.01.24 um 09:44 schrieb Hans de Goede:
<snip>
>> So my proposal would be an ioctl interface (ioctl only no r/w)
>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
> The be sure the "14 rows" is a typo? And should be 16 rows?
Yes that should be 16.
<snip>
>> This way we can address all the possible keys in the various
>> standard layouts in one standard wat and then the drivers can
>> just skip keys which are not there when preparing the buffer
>> to send to the hw / fw.
>
> Some remarks here:
>
> - Some keyboards might have two or more leds for big keys like (iso-)enter, shift, capslock, num+, etc. that in theory are individually controllable by the firmware. In windows drivers this is usually abstracted away, but could be interesting for effects (e.g. if the top of iso-enter is separate from the bottom of iso-enter like with one of our devices).
>
> - In combination with this: The driver might not be able to tell if the actual physical keyboard is ISO or ANSI, so it might not be able the correctly assign the leds around enter correctly as being an own key or being part of ANSI- or ISO-enter.
>
> - Should the interface have different addresses for the different enter and num+ styles (or even the different length shifts and spacebars)?
>
> One idea for this: Actually assign 1 value per line for tall keys per line, 3 (or maybe even 4, to have one spare) values per line for wide keys and 6 (or 8) values for space. e.g.:
That sounds workable OTOH combined with your remarks about also supporting
lightbars. I'm starting to think that we need to just punt this to userspace.
So basically change things from trying to present a standardized address
space where say the 'Q' key is always in the same place just model
a keyboard as a string of LEDs (1 dimensional / so an array) and leave
mapping which address in the array is which key to userspace, then userspace
can have json or whatever files for this per keyboard.
This keeps the kernel interface much more KISS which I think is what
we need to strive for.
So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then
that can be used for rbb-kbds and also your lightbar example as well
as actual RGB LED strings, which depending on the controller may
also have zones / effects, etc. just like the keyboards.
> - Right shift would have 3 values in row 10. The first value might be the left side of shift or the additional ABNT/JIS key. The 2nd Key might be the left side or middle of shift and the third key might be the right side of shift or the only value for the whole key. The additional ABNT/JIS key still also has a dedicated value which is used by drivers which can differentiate between physical layouts.
>
> - Enter would have 3 values in row 8 and 3 values in row 9. With the same disambiguation as the additional ABNT/JIS but this time for ansii-/ and iso-#
>
> - Num+ would have 2 values, one row 8 and one in row 9. The one in row 9 might control the whole key or might just control the lower half. The one in row 8 might be another key or the upper half
>
> For the left half if the main block the leftmost value should be the "might be the only relevant"-value while the right most value should be the "might be another key"-value. For the right side of the main block this should be swapped. Unused values should be adjacent to the "might be another key"-value, e.g.:
>
> | Left shift value 1 | Left shift value 2 | Left shift value 3 | Left shift value 4 | 102nd key value
> ISO/ANSI aware | Left shift color | Unused | Unused | Unused | 102nd key color
> ISO non aware 1 led under shift | Left shift color | Unused | Unused | 102nd key color | Unused
> ANSI non aware 1 led under shift | Left shift color | Unused | Unused | Unused | Unused
> ISO non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | 102nd key color | Unused
> ANSI non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | Unused | Unused
> ISO non aware 3 leds under shift | Left shift left color | Left shift middle color | Left shift right color | 102nd key color | Unused
> ANSI non aware 3 leds under shift | Left shift left color | Left shift middle color | Unused | Left shift right color | Unused
> ANSI non aware 4 leds under shift | Left shift left color | Left shift middle left color | Left shift middle right color | Left shift right color | Unused
>
> Like this with no information you can still reliable target the ANSI-shift space, if you know it's an ISO keyboard from user space you can target shift and 102nd key, and if you have even more information you can have multi color shift if the firmware supports it.
Right, so see above I think we need to push all these complications
into userspace. And simple come up for a kernel interface
for RGB LED strings with zones / effects / possibly individual
addressable LEDs.
Also we should really only use whatever kernel interface we come up
with for devices which cannot be supported directly from userspace
through e.g. hidraw access. Looking at keyboards then the openrgb project:
https://openrgb.org/devices_0.9.html
Currently already supports 398 keyboard modes, we really do not want
to be adding support for all those to the kernel.
Further down in the thread you mention Mice with RGB LEDs,
Mice are almost always HID devices and already have extensive support,
including for their LEDs in userspace through libratbag and the piper UI,
see the screenshots here (click on the camera icon):
https://linux.softpedia.com/get/Utilities/Piper-libratbag-104168.shtml
Again we really don't want to be re-doing all this work in the kernel
only to end up conflicting with the existing userspace support.
<snip>
>> 5. A set_effect ioctl which takes a struct with the following members:
>>
>> {
>> long size; /* Size of passed in struct including the size member itself */
>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
>> int zone; /* zone to apply the effect to */
> Don't know if this is necessary, the keyboards I have seen so far apply firmware effects globally.
>> int speed; /* cycle speed of the effect in milli-hz */
>
> I would split this into speed and speed_max and don't specify an actual unit. The firmwares effects I have seen so far: If they have a speed value, it's some low number interpreted as a proportional x/n * the max speed of this effect, with n being some low number like 8 or 10.
>
> But i don't know if such clearly named properties are even sensefull, see below.
>
>> char color1[3]; /* effect dependend may be unused. */
>> char color2[3]; /* effect dependend may be unused. */
>> }
>
> We can not predetermine how many colors we might need in the future.
>
> Firmware effects can vary vastly in complexity, e.g. breathing can be a single bit switch that just varies the brightness of whatever color setting is currently applied. It could have an optional speed argument. It could have nth additional color arguments to cycle through, it could have an optional randomize bit that either randomizes the order of the defined colors or means that it is picking completely random color ignoring the color settings if set.
>
> Like this we could have a very fast explosion of the effects enum e.g.: breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors, breathing_speed_controlled, breathing_speed_controlled_2_colors, ... breathing_speed_controlled_n_colors_random_bit, etc.
>
> Or we give up on generic names and just make something like: tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2, clevo_breathing_1
>
> Each with an own struct defined in a big .h file.
>
> Otherwise I think the config struct needs to be dynamically created out of information the driver gives to userspace.
Given that as mentioned above I believe that we should only use a kernel
driver where direct userspace access is impossible I believe that having
things like tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2,
clevo_breathing_1, etc. for the hopefully small set of devices which
needs an actual kernel driver to be reasonable.
Talking about existing RGB LED support I believe that we should also
reach out to and get feedback on (or even an ack for) the new rgbledstring
API from the OpenRGB folks: https://openrgb.org
Maybe they already have a nice abstraction to deal with different
kind of effects which we can copy for the kernel API ?
Regards,
Hans
Hi Hans,
Am 29.01.24 um 14:24 schrieb Hans de Goede:
> Hi Werner,
>
> On 1/19/24 17:04, Werner Sembach wrote:
>> Am 19.01.24 um 09:44 schrieb Hans de Goede:
> <snip>
>
>>> So my proposal would be an ioctl interface (ioctl only no r/w)
>>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>>
>>> For per key controllable rgb LEDs we need to discuss a coordinate
>>> system. I propose using a fixed size of 16 rows of 64 keys,
>>> so 64x16 in standard WxH notation.
>>>
>>> And then storing RGB in separate bytes, so userspace will then
>>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>>> = 3072 bytes. With the kernel driver ignoring parts of
>>> the buffer where there are no actual keys.
>> The be sure the "14 rows" is a typo? And should be 16 rows?
> Yes that should be 16.
>
> <snip>
>
>>> This way we can address all the possible keys in the various
>>> standard layouts in one standard wat and then the drivers can
>>> just skip keys which are not there when preparing the buffer
>>> to send to the hw / fw.
>> Some remarks here:
>>
>> - Some keyboards might have two or more leds for big keys like (iso-)enter, shift, capslock, num+, etc. that in theory are individually controllable by the firmware. In windows drivers this is usually abstracted away, but could be interesting for effects (e.g. if the top of iso-enter is separate from the bottom of iso-enter like with one of our devices).
>>
>> - In combination with this: The driver might not be able to tell if the actual physical keyboard is ISO or ANSI, so it might not be able the correctly assign the leds around enter correctly as being an own key or being part of ANSI- or ISO-enter.
>>
>> - Should the interface have different addresses for the different enter and num+ styles (or even the different length shifts and spacebars)?
>>
>> One idea for this: Actually assign 1 value per line for tall keys per line, 3 (or maybe even 4, to have one spare) values per line for wide keys and 6 (or 8) values for space. e.g.:
> That sounds workable OTOH combined with your remarks about also supporting
> lightbars. I'm starting to think that we need to just punt this to userspace.
>
> So basically change things from trying to present a standardized address
> space where say the 'Q' key is always in the same place just model
> a keyboard as a string of LEDs (1 dimensional / so an array) and leave
> mapping which address in the array is which key to userspace, then userspace
> can have json or whatever files for this per keyboard.
>
> This keeps the kernel interface much more KISS which I think is what
> we need to strive for.
>
> So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then
> that can be used for rbb-kbds and also your lightbar example as well
> as actual RGB LED strings, which depending on the controller may
> also have zones / effects, etc. just like the keyboards.
>
>
>
>> - Right shift would have 3 values in row 10. The first value might be the left side of shift or the additional ABNT/JIS key. The 2nd Key might be the left side or middle of shift and the third key might be the right side of shift or the only value for the whole key. The additional ABNT/JIS key still also has a dedicated value which is used by drivers which can differentiate between physical layouts.
>>
>> - Enter would have 3 values in row 8 and 3 values in row 9. With the same disambiguation as the additional ABNT/JIS but this time for ansii-/ and iso-#
>>
>> - Num+ would have 2 values, one row 8 and one in row 9. The one in row 9 might control the whole key or might just control the lower half. The one in row 8 might be another key or the upper half
>>
>> For the left half if the main block the leftmost value should be the "might be the only relevant"-value while the right most value should be the "might be another key"-value. For the right side of the main block this should be swapped. Unused values should be adjacent to the "might be another key"-value, e.g.:
>>
>> | Left shift value 1 | Left shift value 2 | Left shift value 3 | Left shift value 4 | 102nd key value
>> ISO/ANSI aware | Left shift color | Unused | Unused | Unused | 102nd key color
>> ISO non aware 1 led under shift | Left shift color | Unused | Unused | 102nd key color | Unused
>> ANSI non aware 1 led under shift | Left shift color | Unused | Unused | Unused | Unused
>> ISO non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | 102nd key color | Unused
>> ANSI non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | Unused | Unused
>> ISO non aware 3 leds under shift | Left shift left color | Left shift middle color | Left shift right color | 102nd key color | Unused
>> ANSI non aware 3 leds under shift | Left shift left color | Left shift middle color | Unused | Left shift right color | Unused
>> ANSI non aware 4 leds under shift | Left shift left color | Left shift middle left color | Left shift middle right color | Left shift right color | Unused
>>
>> Like this with no information you can still reliable target the ANSI-shift space, if you know it's an ISO keyboard from user space you can target shift and 102nd key, and if you have even more information you can have multi color shift if the firmware supports it.
> Right, so see above I think we need to push all these complications
> into userspace. And simple come up for a kernel interface
> for RGB LED strings with zones / effects / possibly individual
> addressable LEDs.
>
> Also we should really only use whatever kernel interface we come up
> with for devices which cannot be supported directly from userspace
> through e.g. hidraw access. Looking at keyboards then the openrgb project:
>
> https://openrgb.org/devices_0.9.html
>
> Currently already supports 398 keyboard modes, we really do not want
> to be adding support for all those to the kernel.
I think that are mostly external keyboards, so in theory a possible cut could
also between built-in and external devices.
>
> Further down in the thread you mention Mice with RGB LEDs,
> Mice are almost always HID devices and already have extensive support,
> including for their LEDs in userspace through libratbag and the piper UI,
> see the screenshots here (click on the camera icon):
> https://linux.softpedia.com/get/Utilities/Piper-libratbag-104168.shtml
>
> Again we really don't want to be re-doing all this work in the kernel
> only to end up conflicting with the existing userspace support.
>
> <snip>
>
>>> 5. A set_effect ioctl which takes a struct with the following members:
>>>
>>> {
>>> long size; /* Size of passed in struct including the size member itself */
>>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
>>> int zone; /* zone to apply the effect to */
>> Don't know if this is necessary, the keyboards I have seen so far apply firmware effects globally.
>>> int speed; /* cycle speed of the effect in milli-hz */
>> I would split this into speed and speed_max and don't specify an actual unit. The firmwares effects I have seen so far: If they have a speed value, it's some low number interpreted as a proportional x/n * the max speed of this effect, with n being some low number like 8 or 10.
>>
>> But i don't know if such clearly named properties are even sensefull, see below.
>>
>>> char color1[3]; /* effect dependend may be unused. */
>>> char color2[3]; /* effect dependend may be unused. */
>>> }
>> We can not predetermine how many colors we might need in the future.
>>
>> Firmware effects can vary vastly in complexity, e.g. breathing can be a single bit switch that just varies the brightness of whatever color setting is currently applied. It could have an optional speed argument. It could have nth additional color arguments to cycle through, it could have an optional randomize bit that either randomizes the order of the defined colors or means that it is picking completely random color ignoring the color settings if set.
>>
>> Like this we could have a very fast explosion of the effects enum e.g.: breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors, breathing_speed_controlled, breathing_speed_controlled_2_colors, ... breathing_speed_controlled_n_colors_random_bit, etc.
>>
>> Or we give up on generic names and just make something like: tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2, clevo_breathing_1
>>
>> Each with an own struct defined in a big .h file.
>>
>> Otherwise I think the config struct needs to be dynamically created out of information the driver gives to userspace.
> Given that as mentioned above I believe that we should only use a kernel
> driver where direct userspace access is impossible I believe that having
> things like tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2,
> clevo_breathing_1, etc. for the hopefully small set of devices which
> needs an actual kernel driver to be reasonable.
So also no basic driver? Or still the concept from before with a basic 1 zone
only driver via leds subsystem to have something working, but it is unregistered
by userspace, if open rgb wants to take over for fine granular support?
>
> Talking about existing RGB LED support I believe that we should also
> reach out to and get feedback on (or even an ack for) the new rgbledstring
> API from the OpenRGB folks: https://openrgb.org
>
> Maybe they already have a nice abstraction to deal with different
> kind of effects which we can copy for the kernel API ?
I opened an issue regarding this:
https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916
>
> Regards,
>
> Hans
>
>
>
Kind regards,
Werner
Hi Werner,
On 1/30/24 12:12, Werner Sembach wrote:
> Hi Hans,
>
> Am 29.01.24 um 14:24 schrieb Hans de Goede:
<snip>
>> That sounds workable OTOH combined with your remarks about also supporting
>> lightbars. I'm starting to think that we need to just punt this to userspace.
>>
>> So basically change things from trying to present a standardized address
>> space where say the 'Q' key is always in the same place just model
>> a keyboard as a string of LEDs (1 dimensional / so an array) and leave
>> mapping which address in the array is which key to userspace, then userspace
>> can have json or whatever files for this per keyboard.
>>
>> This keeps the kernel interface much more KISS which I think is what
>> we need to strive for.
>>
>> So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then
>> that can be used for rbb-kbds and also your lightbar example as well
>> as actual RGB LED strings, which depending on the controller may
>> also have zones / effects, etc. just like the keyboards.
<snip>
>> Right, so see above I think we need to push all these complications
>> into userspace. And simple come up for a kernel interface
>> for RGB LED strings with zones / effects / possibly individual
>> addressable LEDs.
>>
>> Also we should really only use whatever kernel interface we come up
>> with for devices which cannot be supported directly from userspace
>> through e.g. hidraw access. Looking at keyboards then the openrgb project:
>>
>> https://openrgb.org/devices_0.9.html
>>
>> Currently already supports 398 keyboard modes, we really do not want
>> to be adding support for all those to the kernel.
> I think that are mostly external keyboards, so in theory a possible cut could also between built-in and external devices.
IMHO it would be better to limit /dev/rgbledstring use to only
cases where direct userspace control is not possible and thus
have the cut be based on whether direct userspace control
(e.g. /dev/hidraw access) is possible or not.
>> Further down in the thread you mention Mice with RGB LEDs,
>> Mice are almost always HID devices and already have extensive support,
>> including for their LEDs in userspace through libratbag and the piper UI,
>> see the screenshots here (click on the camera icon):
>> https://linux.softpedia.com/get/Utilities/Piper-libratbag-104168.shtml
>>
>> Again we really don't want to be re-doing all this work in the kernel
>> only to end up conflicting with the existing userspace support.
>>
>> <snip>
>>
>>>> 5. A set_effect ioctl which takes a struct with the following members:
>>>>
>>>> {
>>>> long size; /* Size of passed in struct including the size member itself */
>>>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
>>>> int zone; /* zone to apply the effect to */
>>> Don't know if this is necessary, the keyboards I have seen so far apply firmware effects globally.
>>>> int speed; /* cycle speed of the effect in milli-hz */
>>> I would split this into speed and speed_max and don't specify an actual unit. The firmwares effects I have seen so far: If they have a speed value, it's some low number interpreted as a proportional x/n * the max speed of this effect, with n being some low number like 8 or 10.
>>>
>>> But i don't know if such clearly named properties are even sensefull, see below.
>>>
>>>> char color1[3]; /* effect dependend may be unused. */
>>>> char color2[3]; /* effect dependend may be unused. */
>>>> }
>>> We can not predetermine how many colors we might need in the future.
>>>
>>> Firmware effects can vary vastly in complexity, e.g. breathing can be a single bit switch that just varies the brightness of whatever color setting is currently applied. It could have an optional speed argument. It could have nth additional color arguments to cycle through, it could have an optional randomize bit that either randomizes the order of the defined colors or means that it is picking completely random color ignoring the color settings if set.
>>>
>>> Like this we could have a very fast explosion of the effects enum e.g.: breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors, breathing_speed_controlled, breathing_speed_controlled_2_colors, ... breathing_speed_controlled_n_colors_random_bit, etc.
>>>
>>> Or we give up on generic names and just make something like: tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2, clevo_breathing_1
>>>
>>> Each with an own struct defined in a big .h file.
>>>
>>> Otherwise I think the config struct needs to be dynamically created out of information the driver gives to userspace.
>> Given that as mentioned above I believe that we should only use a kernel
>> driver where direct userspace access is impossible I believe that having
>> things like tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2,
>> clevo_breathing_1, etc. for the hopefully small set of devices which
>> needs an actual kernel driver to be reasonable.
> So also no basic driver? Or still the concept from before with a basic 1 zone only driver via leds subsystem to have something working, but it is unregistered by userspace, if open rgb wants to take over for fine granular support?
Ah good point, no I think that a basic driver just for kbd backlight
brightness support which works with the standard desktop environment
controls for this makes sense.
Combined with some mechanism for e.g. openrgb to fully take over
control as discussed. It is probably a good idea to file a separate
issue with the openrgb project to discuss the takeover API.
>> Talking about existing RGB LED support I believe that we should also
>> reach out to and get feedback on (or even an ack for) the new rgbledstring
>> API from the OpenRGB folks: https://openrgb.org
>>
>> Maybe they already have a nice abstraction to deal with different
>> kind of effects which we can copy for the kernel API ?
> I opened an issue regarding this: https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916
Great, thank you.
Regards,
Hans
Hi Hans,
resend because Thunderbird htmlified the mail :/
Am 30.01.24 um 18:10 schrieb Hans de Goede:
> Hi Werner,
>
> On 1/30/24 12:12, Werner Sembach wrote:
>> Hi Hans,
>>
>> Am 29.01.24 um 14:24 schrieb Hans de Goede:
<snip>
>> I think that are mostly external keyboards, so in theory a possible cut could also between built-in and external devices.
> IMHO it would be better to limit /dev/rgbledstring use to only
> cases where direct userspace control is not possible and thus
> have the cut be based on whether direct userspace control
> (e.g. /dev/hidraw access) is possible or not.
Ack
<snip>
>> So also no basic driver? Or still the concept from before with a basic 1 zone only driver via leds subsystem to have something working, but it is unregistered by userspace, if open rgb wants to take over for fine granular support?
> Ah good point, no I think that a basic driver just for kbd backlight
> brightness support which works with the standard desktop environment
> controls for this makes sense.
>
> Combined with some mechanism for e.g. openrgb to fully take over
> control as discussed. It is probably a good idea to file a separate
> issue with the openrgb project to discuss the takeover API.
I think the OpenRGB maintainers are pretty flexible at that point, after all
it's similar to enable commands a lot of rgb devices need anyway. I would
include it in a full api proposal.
On this note: Any particular reason you suggested an ioctl interface instead of
a sysfs one? (Open question as, for example, I have no idea what performance
implications both have)
<snip>
>> I opened an issue regarding this:https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916
> Great, thank you.
First replies are in.
> Regards,
>
> Hans
Kind regards,
Werner
Hi,
On 1/30/24 19:09, Werner Sembach wrote:
> Hi Hans,
>
> resend because Thunderbird htmlified the mail :/
I use thunderbird too. If you right click on the server name
and then go to "Settings" -> "Composition & Addressing"
and then uncheck "Compose messages in HTML format"
I think that should do the trick.
> Am 30.01.24 um 18:10 schrieb Hans de Goede:
>> Hi Werner,
>>
>> On 1/30/24 12:12, Werner Sembach wrote:
>>> Hi Hans,
>>>
>>> Am 29.01.24 um 14:24 schrieb Hans de Goede:
> <snip>
>>> I think that are mostly external keyboards, so in theory a possible cut could also between built-in and external devices.
>> IMHO it would be better to limit /dev/rgbledstring use to only
>> cases where direct userspace control is not possible and thus
>> have the cut be based on whether direct userspace control
>> (e.g. /dev/hidraw access) is possible or not.
>
> Ack
>
> <snip>
>
>>> So also no basic driver? Or still the concept from before with a basic 1 zone only driver via leds subsystem to have something working, but it is unregistered by userspace, if open rgb wants to take over for fine granular support?
>> Ah good point, no I think that a basic driver just for kbd backlight
>> brightness support which works with the standard desktop environment
>> controls for this makes sense.
>>
>> Combined with some mechanism for e.g. openrgb to fully take over
>> control as discussed. It is probably a good idea to file a separate
>> issue with the openrgb project to discuss the takeover API.
>
> I think the OpenRGB maintainers are pretty flexible at that point, after all it's similar to enable commands a lot of rgb devices need anyway. I would include it in a full api proposal.
Ack.
> On this note: Any particular reason you suggested an ioctl interface instead of a sysfs one? (Open question as, for example, I have no idea what performance implications both have)
sysfs APIs typically have a one file per setting approach,
so for effects with speed and multiple-color settings you
would need a whole bunch of different files and then you
would either need to immediately apply every setting,
needing multiple writes to the hw for a single effect
update, or have some sort of "commit" sysfs attribute.
With ioctls you can simply provide all the settings
in one call, which is why I suggested using ioctls.
Regards,
Hans
Hi,
Am 30.01.24 um 19:35 schrieb Hans de Goede:
> Hi,
>
> On 1/30/24 19:09, Werner Sembach wrote:
>> Hi Hans,
>>
>> resend because Thunderbird htmlified the mail :/
> I use thunderbird too. If you right click on the server name
> and then go to "Settings" -> "Composition & Addressing"
> and then uncheck "Compose messages in HTML format"
> I think that should do the trick.
Can't set this globally or other people will complain that my replies delete
company logos in signatures xD. But usually the auto detection of Thunderbird works.
>
>> Am 30.01.24 um 18:10 schrieb Hans de Goede:
>>> Hi Werner,
>>>
>>> On 1/30/24 12:12, Werner Sembach wrote:
>>>> Hi Hans,
>>>>
>>>> Am 29.01.24 um 14:24 schrieb Hans de Goede:
>> <snip>
>>>> I think that are mostly external keyboards, so in theory a possible cut could also between built-in and external devices.
>>> IMHO it would be better to limit /dev/rgbledstring use to only
>>> cases where direct userspace control is not possible and thus
>>> have the cut be based on whether direct userspace control
>>> (e.g. /dev/hidraw access) is possible or not.
>> Ack
>>
>> <snip>
>>
>>>> So also no basic driver? Or still the concept from before with a basic 1 zone only driver via leds subsystem to have something working, but it is unregistered by userspace, if open rgb wants to take over for fine granular support?
>>> Ah good point, no I think that a basic driver just for kbd backlight
>>> brightness support which works with the standard desktop environment
>>> controls for this makes sense.
>>>
>>> Combined with some mechanism for e.g. openrgb to fully take over
>>> control as discussed. It is probably a good idea to file a separate
>>> issue with the openrgb project to discuss the takeover API.
>> I think the OpenRGB maintainers are pretty flexible at that point, after all it's similar to enable commands a lot of rgb devices need anyway. I would include it in a full api proposal.
> Ack.
>
>> On this note: Any particular reason you suggested an ioctl interface instead of a sysfs one? (Open question as, for example, I have no idea what performance implications both have)
> sysfs APIs typically have a one file per setting approach,
> so for effects with speed and multiple-color settings you
> would need a whole bunch of different files and then you
> would either need to immediately apply every setting,
> needing multiple writes to the hw for a single effect
> update, or have some sort of "commit" sysfs attribute.
>
> With ioctls you can simply provide all the settings
> in one call, which is why I suggested using ioctls.
Ack
If the static mode update is fast enough to have userspace controlled
animations, OpenRGB is calling that direct mode. Is it feasible to send 30 or
more ioctls per second for such an direct mode? Or should this spawn a special
purpose sysfs file that is kept open by userspace to continuously update the
keyboard?
>
> Regards,
>
> Hans
>
>
>
Regards,
Werner
Hi,
On 1/30/24 20:08, Werner Sembach wrote:
> Hi,
>
> Am 30.01.24 um 19:35 schrieb Hans de Goede:
>> Hi,
>>
>> On 1/30/24 19:09, Werner Sembach wrote:
>>> Hi Hans,
>>>
>>> resend because Thunderbird htmlified the mail :/
>> I use thunderbird too. If you right click on the server name
>> and then go to "Settings" -> "Composition & Addressing"
>> and then uncheck "Compose messages in HTML format"
>> I think that should do the trick.
> Can't set this globally or other people will complain that my replies delete company logos in signatures xD. But usually the auto detection of Thunderbird works.
>>
>>> Am 30.01.24 um 18:10 schrieb Hans de Goede:
>>>> Hi Werner,
>>>>
>>>> On 1/30/24 12:12, Werner Sembach wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Am 29.01.24 um 14:24 schrieb Hans de Goede:
>>> <snip>
>>>>> I think that are mostly external keyboards, so in theory a possible cut could also between built-in and external devices.
>>>> IMHO it would be better to limit /dev/rgbledstring use to only
>>>> cases where direct userspace control is not possible and thus
>>>> have the cut be based on whether direct userspace control
>>>> (e.g. /dev/hidraw access) is possible or not.
>>> Ack
>>>
>>> <snip>
>>>
>>>>> So also no basic driver? Or still the concept from before with a basic 1 zone only driver via leds subsystem to have something working, but it is unregistered by userspace, if open rgb wants to take over for fine granular support?
>>>> Ah good point, no I think that a basic driver just for kbd backlight
>>>> brightness support which works with the standard desktop environment
>>>> controls for this makes sense.
>>>>
>>>> Combined with some mechanism for e.g. openrgb to fully take over
>>>> control as discussed. It is probably a good idea to file a separate
>>>> issue with the openrgb project to discuss the takeover API.
>>> I think the OpenRGB maintainers are pretty flexible at that point, after all it's similar to enable commands a lot of rgb devices need anyway. I would include it in a full api proposal.
>> Ack.
>>
>>> On this note: Any particular reason you suggested an ioctl interface instead of a sysfs one? (Open question as, for example, I have no idea what performance implications both have)
>> sysfs APIs typically have a one file per setting approach,
>> so for effects with speed and multiple-color settings you
>> would need a whole bunch of different files and then you
>> would either need to immediately apply every setting,
>> needing multiple writes to the hw for a single effect
>> update, or have some sort of "commit" sysfs attribute.
>>
>> With ioctls you can simply provide all the settings
>> in one call, which is why I suggested using ioctls.
>
> Ack
>
> If the static mode update is fast enough to have userspace controlled animations, OpenRGB is calling that direct mode. Is it feasible to send 30 or more ioctls per second for such an direct mode? Or should this spawn a special purpose sysfs file that is kept open by userspace to continuously update the keyboard?
ioctls are quite fast and another advantage of ioctls is
you open the /dev/rgbledstring# device only once and
then re-use the fd for as many ioctls as you want.
Regards,
Hans
Hi,
so I combined Hans last draft, with the discussion since then and the comments
from the OpenRGB maintainers from here
https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916 and my own experience
and came up witrh this rough updated draft for the new uapi:
Future handling of complex RGB devices on Linux:
Optional: Provide a basic leds-subsystem driver:
- The whole device is treated as a singular RGB led in the current leds uapi
- Backwards compatibility
- Have something work out-of-the-box and during boot time
- The driver also registers a misc device with a singluar sysfs attribute
select_uapi
- reading this gives back "[leds] none"
- the current active uapi can be selected by writing it to that attribute
- switching the uapi means deregistering the device from that entirely
and registering and initializing it with the new one froms scratch
- selecting none only does the deregistering
If the device is already reachable by userspace directly, e.g. via hidraw, the
kernel will only offer this basic implementation and a more complex driver has
to be implemented in userspace.
- This driver has to use the select_uapi attribute first and select "none"
to avoid undefined behaviour caused by accessing the leds upai and hidraw to
control the lighting at the same time
- Question: How to best associate the select_uapi attribute to the
corresponding hidraw (or other) direct access channel? So that a userspace
driver can reliable check whether or not this has to be set.
Devices not reachable by userspace directly, e.g. because they are controled via
a wmi interface, can also be implemented in the new rgbledstring-subsystem
(working title) for more complex control
- a rgbledstring device provides an ioctl interface (ioctl only no r/w)
using /dev/rgbledstring0, /dev/rgbledstring1, etc. registered as a misc chardev.
- get-device-info ioctl which returns in a single struct:
- char name[64] /* Device model name /
identifier, preferable human readable. For keyboards, if known to the driver,
physical layout (or even printed layout) should be separated here */
- enum device_type /* e.g. keyboard, mouse,
lightbar, etc. */
- char firmware_version_string[64] /* if known to the driver,
empty otherwise */
- char serial_number[64] /* if known to the driver,
empty otherwise */
- enum supported_modes[64] /* modes supported by the
firmware e.g. static/direct, breathing, scan, raindrops, etc. */
- get-mode-info icotl, RFC here: Hans thinks it is better to have the
modes and their inputs staticly defined and have, if required, something like
breathing_clevo_1, breathing_clevo_2, breathing_tongfang_1 if the inputs vary
between vendors. I think a dynamic approach could be useful where userspace just
queries the struct required for each individual mode.
- input: a mode from the supported_modes extracted from get-device-info
- output: static information about the mode, e.g.
max_animation_speed, max_brightness, etc.
- output: the struct/a list of attributes and their types required
to configure the mode
- set-mode ioctl takes a single struct:
- enum mode /* from supported_modes */
- union data
- char raw[3072]
- <all structs returned by get-mode-info>
- The driver also registers a singluar sysfs attribute select_uapi
- reading this gives back "[leds] rgbledstring none" or
"[rgbledstring] none" respectifly
- Discussion question: should select_uapi instead be use_leds_uapi
- if 1: use basic leds driver
- if 0: if device is userspace accessible no kernel driver is
active, if device ist not userspace accessible register rgbledstring (aka
implicit separation between rgbledstring and none instead of explicit one)
Zone configuration would be seen as a subset of mode configuration, as I suspect
not every mode needs the zone configuration even on devices that offer it?
The most simple mode would be static/direct and the set-mode struct would look
like this:
{
enum mode, /* = static */
{
uint8 brightness, /* global brightness, some keyboards offer this */
uint8 color[<number_of_leds>*3]
}
}
Question: Are there modes that have a separate setup command that is only
required once and then a continuous stream of update information? If yes, should
we reflect that by splitting set-mode into set-mode-setup and set-mode-update
(with get-mode-info returning one struct for each)? Or should userspace just
always send setup and update information and it's up to the kernel driver to
only resend the setup command when something has changed? In the former case
set-mode-update might be a noop in most cases.
Discussion on this might also happen here:
https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1751170108
Regards,
Werner
On Wed, Jan 31, 2024 at 3:42 AM Werner Sembach <wse@tuxedocomputerscom> wrote:
>
> Hi,
>
> so I combined Hans last draft, with the discussion since then and the comments
> from the OpenRGB maintainers from here
> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916 and my own experience
> and came up witrh this rough updated draft for the new uapi:
>
> Future handling of complex RGB devices on Linux:
>
> Optional: Provide a basic leds-subsystem driver:
> - The whole device is treated as a singular RGB led in the current leds uapi
> - Backwards compatibility
> - Have something work out-of-the-box and during boot time
> - The driver also registers a misc device with a singluar sysfs attribute
> select_uapi
> - reading this gives back "[leds] none"
> - the current active uapi can be selected by writing it to that attribute
> - switching the uapi means deregistering the device from that entirely
> and registering and initializing it with the new one froms scratch
> - selecting none only does the deregistering
>
> If the device is already reachable by userspace directly, e.g. via hidraw, the
> kernel will only offer this basic implementation and a more complex driver has
> to be implemented in userspace.
> - This driver has to use the select_uapi attribute first and select "none"
> to avoid undefined behaviour caused by accessing the leds upai and hidraw to
> control the lighting at the same time
> - Question: How to best associate the select_uapi attribute to the
> corresponding hidraw (or other) direct access channel? So that a userspace
> driver can reliable check whether or not this has to be set.
>
> Devices not reachable by userspace directly, e.g. because they are controled via
> a wmi interface, can also be implemented in the new rgbledstring-subsystem
> (working title) for more complex control
> - a rgbledstring device provides an ioctl interface (ioctl only no r/w)
> using /dev/rgbledstring0, /dev/rgbledstring1, etc. registered as a misc chardev.
> - get-device-info ioctl which returns in a single struct:
> - char name[64] /* Device model name /
> identifier, preferable human readable. For keyboards, if known to the driver,
> physical layout (or even printed layout) should be separated here */
> - enum device_type /* e.g. keyboard, mouse,
> lightbar, etc. */
> - char firmware_version_string[64] /* if known to the driver,
> empty otherwise */
> - char serial_number[64] /* if known to the driver,
> empty otherwise */
> - enum supported_modes[64] /* modes supported by the
> firmware e.g. static/direct, breathing, scan, raindrops, etc. */
> - get-mode-info icotl, RFC here: Hans thinks it is better to have the
> modes and their inputs staticly defined and have, if required, something like
> breathing_clevo_1, breathing_clevo_2, breathing_tongfang_1 if the inputs vary
> between vendors. I think a dynamic approach could be useful where userspace just
> queries the struct required for each individual mode.
> - input: a mode from the supported_modes extracted from get-device-info
> - output: static information about the mode, e.g.
> max_animation_speed, max_brightness, etc.
> - output: the struct/a list of attributes and their types required
> to configure the mode
> - set-mode ioctl takes a single struct:
> - enum mode /* from supported_modes */
> - union data
> - char raw[3072]
> - <all structs returned by get-mode-info>
> - The driver also registers a singluar sysfs attribute select_uapi
> - reading this gives back "[leds] rgbledstring none" or
> "[rgbledstring] none" respectifly
> - Discussion question: should select_uapi instead be use_leds_uapi
> - if 1: use basic leds driver
> - if 0: if device is userspace accessible no kernel driver is
> active, if device ist not userspace accessible register rgbledstring (aka
> implicit separation between rgbledstring and none instead of explicit one)
>
> Zone configuration would be seen as a subset of mode configuration, as I suspect
> not every mode needs the zone configuration even on devices that offer it?
>
> The most simple mode would be static/direct and the set-mode struct would look
> like this:
> {
> enum mode, /* = static */
> {
> uint8 brightness, /* global brightness, some keyboards offer this */
> uint8 color[<number_of_leds>*3]
> }
> }
>
> Question: Are there modes that have a separate setup command that is only
> required once and then a continuous stream of update information? If yes, should
> we reflect that by splitting set-mode into set-mode-setup and set-mode-update
> (with get-mode-info returning one struct for each)? Or should userspace just
> always send setup and update information and it's up to the kernel driver to
> only resend the setup command when something has changed? In the former case
> set-mode-update might be a noop in most cases.
>
> Discussion on this might also happen here:
> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1751170108
>
> Regards,
>
> Werner
>
>
Hi Werner,
I don't have a particular opinion as I don't know too much about RGB
keyboards. I just want to provide some food for thought and provide
some extra context of other devices. Just to challenge the discussion
and make sure than any API is flexible enough as it is hard to change
kernel interfaces later on.
At Sony our PlayStation controllers historically had a variety of LEDs
whether basic indicator ones (e.g. used to pick a player number) as
well as RGB leds. The devices are all HID based, but we do custom
parsing in hid-playstation to break out them out through LED framework
(regular leds and leds-class-multicolor for RGB). They were a bit of a
nightmare for applications to discover as crawling sysfs isn't fun (we
wrote a lot of code for Android's input framework to do this for our
own peripherals, but others too).
I'm not entirely sure where your RGB proposal is headed, but if one of
the higher goals is making dealing with LEDs and input devices easier,
maybe this extra info helps some of the discussion.
Thanks,
Roderick Colenbrander
Hi,
so after more feedback from the OpenRGB maintainers I came up with an even more
generic proposal:
https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
Copy pasting the relevant part:
>Another, yet more generic, approach:
>
>```
>get-device-info ioctl returning:
>{
> char name[64] /* Device model name / identifier */
> enum device_type /* e.g. keyboard, mouse, lightbar, etc. */
> char firmware_version_string[64] /* if known to the driver, empty
otherwise */
> char serial_number[64] /* if known to the driver, empty
otherwise */
> enum supported_commands[128] /* comands supported by the firmware */
>}
>
>evaluate-set-command ioctl taking:
>{
> enum command /* one of supported_commands */
> union data
> {
> char raw[3072],
> {
> <input struct for command 0>
> },
> {
> <input struct for command 1>
> },
> ...
> }
>}
>
>evaluate-get-command ioctl taking:
>{
> enum command /* one of supported_commands */
> union data
> {
> char raw[3072],
> {
> <input struct for command 0>
> },
> {
> <input struct for command 1>
> },
> ...
> }
>}
>and returning:
>{
> union data
> {
> char raw[3072],
> {
> <return struct for command 0> /* not every command might have
one */
> },
> {
> <return struct for command 1> /* not every command might have
one */
> },
> ...
> }
>}
>```
>
>- char name[64] still includes, if know to the driver, information about
physical or even printed layout.
>- differentiation between evaluate-set-command and evaluate-get-command is
mainly there for performance optimization for direct mode (for
evaluate-set-command the kernel does not have to copy anything back to userspace)
>- commands without a return struct must not be used with evaluate-get-command
>- the input struct might be empty for very simple commands (or "int unused" to
not confuse the compiler if neccessary)
>
>Now is the question: How does userspace know which commands takes/returns
which structs? Define them in one big header file (as struct
clevo_set_breathing_mode_1_input, struct tongfang_set_breathing_mode_1_input,
etc.), or somehow dynamicaly? I'm warming up to Hans suggestion to just do it
statically, unlike my suggestion yesterday.
>
>Min/Max values are documented in the header file (if not implied by variable
type). With different max value -> different command, e.g.
clevo_set_breathing_mode_1 for devices with speed from 0 to 7 and
clevo_set_breathing_mode_2 for devices with speed from 1 to 10.
But at this point it is almost a generic interface that can be used to expose
anything to userspace, looping back to the sanitized-wmiraw idea that was
floating around earlier.
So a new approach (Please correct me if there is already something similar I'm
not aware of):
New subsystem "Platform Device Commands" (short platdevcom) (I'm open for better
name suggestions):
- Registers /sys/class/platdevcom/platdevcom[0-9]* (similar to hidraw)
- Has get-device-info ioctl, evaluate-set-command ioctl, and
evaluate-get-command ioctl as described above
- device_type enum entries for rgb would be for example rgbleds_keyboard,
rgbleds_mouse, etc.
On a high level this subsystem can be used to expose any platform functionality
to userspace that doesn't fit another subsystem in a central location. This
could be for example a nearly 1 to 1 sanitized mapping to wmi calls. Or writing
a specific EC register to control OEM BIOS features like flexi charging (only
charge battery to specific percentage to extend the live).
However I am aware that this is hardly an api. So Maybe it's best to just fall
back on extending the leds subsystem with the deactivate command, and from there
just implement the few rgb devices that are not hidraw as misc devices in a per
OEM fasion without a unified api.
Hi!
> so after more feedback from the OpenRGB maintainers I came up with an even
> more generic proposal:
> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
> >evaluate-set-command ioctl taking:
> >{
> >??? enum command??? ??? ??? ??? /* one of supported_commands */
> >??? union data
> >??? {
> >??? ??? char raw[3072],
> >??? ??? {
> >??? ??? ??? <input struct for command 0>
> >??? ??? },
Yeah, so ... this is not a interface. This is a backdoor to pass
arbitrary data. That's not going to fly.
For keyboards, we don't need complete new interface; we reasonable
extensions over existing display APIs -- keyboards are clearly 2D.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
On Wed, 21 Feb 2024 23:17:52 +0100
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > so after more feedback from the OpenRGB maintainers I came up with an even
> > more generic proposal:
> > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>
> > >evaluate-set-command ioctl taking:
> > >{
> > > enum command /* one of supported_commands */
> > > union data
> > > {
> > > char raw[3072],
> > > {
> > > <input struct for command 0>
> > > },
>
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
>
> For keyboards, we don't need complete new interface; we reasonable
> extensions over existing display APIs -- keyboards are clearly 2D.
I suppose they could be seen as *a* display, but if you are referring
to DRM KMS UAPI, then no, I don't see that fitting at all:
- the "pixel grid" is not orthogonal, it's not a rectangle, and it
might not be a grid at all
- Timings and video modes? DRM KMS has always been somewhat awkward for
display devices that do not have an inherent scanout cycle and timings
totally depend on the amount of pixels updated at a time
(FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
They do work, but they are very different from the usual hardware
involved with KMS, require special consideration in userspace, and
they still are actual displays while what we're talking about here
are not.
- KMS has no concept of programmed autonomous animations, and likely
never will. They are not useful with actual displays.
- Userspace will try to light up KMS outputs automatically and extend
the traditional desktop there. This was already a problem for
head-mounted displays (HMD) where it made no sense. That was worked
around with an in-kernel list of HMDs and some KMS property quirking.
Modern KMS UAPI very much aims to be a generic UAPI that abstracts
display devices. It already breaks down a little for things like USB
displays and virtual machines (e.g. qemu, vmware, especially with
remote viewers), which I find unfortunate. With HMDs the genericity
breaks down in other ways, but I'd claim HMDs are a better fit still
than full-featured VM virtual displays (cursor plane hijacking). With
non-displays like keyboards the genericity would be completely lost, as
they won't work at all the same way as displays. You cannot even show
proper images there, only coarse light patterns *IF* you actually know
the pixel layout. But the pixel layout is(?) hardware-specific which is
the opposite of generic.
While you could dress keyboard lights etc. up with DRM KMS UAPI, the
userspace would have to be written from scratch for them, and you
somehow need to make existing KMS userspace to never touch those
devices. What's the point of using DRM KMS UAPI in the first place,
then?
Thanks,
pq
Hi,
On 2/21/24 23:17, Pavel Machek wrote:
> Hi!
>
>> so after more feedback from the OpenRGB maintainers I came up with an even
>> more generic proposal:
>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>
>>> evaluate-set-command ioctl taking:
>>> {
>>> enum command /* one of supported_commands */
>>> union data
>>> {
>>> char raw[3072],
>>> {
>>> <input struct for command 0>
>>> },
>
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
Pavel, Note the data will be interpreted by a kernel driver and
not passed directly to the hw.
With that said I tend to agree that this seems to be a bit too
generic.
Werner, it seems you are basically re-inventing ioctls here.
If you are going to use per vendor specific data structs for various
commands and have those defined in the kernel userspace API headers,
then this means that userspace will already need updated versions
of those headers to support new vendors / new laptop models if
the commands change for a new model.
So what you are basically providing here is a generic interface
to pass a cmd number + a cmd-bumber-specific data struct and
we already have that it is called an ioctl.
So I think that the conclusion of this whole discussion is that
with the exception of a get-dev-info ioctl, we simply want
vendor specific ioctls, using 1 ioctl per command.
Given that these devices are all different in various ways
and that we only want this for devices which cannot be accessed
from userspace directly (so a limit set of devices) I really
think that just doing custom ioctls per vendor is best.
This certainly is the most KISS approach. This proposal
in essence is just an arbitrary command multiplexer /
demultiplexer and ioctls already are exactly that.
With the added advantage of being able to directly use
pass the vendor-cmd-specific struct to the ioctl instead
of having to first embed it in some other struct.
Regards,
Hans
> This certainly is the most KISS approach. This proposal
> in essence is just an arbitrary command multiplexer /
> demultiplexer and ioctls already are exactly that.
>
> With the added advantage of being able to directly use
> pass the vendor-cmd-specific struct to the ioctl instead
> of having to first embed it in some other struct.
There's also the question of how much complexity needs to remain in the
kernel, if vendor-specific ioctls are made available.
Does every vendor driver implement a complex mapping to hardware
registers? What about drivers that basically implement no mapping at all
and simply forward all data to the hardware without any checking? The
latter case would match Pavel's concerns, although I don't see how this
is any different from the situation today, where userspace talks
directly to the hardware via libusb etc.
To be honest, I think the kernel shouldn't include too much high-level
complexity. If there is a desire to implement a generic display device
on top of the RGB device, this should be a configurable service running
in user space. The kernel should provide an interface to expose this
emulated display as a "real" display to applications - unless this can
also be done entirely in user space in a generic way.
Hi,
On 2/22/24 12:38, Gregor Riepl wrote:
>> This certainly is the most KISS approach. This proposal
>> in essence is just an arbitrary command multiplexer /
>> demultiplexer and ioctls already are exactly that.
>>
>> With the added advantage of being able to directly use
>> pass the vendor-cmd-specific struct to the ioctl instead
>> of having to first embed it in some other struct.
>
> There's also the question of how much complexity needs to remain in the kernel, if vendor-specific ioctls are made available.
>
> Does every vendor driver implement a complex mapping to hardware registers? What about drivers that basically implement no mapping at all and simply forward all data to the hardware without any checking? The latter case would match Pavel's concerns, although I don't see how this is any different from the situation today, where userspace talks directly to the hardware via libusb etc.
This whole discussion got started by embedded-controller driven
keyboards in laptops with per key RGB lighting. We cannot just
allow userspace raw-access to the embedded-controller.
So these per vendor ioctl commands will need to do the minimum
to make sure userspace cannot do bad things. But yes complex
stuff like figuring out which LED(s) maps to say the enter key
should be left to userspace.
Especially since this can differ per keyboardlayout.
> To be honest, I think the kernel shouldn't include too much high-level complexity. If there is a desire to implement a generic display device on top of the RGB device, this should be a configurable service running in user space. The kernel should provide an interface to expose this emulated display as a "real" display to applications - unless this can also be done entirely in user space in a generic way.
We really need to stop seeing per key addressable RGB keyboards as displays:
1. Some "pixels" are non square
2. Not all "pixels" have the same width-height ratio
3. Not all rows have the same amount of pixels
4. There are holes in the rows like between the enter key and then numpad
5. Some "pixels" have multiple LEDs beneath them. These might be addressable
per LEDs are the sub-pixels ? What about a 2 key wide backspace key vs
the 1 key wide + another key (some non US layouts) in place of the backspace?
This will be "2 pixels" in some layout and 1 pixel with maybe / maybe-not
2 subpixels where the sub-pixels may/may not be individually addressable ?
For all these reasons the display analogy really is a bit fit for these keyboards
we tried to come up with a universal coordinate system for these at the beginning
of the thread and we failed ...
Regards,
Hans
Hi,
Thanks everyone for the exhaustive feedback. And at least this thread is a good
comprehesive reference for the future ^^.
To recap the hopefully final UAPI for complex RGB lighting devices:
- By default there is a singular /sys/class/leds/* entry that treats the device
as if it was a single zone RGB keyboard backlight with no special effects.
- There is an accompanying misc device with the sysfs attributes "name",
"device_type", "firmware_version_string", "serial_number" for device
identification and "use_leds_uapi" that defaults to 1.
- If set to 0 the /sys/class/leds/* entry disappears. The driver should
keep the last state the backlight was in active if possible.
- If set 1 it appears again. The driver should bring it back to a static 1
zone setting while avoiding flicker if possible.
- If the device is not controllable by for example hidraw, the misc device might
also implement additional ioctls or sysfs attributes to allow a more complex low
level control for the keyboard backlight. This is will be a highly vendor
specific UAPI.
- The actual logic interacting with this low level UAPI is implemented by a
userspace driver
Implementation wise: For the creation of the misc device with the use_leds_uapi
switch a helper function/macro might be useful? Wonder if it should go into
leds.h, led-class-multicolor.h, or a new header file?
- Out of my head it would look something like this:
led_classdev_add_optional_misc_control(
struct led_classdev *led_cdev,
char* name,
char* device_type,
char* firmware_version_string,
char* serial_number,
void (*deregister_led)(struct led_classdev *led_cdev),
void (*reregister_led)(struct led_classdev *led_cdev))
Let me know your thoughts and hopefully I can start implementing it soon for one
of our devices.
Kind regards,
Werner Sembach
Hi!
> > Yeah, so ... this is not a interface. This is a backdoor to pass
> > arbitrary data. That's not going to fly.
>
> Pavel, Note the data will be interpreted by a kernel driver and
> not passed directly to the hw.
Yes, still not flying :-).
> With that said I tend to agree that this seems to be a bit too
> generic.
Exactly.
> Given that these devices are all different in various ways
> and that we only want this for devices which cannot be accessed
> from userspace directly (so a limit set of devices) I really
> think that just doing custom ioctls per vendor is best.
I don't think that's good idea in the long term. Kernel should provide
hardware abstraction, so that userspace does not need to know about
hardware. Obviously there are exceptions, but this should not be one
of those.
BR,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> > > so after more feedback from the OpenRGB maintainers I came up with an even
> > > more generic proposal:
> > > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
> >
> > > >evaluate-set-command ioctl taking:
> > > >{
> > > >??? enum command??? ??? ??? ??? /* one of supported_commands */
> > > >??? union data
> > > >??? {
> > > >??? ??? char raw[3072],
> > > >??? ??? {
> > > >??? ??? ??? <input struct for command 0>
> > > >??? ??? },
> >
> > Yeah, so ... this is not a interface. This is a backdoor to pass
> > arbitrary data. That's not going to fly.
> >
> > For keyboards, we don't need complete new interface; we reasonable
> > extensions over existing display APIs -- keyboards are clearly 2D.
>
> I suppose they could be seen as *a* display, but if you are referring
> to DRM KMS UAPI, then no, I don't see that fitting at all:
So -- we already have very similar displays in
drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
1-bit display for example.
> - the "pixel grid" is not orthogonal, it's not a rectangle, and it
> might not be a grid at all
It is quite close to orthogonal. I'd suggest simply pretending it is
orthogonal grid with some pixels missing :-). We already have
cellphone displays with rounded corners and holes in them, so I
suspect handling of missing pixels will be neccessary anyway.
> - Timings and video modes? DRM KMS has always been somewhat awkward for
> display devices that do not have an inherent scanout cycle and timings
> totally depend on the amount of pixels updated at a time
> (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
> They do work, but they are very different from the usual hardware
> involved with KMS, require special consideration in userspace, and
> they still are actual displays while what we're talking about here
> are not.
As you say, there are other displays with similar problems.
> - KMS has no concept of programmed autonomous animations, and likely
> never will. They are not useful with actual displays.
Yep. We need some kind of extension here, and this is likely be quite
vendor-specific, as animations will differ between vendors. I guess
"please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
> - Userspace will try to light up KMS outputs automatically and extend
> the traditional desktop there. This was already a problem for
> head-mounted displays (HMD) where it made no sense. That was worked
> around with an in-kernel list of HMDs and some KMS property
> quirking.
This will need fixing for cfag12864b.c, no? Perhaps userspace should
simply ignore anything smaller than 240x160 or something...
> Modern KMS UAPI very much aims to be a generic UAPI that abstracts
> display devices. It already breaks down a little for things like USB
> displays and virtual machines (e.g. qemu, vmware, especially with
> remote viewers), which I find unfortunate. With HMDs the genericity
> breaks down in other ways, but I'd claim HMDs are a better fit still
> than full-featured VM virtual displays (cursor plane hijacking). With
> non-displays like keyboards the genericity would be completely lost, as
> they won't work at all the same way as displays. You cannot even show
> proper images there, only coarse light patterns *IF* you actually know
> the pixel layout. But the pixel layout is(?) hardware-specific which is
> the opposite of generic.
>
> While you could dress keyboard lights etc. up with DRM KMS UAPI, the
> userspace would have to be written from scratch for them, and you
> somehow need to make existing KMS userspace to never touch those
> devices. What's the point of using DRM KMS UAPI in the first place,
> then?
Well, at least we have good UAPI to work with. Other options were 100
files in /sys/class/leds, pretending it is a linear array of leds,
just passing raw data around, and pretending it is grid of RGB888
data.
Anyway, there are devices such as these: (8x8 LED display).
https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
We should think about what interface we want for these, and then I
believe we should make RGB keyboards use something similar.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> > To be honest, I think the kernel shouldn't include too much high-level complexity. If there is a desire to implement a generic display device on top of the RGB device, this should be a configurable service running in user space. The kernel should provide an interface to expose this emulated display as a "real" display to applications - unless this can also be done entirely in user space in a generic way.
>
> We really need to stop seeing per key addressable RGB keyboards as displays:
>
> 1. Some "pixels" are non square
> 2. Not all "pixels" have the same width-height ratio
They are quite close to square usually.
> 3. Not all rows have the same amount of pixels
True for cellphone displays, too. Rounded corners.
> 4. There are holes in the rows like between the enter key and then numpad
True for cellphone displays, too. Hole for camera.
> 5. Some "pixels" have multiple LEDs beneath them. These might be addressable
> per LEDs are the sub-pixels ? What about a 2 key wide backspace key vs
> the 1 key wide + another key (some non US layouts) in place of the backspace?
> This will be "2 pixels" in some layout and 1 pixel with maybe / maybe-not
> 2 subpixels where the sub-pixels may/may not be individually addressable ?
Treat those "sub pixels" as pixels. They will be in same matrix as the rest.
> For all these reasons the display analogy really is a bit fit for these keyboards
> we tried to come up with a universal coordinate system for these at the beginning
> of the thread and we failed ...
I'd suggest trying harder this time :-).
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> For all these reasons the display analogy really is a bit fit for these keyboards
> we tried to come up with a universal coordinate system for these at the beginning
> of the thread and we failed ...
I quite liked the coordinate system proposal. I can propose this:
Vendor maps the keyboard lights to a grid. That would be something
16x8 for thinkpad X220. Then we provide functionality to query "is a
working pixel there" and "what kind of key is at this pixel" -- I
guess we can use input keycodes for that. Multiple pixels can map to
one keycode.
(And then we make best effort to map normal keyboards into similar
grids).
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
Am 21.02.24 um 23:17 schrieb Pavel Machek:
> Hi!
>
>> so after more feedback from the OpenRGB maintainers I came up with an even
>> more generic proposal:
>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>>> evaluate-set-command ioctl taking:
>>> {
>>> enum command /* one of supported_commands */
>>> union data
>>> {
>>> char raw[3072],
>>> {
>>> <input struct for command 0>
>>> },
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
>
> For keyboards, we don't need complete new interface; we reasonable
> extensions over existing display APIs -- keyboards are clearly 2D.
Maybe we should look on this from a users perspective: Running custom Animation
(i.e. where treating it as a display would be helpfull) is only >one< of the
ways a user might want to use the keyboard backlight.
Equally viable are for example:
- Having it mono colored.
- Playing a hardware effect
- Playing a hardware effect on one side of the keyboard while having the other
side of the keyboard playing a custom animation (As I learned from the OpenRGB
maintainers: There are keyboards which allow this)
There is no reason to define a custom animation as the default and then just
bolt the other options on top as it might not even be possible for some devices
where the firmware is plainly to slow for custom animations.
Also I still think a 2*2, 1*3 or even 1*1 matrix is not a display, coming back
aground to the earlier point where I want to implement this for keyboard first,
but this discussion is also thought to find a way that works for all complex RGB
devices. And I don't think it is a good idea find a way that works for keyboards
and then introduce again something completely new for other device categories.
>
> Best regards,
> Pavel
Best regards,
Werner
On Thu, 22 Feb 2024 18:38:16 +0100
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > > > so after more feedback from the OpenRGB maintainers I came up with an even
> > > > more generic proposal:
> > > > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
> > >
> > > > >evaluate-set-command ioctl taking:
> > > > >{
> > > > > enum command /* one of supported_commands */
> > > > > union data
> > > > > {
> > > > > char raw[3072],
> > > > > {
> > > > > <input struct for command 0>
> > > > > },
> > >
> > > Yeah, so ... this is not a interface. This is a backdoor to pass
> > > arbitrary data. That's not going to fly.
> > >
> > > For keyboards, we don't need complete new interface; we reasonable
> > > extensions over existing display APIs -- keyboards are clearly 2D.
> >
> > I suppose they could be seen as *a* display, but if you are referring
> > to DRM KMS UAPI, then no, I don't see that fitting at all:
>
> So -- we already have very similar displays in
> drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
> 1-bit display for example.
Auxdisplay are not exposed as DRM KMS device, or are they?
I don't see cfag12864b.c using any DRM calls.
DRM drivers are under drivers/gpu/drm.
I know nothing about auxdisplay. If it's fbdev UAPI, then I don't know -
people have been trying to kill it for years, and basing anything on it
seems like a dead idea. Or maybe it's ok for extremely small displays
where there is no display controller to speak of, and definitely no use
ever for dmabuf? Where CPU banging bits of raw pixels is enough, and no
desktop would ever want to touch them.
But I'm not talking about fbdev either, I'm talking about DRM KMS.
>
> > - the "pixel grid" is not orthogonal, it's not a rectangle, and it
> > might not be a grid at all
>
> It is quite close to orthogonal. I'd suggest simply pretending it is
> orthogonal grid with some pixels missing :-). We already have
> cellphone displays with rounded corners and holes in them, so I
> suspect handling of missing pixels will be neccessary anyway.
Yes, but I do not agree on the similarity at all, nor that it would be
"close to orthogonal" by simply looking at my keyboard. Hans de Goede
iterated on this much better than I.
> > - Timings and video modes? DRM KMS has always been somewhat awkward for
> > display devices that do not have an inherent scanout cycle and timings
> > totally depend on the amount of pixels updated at a time
> > (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
> > They do work, but they are very different from the usual hardware
> > involved with KMS, require special consideration in userspace, and
> > they still are actual displays while what we're talking about here
> > are not.
>
> As you say, there are other displays with similar problems.
That's no justification to add even more problems.
> > - KMS has no concept of programmed autonomous animations, and likely
> > never will. They are not useful with actual displays.
>
> Yep. We need some kind of extension here, and this is likely be quite
> vendor-specific, as animations will differ between vendors. I guess
> "please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
Right. I very much believe that is not going to fly in DRM KMS.
> > - Userspace will try to light up KMS outputs automatically and extend
> > the traditional desktop there. This was already a problem for
> > head-mounted displays (HMD) where it made no sense. That was worked
> > around with an in-kernel list of HMDs and some KMS property
> > quirking.
>
> This will need fixing for cfag12864b.c, no? Perhaps userspace should
> simply ignore anything smaller than 240x160 or something...
Yes, a size limit would make sense in desktop usage.
> > Modern KMS UAPI very much aims to be a generic UAPI that abstracts
> > display devices. It already breaks down a little for things like USB
> > displays and virtual machines (e.g. qemu, vmware, especially with
> > remote viewers), which I find unfortunate. With HMDs the genericity
> > breaks down in other ways, but I'd claim HMDs are a better fit still
> > than full-featured VM virtual displays (cursor plane hijacking). With
> > non-displays like keyboards the genericity would be completely lost, as
> > they won't work at all the same way as displays. You cannot even show
> > proper images there, only coarse light patterns *IF* you actually know
> > the pixel layout. But the pixel layout is(?) hardware-specific which is
> > the opposite of generic.
> >
> > While you could dress keyboard lights etc. up with DRM KMS UAPI, the
> > userspace would have to be written from scratch for them, and you
> > somehow need to make existing KMS userspace to never touch those
> > devices. What's the point of using DRM KMS UAPI in the first place,
> > then?
>
> Well, at least we have good UAPI to work with.
.Where the great majority of that UAPI is ill-fitting for the device,
and requires userspace to use existing UAPI in completely new ways. KMS
UAPI is also very complex to use, because the devices it is meant for
are complex, timing sensitive, and capable of image processing.
> Other options were 100
> files in /sys/class/leds, pretending it is a linear array of leds,
> just passing raw data around, and pretending it is grid of RGB888
> data.
>
> Anyway, there are devices such as these: (8x8 LED display).
>
> https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
>
> We should think about what interface we want for these, and then I
> believe we should make RGB keyboards use something similar.
Unfortunately I cannot say anything about any other UAPI options. I know
nothing of any of them. My comments come from being a Weston developer,
a Wayland compositor for both desktop'ish and embedded use cases that
uses DRM KMS.
Thanks,
pq
Hi
Am 22.02.24 um 18:38 schrieb Pavel Machek:
> Hi!
>
>>>> so after more feedback from the OpenRGB maintainers I came up with an even
>>>> more generic proposal:
>>>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>>>>> evaluate-set-command ioctl taking:
>>>>> {
>>>>> enum command /* one of supported_commands */
>>>>> union data
>>>>> {
>>>>> char raw[3072],
>>>>> {
>>>>> <input struct for command 0>
>>>>> },
>>> Yeah, so ... this is not a interface. This is a backdoor to pass
>>> arbitrary data. That's not going to fly.
>>>
>>> For keyboards, we don't need complete new interface; we reasonable
>>> extensions over existing display APIs -- keyboards are clearly 2D.
>> I suppose they could be seen as *a* display, but if you are referring
>> to DRM KMS UAPI, then no, I don't see that fitting at all:
> So -- we already have very similar displays in
> drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
> 1-bit display for example.
Auxdisplay can be anything for everyone. It appears to be the rest that
didn't clearly fit elsewhere. The core interface seems to be a custom
char device. The fbdev code in cfag12864b is not representative.
>
>> - the "pixel grid" is not orthogonal, it's not a rectangle, and it
>> might not be a grid at all
> It is quite close to orthogonal. I'd suggest simply pretending it is
> orthogonal grid with some pixels missing :-). We already have
> cellphone displays with rounded corners and holes in them, so I
> suspect handling of missing pixels will be neccessary anyway.
>
>> - Timings and video modes? DRM KMS has always been somewhat awkward for
>> display devices that do not have an inherent scanout cycle and timings
>> totally depend on the amount of pixels updated at a time
>> (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
>> They do work, but they are very different from the usual hardware
>> involved with KMS, require special consideration in userspace, and
>> they still are actual displays while what we're talking about here
>> are not.
> As you say, there are other displays with similar problems.
>
>> - KMS has no concept of programmed autonomous animations, and likely
>> never will. They are not useful with actual displays.
> Yep. We need some kind of extension here, and this is likely be quite
> vendor-specific, as animations will differ between vendors. I guess
> "please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
The litmus test for DRM and fbdev is something like "would the user run
the console, desktop, or any other meaningful output in this display".
That is also what userspace (e.g., X, Wayland, gfx terminals) expects: a
display to show the user's main output. Keyboard LEDs don't fit here.
Best regards
Thomas
>
>> - Userspace will try to light up KMS outputs automatically and extend
>> the traditional desktop there. This was already a problem for
>> head-mounted displays (HMD) where it made no sense. That was worked
>> around with an in-kernel list of HMDs and some KMS property
>> quirking.
> This will need fixing for cfag12864b.c, no? Perhaps userspace should
> simply ignore anything smaller than 240x160 or something...
>
>> Modern KMS UAPI very much aims to be a generic UAPI that abstracts
>> display devices. It already breaks down a little for things like USB
>> displays and virtual machines (e.g. qemu, vmware, especially with
>> remote viewers), which I find unfortunate. With HMDs the genericity
>> breaks down in other ways, but I'd claim HMDs are a better fit still
>> than full-featured VM virtual displays (cursor plane hijacking). With
>> non-displays like keyboards the genericity would be completely lost, as
>> they won't work at all the same way as displays. You cannot even show
>> proper images there, only coarse light patterns *IF* you actually know
>> the pixel layout. But the pixel layout is(?) hardware-specific which is
>> the opposite of generic.
>>
>> While you could dress keyboard lights etc. up with DRM KMS UAPI, the
>> userspace would have to be written from scratch for them, and you
>> somehow need to make existing KMS userspace to never touch those
>> devices. What's the point of using DRM KMS UAPI in the first place,
>> then?
> Well, at least we have good UAPI to work with. Other options were 100
> files in /sys/class/leds, pretending it is a linear array of leds,
> just passing raw data around, and pretending it is grid of RGB888
> data.
>
> Anyway, there are devices such as these: (8x8 LED display).
>
> https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
>
> We should think about what interface we want for these, and then I
> believe we should make RGB keyboards use something similar.
>
> Best regards,
> Pavel
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Hi Werner,
Sorry for the late reply.
On 2/22/24 2:14 PM, Werner Sembach wrote:
> Hi,
>
> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>
> To recap the hopefully final UAPI for complex RGB lighting devices:
>
> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
Ack this sounds good.
>
> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it.
Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before
and makes sense. But at least for HID devices the rest of this info is already available
in sysfs attributes on the HID devices (things like vendor and product id) and since the
userspace code needs per device code to drive the kbd anyways it can also have device
specific code to retrieve all this info, so the other sysfs attributes just feel like
duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd
drivers which already get this info from other places.
> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>
> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
Ack, if this finds it way into some documentation (which it should) please make it
clear that this is about the "use_leds_uapi" sysfs attribute.
> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
IMHO this is the only case where actually using a misc device makes sense, so that
you have a chardev to do the ioctls on. misc-device-s should really only be used
when you need a chardev under /dev . Since you don't need the chardev for the e.g.
hidraw case you should not use a miscdev there IMHO.
>
> - The actual logic interacting with this low level UAPI is implemented by a userspace driver
>
> Implementation wise: For the creation of the misc device with the use_leds_uapi switch a helper function/macro might be useful? Wonder if it should go into leds.h, led-class-multicolor.h, or a new header file?
See above, I don't think we want the misc device for the hidraw case, at which
point I think the helper becomes unnecessary since just a single sysfs write
callback is necessary.
Also for adding new sysfs attributes it is strongly encouraged to use
device_driver.dev_groups so that the device core handled registering /
unregistering the sysfs attributes which fixes a bunch of races; and
using device_driver.dev_groups does not mix well with a helper as you've
suggested.
>
> - Out of my head it would look something like this:
>
> led_classdev_add_optional_misc_control(
> struct led_classdev *led_cdev,
> char* name,
> char* device_type,
> char* firmware_version_string,
> char* serial_number,
> void (*deregister_led)(struct led_classdev *led_cdev),
> void (*reregister_led)(struct led_classdev *led_cdev))
>
> Let me know your thoughts and hopefully I can start implementing it soon for one of our devices.
I think overall the plan sounds good, with my main suggested change
being to not use an unnecessary misc device for the hid-raw case.
Regards,
Hans
Hi Hans,
Am 18.03.24 um 12:11 schrieb Hans de Goede:
> Hi Werner,
>
> Sorry for the late reply.
np
>
> On 2/22/24 2:14 PM, Werner Sembach wrote:
>> Hi,
>>
>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>
>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>
>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
> Ack this sounds good.
>
>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
> You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it.
>
> Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before
> and makes sense. But at least for HID devices the rest of this info is already available
> in sysfs attributes on the HID devices (things like vendor and product id) and since the
> userspace code needs per device code to drive the kbd anyways it can also have device
> specific code to retrieve all this info, so the other sysfs attributes just feel like
> duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd
> drivers which already get this info from other places.
The parent device can be a hid device, a wmi device, a platform device etc. so I
thought it would be nice to have some unified way for identification.
>
>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>
>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
> Ack, if this finds it way into some documentation (which it should) please make it
> clear that this is about the "use_leds_uapi" sysfs attribute.
Ack
>
>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
> IMHO this is the only case where actually using a misc device makes sense, so that
> you have a chardev to do the ioctls on. misc-device-s should really only be used
> when you need a chardev under /dev . Since you don't need the chardev for the e.g.
> hidraw case you should not use a miscdev there IMHO.
My train of thought was that it would be nice to have the use_leds_uapi at a
somewhat unified location in sysfs. The parent device can be of any kind, like I
mentioned above, and while for deactivating it can easily be found via
/sys/class/leds/*/device/. For reactivating, the leds entry doesn't exist.
>
>> - The actual logic interacting with this low level UAPI is implemented by a userspace driver
>>
>> Implementation wise: For the creation of the misc device with the use_leds_uapi switch a helper function/macro might be useful? Wonder if it should go into leds.h, led-class-multicolor.h, or a new header file?
> See above, I don't think we want the misc device for the hidraw case, at which
> point I think the helper becomes unnecessary since just a single sysfs write
> callback is necessary.
>
> Also for adding new sysfs attributes it is strongly encouraged to use
> device_driver.dev_groups so that the device core handled registering /
> unregistering the sysfs attributes which fixes a bunch of races; and
> using device_driver.dev_groups does not mix well with a helper as you've
> suggested.
Ok, I will see when I start implementing.
>
>> - Out of my head it would look something like this:
>>
>> led_classdev_add_optional_misc_control(
>> struct led_classdev *led_cdev,
>> char* name,
>> char* device_type,
>> char* firmware_version_string,
>> char* serial_number,
>> void (*deregister_led)(struct led_classdev *led_cdev),
>> void (*reregister_led)(struct led_classdev *led_cdev))
>>
>> Let me know your thoughts and hopefully I can start implementing it soon for one of our devices.
> I think overall the plan sounds good, with my main suggested change
> being to not use an unnecessary misc device for the hid-raw case.
>
> Regards,
>
> Hans
>
>
Thanks for the feedback,
Werner
Hi Hans and the others,
Am 22.02.24 um 14:14 schrieb Werner Sembach:
> Hi,
>
> Thanks everyone for the exhaustive feedback. And at least this thread is a
> good comprehesive reference for the future ^^.
>
> To recap the hopefully final UAPI for complex RGB lighting devices:
>
> - By default there is a singular /sys/class/leds/* entry that treats the
> device as if it was a single zone RGB keyboard backlight with no special effects.
>
> - There is an accompanying misc device with the sysfs attributes "name",
> "device_type", "firmware_version_string", "serial_number" for device
> identification and "use_leds_uapi" that defaults to 1.
>
> - If set to 0 the /sys/class/leds/* entry disappears. The driver should
> keep the last state the backlight was in active if possible.
>
> - If set 1 it appears again. The driver should bring it back to a static 1
> zone setting while avoiding flicker if possible.
>
> - If the device is not controllable by for example hidraw, the misc device
> might also implement additional ioctls or sysfs attributes to allow a more
> complex low level control for the keyboard backlight. This is will be a highly
> vendor specific UAPI.
So in the OpenRGB issue thread
https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices
aka HID LampArray was mentioned. I did dismiss it because I thought that is only
relevant for firmware, but I now stumbled upon the Virtual HID Framework (VHF)
https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf-
and now I wonder if an equivalent exists for Linux? A quick search did not yield
any results for me.
If a virtual HID device is possible and the WMI interface can reasonably be
mapped to the LampArray API this might be the best starting point:
- Implement a Virtual HID device with LampArray
- Implement LampArray in OpenRGB
- (Optional) Implement a generic LampArray leds subsystem driver that maps to
the single zone control and ads the use_leds_uapi sysfs switch to the virtual
HID device
- (Optional) Implement vendor specific controls for
AutonomousMode/built-in-firmware-effects via custom HID commands
- (Optional) Implement Virtual HID devices for actual HID devices that don't
support LampArray in firmware (Open question: How to prevent userspace/OpenRGB
from interacting with original HID when the virtual HID device is not in
AutonomousMode? How to associate the original and virtual HID device to each
other that userspace can easily recognize this relation? Or is it possible to
add virtual HID commands on top of a real HID device, making it look exactly
like the pure virtual devices for userspace?)
The LampArray API hereby is made with the intention to be used for multi leds
devices, like per-key-backlight keyboards, unlike the leds UAPI. And it is
coming anyway with new RGB devices soon. So it would not conflict with a "don't
introduce unnecessary UAPI interfaces" principle. Are there any plans already of
Wrapping LampArray in some kind ioctl/sysfs API? Or just have it used via
hidraw? Or was there no discussion about it till now?
Regards,
Werner
>
> - The actual logic interacting with this low level UAPI is implemented by
> a userspace driver
>
> Implementation wise: For the creation of the misc device with the
> use_leds_uapi switch a helper function/macro might be useful? Wonder if it
> should go into leds.h, led-class-multicolor.h, or a new header file?
>
> - Out of my head it would look something like this:
>
> led_classdev_add_optional_misc_control(
> struct led_classdev *led_cdev,
> char* name,
> char* device_type,
> char* firmware_version_string,
> char* serial_number,
> void (*deregister_led)(struct led_classdev *led_cdev),
> void (*reregister_led)(struct led_classdev *led_cdev))
>
> Let me know your thoughts and hopefully I can start implementing it soon for
> one of our devices.
>
> Kind regards,
>
> Werner Sembach
>
Am 20.03.24 um 12:16 schrieb Werner Sembach:
> Hi Hans and the others,
>
> Am 22.02.24 um 14:14 schrieb Werner Sembach:
>> Hi,
>>
>> Thanks everyone for the exhaustive feedback. And at least this thread is a
>> good comprehesive reference for the future ^^.
>>
>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>
>> - By default there is a singular /sys/class/leds/* entry that treats the
>> device as if it was a single zone RGB keyboard backlight with no special
>> effects.
>>
>> - There is an accompanying misc device with the sysfs attributes "name",
>> "device_type", "firmware_version_string", "serial_number" for device
>> identification and "use_leds_uapi" that defaults to 1.
>>
>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should
>> keep the last state the backlight was in active if possible.
>>
>> - If set 1 it appears again. The driver should bring it back to a static
>> 1 zone setting while avoiding flicker if possible.
>>
>> - If the device is not controllable by for example hidraw, the misc device
>> might also implement additional ioctls or sysfs attributes to allow a more
>> complex low level control for the keyboard backlight. This is will be a
>> highly vendor specific UAPI.
> So in the OpenRGB issue thread
> https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices
> aka HID LampArray was mentioned. I did dismiss it because I thought that is
> only relevant for firmware, but I now stumbled upon the Virtual HID Framework
> (VHF)
> https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf-
> and now I wonder if an equivalent exists for Linux? A quick search did not
> yield any results for me.
Is this what I have been searching for? https://docs.kernel.org/usb/gadget_hid.html
>
> If a virtual HID device is possible and the WMI interface can reasonably be
> mapped to the LampArray API this might be the best starting point:
>
> - Implement a Virtual HID device with LampArray
>
> - Implement LampArray in OpenRGB
>
> - (Optional) Implement a generic LampArray leds subsystem driver that maps to
> the single zone control and ads the use_leds_uapi sysfs switch to the virtual
> HID device
>
> - (Optional) Implement vendor specific controls for
> AutonomousMode/built-in-firmware-effects via custom HID commands
>
> - (Optional) Implement Virtual HID devices for actual HID devices that don't
> support LampArray in firmware (Open question: How to prevent userspace/OpenRGB
> from interacting with original HID when the virtual HID device is not in
> AutonomousMode? How to associate the original and virtual HID device to each
> other that userspace can easily recognize this relation? Or is it possible to
> add virtual HID commands on top of a real HID device, making it look exactly
> like the pure virtual devices for userspace?)
>
> The LampArray API hereby is made with the intention to be used for multi leds
> devices, like per-key-backlight keyboards, unlike the leds UAPI. And it is
> coming anyway with new RGB devices soon. So it would not conflict with a
> "don't introduce unnecessary UAPI interfaces" principle. Are there any plans
> already of Wrapping LampArray in some kind ioctl/sysfs API? Or just have it
> used via hidraw? Or was there no discussion about it till now?
>
> Regards,
>
> Werner
>
>>
>> - The actual logic interacting with this low level UAPI is implemented by
>> a userspace driver
>>
>> Implementation wise: For the creation of the misc device with the
>> use_leds_uapi switch a helper function/macro might be useful? Wonder if it
>> should go into leds.h, led-class-multicolor.h, or a new header file?
>>
>> - Out of my head it would look something like this:
>>
>> led_classdev_add_optional_misc_control(
>> struct led_classdev *led_cdev,
>> char* name,
>> char* device_type,
>> char* firmware_version_string,
>> char* serial_number,
>> void (*deregister_led)(struct led_classdev *led_cdev),
>> void (*reregister_led)(struct led_classdev *led_cdev))
>>
>> Let me know your thoughts and hopefully I can start implementing it soon for
>> one of our devices.
>>
>> Kind regards,
>>
>> Werner Sembach
>>
Am 20.03.24 um 12:33 schrieb Werner Sembach:
>
> Am 20.03.24 um 12:16 schrieb Werner Sembach:
>> Hi Hans and the others,
>>
>> Am 22.02.24 um 14:14 schrieb Werner Sembach:
>>> Hi,
>>>
>>> Thanks everyone for the exhaustive feedback. And at least this thread is a
>>> good comprehesive reference for the future ^^.
>>>
>>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>>
>>> - By default there is a singular /sys/class/leds/* entry that treats the
>>> device as if it was a single zone RGB keyboard backlight with no special
>>> effects.
>>>
>>> - There is an accompanying misc device with the sysfs attributes "name",
>>> "device_type", "firmware_version_string", "serial_number" for device
>>> identification and "use_leds_uapi" that defaults to 1.
>>>
>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should
>>> keep the last state the backlight was in active if possible.
>>>
>>> - If set 1 it appears again. The driver should bring it back to a static
>>> 1 zone setting while avoiding flicker if possible.
>>>
>>> - If the device is not controllable by for example hidraw, the misc device
>>> might also implement additional ioctls or sysfs attributes to allow a more
>>> complex low level control for the keyboard backlight. This is will be a
>>> highly vendor specific UAPI.
>> So in the OpenRGB issue thread
>> https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices
>> aka HID LampArray was mentioned. I did dismiss it because I thought that is
>> only relevant for firmware, but I now stumbled upon the Virtual HID Framework
>> (VHF)
>> https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf-
>> and now I wonder if an equivalent exists for Linux? A quick search did not
>> yield any results for me.
> Is this what I have been searching for?
> https://docs.kernel.org/usb/gadget_hid.html
Nope is something different: http://www.linux-usb.org/gadget/
>>
>> If a virtual HID device is possible and the WMI interface can reasonably be
>> mapped to the LampArray API this might be the best starting point:
>>
>> - Implement a Virtual HID device with LampArray
>>
>> - Implement LampArray in OpenRGB
>>
>> - (Optional) Implement a generic LampArray leds subsystem driver that maps to
>> the single zone control and ads the use_leds_uapi sysfs switch to the virtual
>> HID device
>>
>> - (Optional) Implement vendor specific controls for
>> AutonomousMode/built-in-firmware-effects via custom HID commands
>>
>> - (Optional) Implement Virtual HID devices for actual HID devices that don't
>> support LampArray in firmware (Open question: How to prevent
>> userspace/OpenRGB from interacting with original HID when the virtual HID
>> device is not in AutonomousMode? How to associate the original and virtual
>> HID device to each other that userspace can easily recognize this relation?
>> Or is it possible to add virtual HID commands on top of a real HID device,
>> making it look exactly like the pure virtual devices for userspace?)
>>
>> The LampArray API hereby is made with the intention to be used for multi leds
>> devices, like per-key-backlight keyboards, unlike the leds UAPI. And it is
>> coming anyway with new RGB devices soon. So it would not conflict with a
>> "don't introduce unnecessary UAPI interfaces" principle. Are there any plans
>> already of Wrapping LampArray in some kind ioctl/sysfs API? Or just have it
>> used via hidraw? Or was there no discussion about it till now?
>>
>> Regards,
>>
>> Werner
>>
>>>
>>> - The actual logic interacting with this low level UAPI is implemented
>>> by a userspace driver
>>>
>>> Implementation wise: For the creation of the misc device with the
>>> use_leds_uapi switch a helper function/macro might be useful? Wonder if it
>>> should go into leds.h, led-class-multicolor.h, or a new header file?
>>>
>>> - Out of my head it would look something like this:
>>>
>>> led_classdev_add_optional_misc_control(
>>> struct led_classdev *led_cdev,
>>> char* name,
>>> char* device_type,
>>> char* firmware_version_string,
>>> char* serial_number,
>>> void (*deregister_led)(struct led_classdev *led_cdev),
>>> void (*reregister_led)(struct led_classdev *led_cdev))
>>>
>>> Let me know your thoughts and hopefully I can start implementing it soon for
>>> one of our devices.
>>>
>>> Kind regards,
>>>
>>> Werner Sembach
>>>
+Cc: Bentiss, Jiri
Hi Werner,
On 3/20/24 12:16 PM, Werner Sembach wrote:
> Hi Hans and the others,
>
> Am 22.02.24 um 14:14 schrieb Werner Sembach:
>> Hi,
>>
>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>
>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>
>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
>>
>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
>>
>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>
>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
>>
>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
> So in the OpenRGB issue thread https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices aka HID LampArray was mentioned. I did dismiss it because I thought that is only relevant for firmware, but I now stumbled upon the Virtual HID Framework (VHF) https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- and now I wonder if an equivalent exists for Linux? A quick search did not yield any results for me.
Oh, interesting. I did not know about the HID LampArray API.
About your question about virtual HID devices, there is uHID,
but as the name suggests this allows userspace to emulate a HID
device.
In your case you want to do the emulation in kernel so that you
can translate the proprietary WMI calls to something HID LampArray
compatible.
I guess you could do this by defining your own HID transport driver,
like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID
"client" for each device which talks HID over i2c in the machine.
Bentiss, Jiri, do you have any input on this. Would something like
that be acceptable to you (just based on the concept at least) ?
Regards,
Hans
On Mar 25 2024, Hans de Goede wrote:
> +Cc: Bentiss, Jiri
>
> Hi Werner,
>
> On 3/20/24 12:16 PM, Werner Sembach wrote:
> > Hi Hans and the others,
> >
> > Am 22.02.24 um 14:14 schrieb Werner Sembach:
> >> Hi,
> >>
> >> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
> >>
> >> To recap the hopefully final UAPI for complex RGB lighting devices:
> >>
> >> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
> >>
> >> - There is an accompanying misc device with the sysfs attributes "name", "device_type",? "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
> >>
> >> ??? - If set to 0 the?/sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
> >>
> >> ??? - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
> >>
> >> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
> > So in the OpenRGB issue thread https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices aka HID LampArray was mentioned. I did dismiss it because I thought that is only relevant for firmware, but I now stumbled upon the Virtual HID Framework (VHF) https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- and now I wonder if an equivalent exists for Linux? A quick search did not yield any results for me.
>
> Oh, interesting. I did not know about the HID LampArray API.
>
> About your question about virtual HID devices, there is uHID,
> but as the name suggests this allows userspace to emulate a HID
> device.
>
> In your case you want to do the emulation in kernel so that you
> can translate the proprietary WMI calls to something HID LampArray
> compatible.
>
> I guess you could do this by defining your own HID transport driver,
> like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID
> "client" for each device which talks HID over i2c in the machine.
>
> Bentiss, Jiri, do you have any input on this. Would something like
> that be acceptable to you (just based on the concept at least) ?
I just read the thread, and I think I now understand a little bit what
this request is :)
IMO working with the HID LampArray is the way forward. So I would
suggest to convert any non-HID RGB "LED display" that we are talking
about as a HID LampArray device through `hid_allocate_device()` in the
kernel. Basically what you are suggesting Hans. It's just that you don't
need a formal transport layer, just a child device that happens to be
HID.
The next question IMO is: do we want the kernel to handle such
machinery? Wouldn't it be simpler to just export the HID device and let
userspace talk to it through hidraw, like what OpenRGB does?
If the kernel already handles the custom protocol into generic HID, the
work for userspace is not too hard because they can deal with a known
protocol and can be cross-platform in their implementation.
I'm mentioning that cross-platform because SDL used to rely on the
input, LEDs, and other Linux peculiarities and eventually fell back on
using hidraw only because it's way more easier that way.
The other advantage of LampArray is that according to Microsoft's
document, new devices are going to support it out of the box, so they'll
be supported out of the box directly.
Most of the time my stance is "do not add new kernel API, you'll regret
it later". So in that case, given that we have a formally approved
standard, I would suggest to use it, and consider it your API.
Side note to self: I really need to resurrect the hidraw revoke series
so we could export those hidraw node to userspace with uaccess through
logind...
Cheers,
Benjamin
Hi Benjamin,
Am 25.03.24 um 16:56 schrieb Benjamin Tissoires:
> On Mar 25 2024, Hans de Goede wrote:
>> +Cc: Bentiss, Jiri
>>
>> Hi Werner,
>>
>> On 3/20/24 12:16 PM, Werner Sembach wrote:
>>> Hi Hans and the others,
>>>
>>> Am 22.02.24 um 14:14 schrieb Werner Sembach:
>>>> Hi,
>>>>
>>>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>>>
>>>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>>>
>>>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
>>>>
>>>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
>>>>
>>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>>>
>>>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
>>>>
>>>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
>>> So in the OpenRGB issue thread https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices aka HID LampArray was mentioned. I did dismiss it because I thought that is only relevant for firmware, but I now stumbled upon the Virtual HID Framework (VHF) https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- and now I wonder if an equivalent exists for Linux? A quick search did not yield any results for me.
>> Oh, interesting. I did not know about the HID LampArray API.
>>
>> About your question about virtual HID devices, there is uHID,
>> but as the name suggests this allows userspace to emulate a HID
>> device.
>>
>> In your case you want to do the emulation in kernel so that you
>> can translate the proprietary WMI calls to something HID LampArray
>> compatible.
>>
>> I guess you could do this by defining your own HID transport driver,
>> like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID
>> "client" for each device which talks HID over i2c in the machine.
>>
>> Bentiss, Jiri, do you have any input on this. Would something like
>> that be acceptable to you (just based on the concept at least) ?
> I just read the thread, and I think I now understand a little bit what
> this request is :)
>
> IMO working with the HID LampArray is the way forward. So I would
> suggest to convert any non-HID RGB "LED display" that we are talking
> about as a HID LampArray device through `hid_allocate_device()` in the
> kernel. Basically what you are suggesting Hans. It's just that you don't
> need a formal transport layer, just a child device that happens to be
> HID.
>
> The next question IMO is: do we want the kernel to handle such
> machinery? Wouldn't it be simpler to just export the HID device and let
> userspace talk to it through hidraw, like what OpenRGB does?
That's already part of my plan: The kernels main goal is to give devices a
LampArray interface that don't have one already (e.g. because they are no HID
devices to begin with).
The actual handling of LampArray will happen in userspace.
Exception is that maybe it could be useful to implement a small subset of
LampArray in a generic leds-subsystem driver for backwards compatibility to
userspace applications that only implement that (e.g. UPower). It would treat
the whole keyboard as a single led.
>
> If the kernel already handles the custom protocol into generic HID, the
> work for userspace is not too hard because they can deal with a known
> protocol and can be cross-platform in their implementation.
>
> I'm mentioning that cross-platform because SDL used to rely on the
> input, LEDs, and other Linux peculiarities and eventually fell back on
> using hidraw only because it's way more easier that way.
>
> The other advantage of LampArray is that according to Microsoft's
> document, new devices are going to support it out of the box, so they'll
> be supported out of the box directly.
>
> Most of the time my stance is "do not add new kernel API, you'll regret
> it later". So in that case, given that we have a formally approved
> standard, I would suggest to use it, and consider it your API.
The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards
compatibility.
The control flow for the whole system would look something like this:
- System boots
- Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects,
sets brightness to a default value, or initializes a solid color)
- systemd-backlight restores last keyboard backlight brightness
- UPower sees sysfs leds entry and exposes it to DBus for DEs to do
keyboard brightness handling
- If the user wants more control they (auto-)start OpenRGB
- OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double
control of the same device by UPower
- OpenRGB directly interacts with hidraw device via LampArray API to give
fine granular control of the backlight
- When OpenRGB closes it should reenable the sysfs leds entry
- System shutdown
- Since OpenRGB reenables the sysfs leds entry, systemd-backlight can
correctly store a brightness value for next boot
Regards,
Werner
>
> Side note to self: I really need to resurrect the hidraw revoke series
> so we could export those hidraw node to userspace with uaccess through
> logind...
>
> Cheers,
> Benjamin
Hi Werner,
On 3/19/24 4:18 PM, Werner Sembach wrote:
> Hi Hans,
>
> Am 18.03.24 um 12:11 schrieb Hans de Goede:
>> Hi Werner,
>>
>> Sorry for the late reply.
> np
>>
>> On 2/22/24 2:14 PM, Werner Sembach wrote:
>>> Hi,
>>>
>>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>>
>>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>>
>>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
>> Ack this sounds good.
>>
>>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
>> You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it.
>>
>> Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before
>> and makes sense. But at least for HID devices the rest of this info is already available
>> in sysfs attributes on the HID devices (things like vendor and product id) and since the
>> userspace code needs per device code to drive the kbd anyways it can also have device
>> specific code to retrieve all this info, so the other sysfs attributes just feel like
>> duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd
>> drivers which already get this info from other places.
> The parent device can be a hid device, a wmi device, a platform device etc. so I thought it would be nice to have some unified way for identification.
I think that some shared ioctl for identifying devices which need a misc-device
makes sense. But for existing hidraw supported keyboards there already is existing
userspace support, which already retreives this in a hidraw specific manner.
And I doubt that the existing userspace projects will add support for a new method
which is only available on new kernels, while they will also need to keep the old
method around to keep things working with older kernels.
So at least for the hidraw case I see little value in exporting the same info
in another way.
>>
>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>>
>>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
>> Ack, if this finds it way into some documentation (which it should) please make it
>> clear that this is about the "use_leds_uapi" sysfs attribute.
> Ack
>>
>>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
>> IMHO this is the only case where actually using a misc device makes sense, so that
>> you have a chardev to do the ioctls on. misc-device-s should really only be used
>> when you need a chardev under /dev . Since you don't need the chardev for the e.g.
>> hidraw case you should not use a miscdev there IMHO.
>
> My train of thought was that it would be nice to have the use_leds_uapi at a somewhat unified location in sysfs. The parent device can be of any kind, like I mentioned above, and while for deactivating it can easily be found via /sys/class/leds/*/device/. For reactivating, the leds entry doesn't exist.
That is an interesting point. But I would expect any process/daemon which wants to
reactivate the in kernel LED class support to be the same process as which
deactivated it. So that daemon can simply canonicalize the /sys/class/leds/*/device
symlink or canonicalize the entire /sys/class/leds/*/device/use_leds_uapi path
and store the canonicalized version for when the daemon wants to reactivate things.
So I still think that having the miscdevice just for enumeration and
use_leds_uapi purposes is overkill.
Regards,
Hans
On Mon, Mar 25, 2024 at 3:25 PM Hans de Goede <[email protected]> wrote:
>
> +Cc: Bentiss, Jiri
Cc'ing Andy and Geert as well who recently became the
maintainers/reviewers of auxdisplay, in case they are interested in
these threads (one of the initial solutions discussed in a past thread
a while ago was to extend auxdisplay).
Cheers,
Miguel
Hi Hans,
Am 25.03.24 um 15:18 schrieb Hans de Goede:
> Hi Werner,
>
> On 3/19/24 4:18 PM, Werner Sembach wrote:
>> Hi Hans,
>>
>> Am 18.03.24 um 12:11 schrieb Hans de Goede:
>>> Hi Werner,
>>>
>>> Sorry for the late reply.
>> np
>>> On 2/22/24 2:14 PM, Werner Sembach wrote:
>>>> Hi,
>>>>
>>>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>>>
>>>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>>>
>>>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
>>> Ack this sounds good.
>>>
>>>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
>>> You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it.
>>>
>>> Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before
>>> and makes sense. But at least for HID devices the rest of this info is already available
>>> in sysfs attributes on the HID devices (things like vendor and product id) and since the
>>> userspace code needs per device code to drive the kbd anyways it can also have device
>>> specific code to retrieve all this info, so the other sysfs attributes just feel like
>>> duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd
>>> drivers which already get this info from other places.
>> The parent device can be a hid device, a wmi device, a platform device etc. so I thought it would be nice to have some unified way for identification.
> I think that some shared ioctl for identifying devices which need a misc-device
> makes sense. But for existing hidraw supported keyboards there already is existing
> userspace support, which already retreives this in a hidraw specific manner.
>
> And I doubt that the existing userspace projects will add support for a new method
> which is only available on new kernels, while they will also need to keep the old
> method around to keep things working with older kernels.
>
> So at least for the hidraw case I see little value in exporting the same info
> in another way.
Ack on the no new device, but since we are adding use_leds_uapi to the parent
device anyway, having the identification on non-HID devices wie sysfs entries
would help even more in not creating a new device just to handle an ioctl.
Just to note here. When we go the LampArray route, everything is a HID device
anyway.
>
>
>>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>>>
>>>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
>>> Ack, if this finds it way into some documentation (which it should) please make it
>>> clear that this is about the "use_leds_uapi" sysfs attribute.
>> Ack
>>>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
>>> IMHO this is the only case where actually using a misc device makes sense, so that
>>> you have a chardev to do the ioctls on. misc-device-s should really only be used
>>> when you need a chardev under /dev . Since you don't need the chardev for the e.g.
>>> hidraw case you should not use a miscdev there IMHO.
>> My train of thought was that it would be nice to have the use_leds_uapi at a somewhat unified location in sysfs. The parent device can be of any kind, like I mentioned above, and while for deactivating it can easily be found via /sys/class/leds/*/device/. For reactivating, the leds entry doesn't exist.
> That is an interesting point. But I would expect any process/daemon which wants to
> reactivate the in kernel LED class support to be the same process as which
> deactivated it. So that daemon can simply canonicalize the /sys/class/leds/*/device
> symlink or canonicalize the entire /sys/class/leds/*/device/use_leds_uapi path
> and store the canonicalized version for when the daemon wants to reactivate things.
Ack
Regards,
Werner
>
> So I still think that having the miscdevice just for enumeration and
> use_leds_uapi purposes is overkill.
>
> Regards,
>
> Hans
>
>
Hi Werner,
On 3/25/24 5:48 PM, Werner Sembach wrote:
> Hi Benjamin,
>
> Am 25.03.24 um 16:56 schrieb Benjamin Tissoires:
>> On Mar 25 2024, Hans de Goede wrote:
>>> +Cc: Bentiss, Jiri
>>>
>>> Hi Werner,
>>>
>>> On 3/20/24 12:16 PM, Werner Sembach wrote:
>>>> Hi Hans and the others,
>>>>
>>>> Am 22.02.24 um 14:14 schrieb Werner Sembach:
>>>>> Hi,
>>>>>
>>>>> Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
>>>>>
>>>>> To recap the hopefully final UAPI for complex RGB lighting devices:
>>>>>
>>>>> - By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
>>>>>
>>>>> - There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.
>>>>>
>>>>> - If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
>>>>>
>>>>> - If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
>>>>>
>>>>> - If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.
>>>> So in the OpenRGB issue thread https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices aka HID LampArray was mentioned. I did dismiss it because I thought that is only relevant for firmware, but I now stumbled upon the Virtual HID Framework (VHF) https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- and now I wonder if an equivalent exists for Linux? A quick search did not yield any results for me.
>>> Oh, interesting. I did not know about the HID LampArray API.
>>>
>>> About your question about virtual HID devices, there is uHID,
>>> but as the name suggests this allows userspace to emulate a HID
>>> device.
>>>
>>> In your case you want to do the emulation in kernel so that you
>>> can translate the proprietary WMI calls to something HID LampArray
>>> compatible.
>>>
>>> I guess you could do this by defining your own HID transport driver,
>>> like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID
>>> "client" for each device which talks HID over i2c in the machine.
>>>
>>> Bentiss, Jiri, do you have any input on this. Would something like
>>> that be acceptable to you (just based on the concept at least) ?
>> I just read the thread, and I think I now understand a little bit what
>> this request is :)
>>
>> IMO working with the HID LampArray is the way forward. So I would
>> suggest to convert any non-HID RGB "LED display" that we are talking
>> about as a HID LampArray device through `hid_allocate_device()` in the
>> kernel. Basically what you are suggesting Hans. It's just that you don't
>> need a formal transport layer, just a child device that happens to be
>> HID.
>>
>> The next question IMO is: do we want the kernel to handle such
>> machinery? Wouldn't it be simpler to just export the HID device and let
>> userspace talk to it through hidraw, like what OpenRGB does?
>
> That's already part of my plan: The kernels main goal is to give devices a LampArray interface that don't have one already (e.g. because they are no HID devices to begin with).
>
> The actual handling of LampArray will happen in userspace.
>
> Exception is that maybe it could be useful to implement a small subset of LampArray in a generic leds-subsystem driver for backwards compatibility to userspace applications that only implement that (e.g. UPower). It would treat the whole keyboard as a single led.
>
>>
>> If the kernel already handles the custom protocol into generic HID, the
>> work for userspace is not too hard because they can deal with a known
>> protocol and can be cross-platform in their implementation.
>>
>> I'm mentioning that cross-platform because SDL used to rely on the
>> input, LEDs, and other Linux peculiarities and eventually fell back on
>> using hidraw only because it's way more easier that way.
>>
>> The other advantage of LampArray is that according to Microsoft's
>> document, new devices are going to support it out of the box, so they'll
>> be supported out of the box directly.
>>
>> Most of the time my stance is "do not add new kernel API, you'll regret
>> it later". So in that case, given that we have a formally approved
>> standard, I would suggest to use it, and consider it your API.
>
> The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
Actually we don't even need that. Typically there is a single HID
driver handling both keys and the backlight, so userspace cannot
just unbind the HID driver since then the keys stop working.
But with a virtual LampArray HID device the only functionality
for an in kernel HID driver would be to export a basic keyboard
backlight control interface for simple non per key backlight control
to integrate nicely with e.g. GNOME's backlight control.
And then when OpenRGB wants to take over it can just unbind the HID
driver from the HID device using existing mechanisms for that.
Hmm, I wonder if that will not also kill hidraw support though ...
I guess getting hidraw support back might require then also manually
binding the default HID input driver. Bentiss any input on this?
Background info: as discussed earlier in the thread Werner would like
to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
device, since those are automatically supported by GNOME (and others)
and will give basic kbd backlight brightness control in the desktop
environment. This could be a simple HID driver for
the hid_allocate_device()-ed virtual HID device, but userspace needs
to be able to move that out of the way when it wants to take over
full control of the per key lighting.
Regards,
Hans
>
> The control flow for the whole system would look something like this:
>
> - System boots
>
> - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
>
> - systemd-backlight restores last keyboard backlight brightness
>
> - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
>
> - If the user wants more control they (auto-)start OpenRGB
>
> - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
>
> - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
>
> - When OpenRGB closes it should reenable the sysfs leds entry
>
> - System shutdown
>
> - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
>
> Regards,
>
> Werner
>
>>
>> Side note to self: I really need to resurrect the hidraw revoke series
>> so we could export those hidraw node to userspace with uaccess through
>> logind...
>>
>> Cheers,
>> Benjamin
>
Hi all,
Am 25.03.24 um 19:30 schrieb Hans de Goede:
[snip]
>>> If the kernel already handles the custom protocol into generic HID, the
>>> work for userspace is not too hard because they can deal with a known
>>> protocol and can be cross-platform in their implementation.
>>>
>>> I'm mentioning that cross-platform because SDL used to rely on the
>>> input, LEDs, and other Linux peculiarities and eventually fell back on
>>> using hidraw only because it's way more easier that way.
>>>
>>> The other advantage of LampArray is that according to Microsoft's
>>> document, new devices are going to support it out of the box, so they'll
>>> be supported out of the box directly.
>>>
>>> Most of the time my stance is "do not add new kernel API, you'll regret
>>> it later". So in that case, given that we have a formally approved
>>> standard, I would suggest to use it, and consider it your API.
>> The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
> Actually we don't even need that. Typically there is a single HID
> driver handling both keys and the backlight, so userspace cannot
> just unbind the HID driver since then the keys stop working.
>
> But with a virtual LampArray HID device the only functionality
> for an in kernel HID driver would be to export a basic keyboard
> backlight control interface for simple non per key backlight control
> to integrate nicely with e.g. GNOME's backlight control.
Don't forget that in the future there will be devices that natively support
LampArray in their firmware, so for them it is the same device.
Regards,
Werner
> And then when OpenRGB wants to take over it can just unbind the HID
> driver from the HID device using existing mechanisms for that.
>
> Hmm, I wonder if that will not also kill hidraw support though ...
> I guess getting hidraw support back might require then also manually
> binding the default HID input driver. Bentiss any input on this?
>
> Background info: as discussed earlier in the thread Werner would like
> to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
> device, since those are automatically supported by GNOME (and others)
> and will give basic kbd backlight brightness control in the desktop
> environment. This could be a simple HID driver for
> the hid_allocate_device()-ed virtual HID device, but userspace needs
> to be able to move that out of the way when it wants to take over
> full control of the per key lighting.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>> The control flow for the whole system would look something like this:
>>
>> - System boots
>>
>> - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
>>
>> - systemd-backlight restores last keyboard backlight brightness
>>
>> - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
>>
>> - If the user wants more control they (auto-)start OpenRGB
>>
>> - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
>>
>> - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
>>
>> - When OpenRGB closes it should reenable the sysfs leds entry
>>
>> - System shutdown
>>
>> - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
>>
>> Regards,
>>
>> Werner
>>
>>> Side note to self: I really need to resurrect the hidraw revoke series
>>> so we could export those hidraw node to userspace with uaccess through
>>> logind...
>>>
>>> Cheers,
>>> Benjamin
On Mar 26 2024, Werner Sembach wrote:
> Hi all,
>
> Am 25.03.24 um 19:30 schrieb Hans de Goede:
>
> [snip]
> > > > If the kernel already handles the custom protocol into generic HID, the
> > > > work for userspace is not too hard because they can deal with a known
> > > > protocol and can be cross-platform in their implementation.
> > > >
> > > > I'm mentioning that cross-platform because SDL used to rely on the
> > > > input, LEDs, and other Linux peculiarities and eventually fell back on
> > > > using hidraw only because it's way more easier that way.
> > > >
> > > > The other advantage of LampArray is that according to Microsoft's
> > > > document, new devices are going to support it out of the box, so they'll
> > > > be supported out of the box directly.
> > > >
> > > > Most of the time my stance is "do not add new kernel API, you'll regret
> > > > it later". So in that case, given that we have a formally approved
> > > > standard, I would suggest to use it, and consider it your API.
> > > The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
I have my reserves with such a kill switch (see below).
> > Actually we don't even need that. Typically there is a single HID
> > driver handling both keys and the backlight, so userspace cannot
> > just unbind the HID driver since then the keys stop working.
I don't think Werner meant unbinding the HID driver, just a toggle to
enable/disable the basic HID core processing of LampArray.
> >
> > But with a virtual LampArray HID device the only functionality
> > for an in kernel HID driver would be to export a basic keyboard
> > backlight control interface for simple non per key backlight control
> > to integrate nicely with e.g. GNOME's backlight control.
>
> Don't forget that in the future there will be devices that natively support
> LampArray in their firmware, so for them it is the same device.
Yeah, the generic LampArray support will not be able to differentiate
"emulated" devices from native ones.
>
> Regards,
>
> Werner
>
> > And then when OpenRGB wants to take over it can just unbind the HID
> > driver from the HID device using existing mechanisms for that.
Again no, it'll be too unpredicted.
> >
> > Hmm, I wonder if that will not also kill hidraw support though ...
> > I guess getting hidraw support back might require then also manually
> > binding the default HID input driver. Bentiss any input on this?
To be able to talk over hidraw you need a driver to be bound, yes. But I
had the impression that LampArray would be supported by default in
hid-input.c, thus making this hard to remove. Having a separate driver
will work, but as soon as the LampArray device will also export a
multitouch touchpad, we are screwed and will have to make a choice
between LampArray and touch...
> >
> > Background info: as discussed earlier in the thread Werner would like
> > to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
> > device, since those are automatically supported by GNOME (and others)
> > and will give basic kbd backlight brightness control in the desktop
> > environment. This could be a simple HID driver for
> > the hid_allocate_device()-ed virtual HID device, but userspace needs
> > to be able to move that out of the way when it wants to take over
> > full control of the per key lighting.
Do we really need to entirely unregister the led class device? Can't we
snoop on the commands and get some "mean value"?
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >
> >
> > > The control flow for the whole system would look something like this:
> > >
> > > - System boots
> > >
> > > ??? - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
> > >
> > > ??? - systemd-backlight restores last keyboard backlight brightness
> > >
> > > ??? - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
> > >
> > > - If the user wants more control they (auto-)start OpenRGB
> > >
> > > ??? - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
> > >
> > > ??? - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
> > >
> > > ??? - When OpenRGB closes it should reenable the sysfs leds entry
That's where your plan falls short: if OpenRGB crashes, or is killed it
will not reset that bit.
Next question: is OpenRGB supposed to keep the hidraw node opened all
the time or not?
If it has to keep it open, we should be able to come up with a somewhat
similar hack that we have with hid-steam: when the hidraw node is
opened, we disable the kernel processing of LampArray. When the node is
closed, we re-enable it.
But that also means we have to distinguish steam/SDL from OpenRGB...
I just carefully read the LampArray spec. And it's simpler than what
I expected. But the thing that caught my attention was that it's
mentioned that there is no way for the host to query the current
color/illumination of the LEDs when the mode is set to
AutonomousMode=false. Which means that the kernel should be able to
snoop into any commands sent from OpenRGB to the device, compute a mean
value and update its internal state.
Basically all we need is the "toggle" to put the led class in read-only
mode while OpenRGB kicks in. Maybe given that we are about to snoop on
the commands sent, we can detect that there is a LampArray command
emitted, attach this information to the struct file * in hidraw, and
then re-enable rw when that user closes the hidraw node.
Cheers,
Benjamin
> > >
> > > - System shutdown
> > >
> > > ??? - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
> > >
> > > Regards,
> > >
> > > Werner
> > >
> > > > Side note to self: I really need to resurrect the hidraw revoke series
> > > > so we could export those hidraw node to userspace with uaccess through
> > > > logind...
> > > >
> > > > Cheers,
> > > > Benjamin
Hi all,
Am 26.03.24 um 16:39 schrieb Benjamin Tissoires:
> On Mar 26 2024, Werner Sembach wrote:
>> Hi all,
>>
>> Am 25.03.24 um 19:30 schrieb Hans de Goede:
>>
>> [snip]
>>>>> If the kernel already handles the custom protocol into generic HID, the
>>>>> work for userspace is not too hard because they can deal with a known
>>>>> protocol and can be cross-platform in their implementation.
>>>>>
>>>>> I'm mentioning that cross-platform because SDL used to rely on the
>>>>> input, LEDs, and other Linux peculiarities and eventually fell back on
>>>>> using hidraw only because it's way more easier that way.
>>>>>
>>>>> The other advantage of LampArray is that according to Microsoft's
>>>>> document, new devices are going to support it out of the box, so they'll
>>>>> be supported out of the box directly.
>>>>>
>>>>> Most of the time my stance is "do not add new kernel API, you'll regret
>>>>> it later". So in that case, given that we have a formally approved
>>>>> standard, I would suggest to use it, and consider it your API.
>>>> The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
> I have my reserves with such a kill switch (see below).
>
>>> Actually we don't even need that. Typically there is a single HID
>>> driver handling both keys and the backlight, so userspace cannot
>>> just unbind the HID driver since then the keys stop working.
> I don't think Werner meant unbinding the HID driver, just a toggle to
> enable/disable the basic HID core processing of LampArray.
>
>>> But with a virtual LampArray HID device the only functionality
>>> for an in kernel HID driver would be to export a basic keyboard
>>> backlight control interface for simple non per key backlight control
>>> to integrate nicely with e.g. GNOME's backlight control.
>> Don't forget that in the future there will be devices that natively support
>> LampArray in their firmware, so for them it is the same device.
> Yeah, the generic LampArray support will not be able to differentiate
> "emulated" devices from native ones.
>
>> Regards,
>>
>> Werner
>>
>>> And then when OpenRGB wants to take over it can just unbind the HID
>>> driver from the HID device using existing mechanisms for that.
> Again no, it'll be too unpredicted.
>
>>> Hmm, I wonder if that will not also kill hidraw support though ...
>>> I guess getting hidraw support back might require then also manually
>>> binding the default HID input driver. Bentiss any input on this?
> To be able to talk over hidraw you need a driver to be bound, yes. But I
> had the impression that LampArray would be supported by default in
> hid-input.c, thus making this hard to remove. Having a separate driver
> will work, but as soon as the LampArray device will also export a
> multitouch touchpad, we are screwed and will have to make a choice
> between LampArray and touch...
>
>>> Background info: as discussed earlier in the thread Werner would like
>>> to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
>>> device, since those are automatically supported by GNOME (and others)
>>> and will give basic kbd backlight brightness control in the desktop
>>> environment. This could be a simple HID driver for
>>> the hid_allocate_device()-ed virtual HID device, but userspace needs
>>> to be able to move that out of the way when it wants to take over
>>> full control of the per key lighting.
> Do we really need to entirely unregister the led class device? Can't we
> snoop on the commands and get some "mean value"?
>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>> The control flow for the whole system would look something like this:
>>>>
>>>> - System boots
>>>>
>>>> - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
>>>>
>>>> - systemd-backlight restores last keyboard backlight brightness
>>>>
>>>> - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
>>>>
>>>> - If the user wants more control they (auto-)start OpenRGB
>>>>
>>>> - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
>>>>
>>>> - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
>>>>
>>>> - When OpenRGB closes it should reenable the sysfs leds entry
> That's where your plan falls short: if OpenRGB crashes, or is killed it
> will not reset that bit.
>
> Next question: is OpenRGB supposed to keep the hidraw node opened all
> the time or not?
TBH I didn't look at the OpenRGB code yet and LampArray there is currently only
planned. I somewhat hope that until the kernel driver is ready someone else
already picked up implementing LampArray in OpenRGB.
>
> If it has to keep it open, we should be able to come up with a somewhat
> similar hack that we have with hid-steam: when the hidraw node is
> opened, we disable the kernel processing of LampArray. When the node is
> closed, we re-enable it.
>
> But that also means we have to distinguish steam/SDL from OpenRGB...
My first thought here also: What is if something else is reading hidraw devices?
Especially for hidraw devices that are not just LampArray.
>
> I just carefully read the LampArray spec. And it's simpler than what
> I expected. But the thing that caught my attention was that it's
> mentioned that there is no way for the host to query the current
> color/illumination of the LEDs when the mode is set to
> AutonomousMode=false. Which means that the kernel should be able to
> snoop into any commands sent from OpenRGB to the device, compute a mean
> value and update its internal state.
>
> Basically all we need is the "toggle" to put the led class in read-only
> mode while OpenRGB kicks in. Maybe given that we are about to snoop on
> the commands sent, we can detect that there is a LampArray command
> emitted, attach this information to the struct file * in hidraw, and
> then re-enable rw when that user closes the hidraw node.
I think a read-only mode is not part of the current led class UAPI. Also I don't
want to associate AutonomousMode=true with led class is used.
AutonomousMode=true could for example mean that it is controlled via keyboard
shortcuts that are directly handled in the keyboard firmware, aka a case where
you want neither OpenRGB nor led class make any writes to the keyboard.
Or AutonomousMode=true could mean that on a device that implements both a
LampArray interface as well as a proprietary legacy interface is currently
controlled via the proprietary legacy interface (a lot of which are supported by
OpenRGB).
Regards,
Werner
>
> Cheers,
> Benjamin
>
>>>> - System shutdown
>>>>
>>>> - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
>>>>
>>>> Regards,
>>>>
>>>> Werner
>>>>
>>>>> Side note to self: I really need to resurrect the hidraw revoke series
>>>>> so we could export those hidraw node to userspace with uaccess through
>>>>> logind...
>>>>>
>>>>> Cheers,
>>>>> Benjamin
Hi,
On 3/26/24 4:39 PM, Benjamin Tissoires wrote:
> On Mar 26 2024, Werner Sembach wrote:
>> Hi all,
>>
>> Am 25.03.24 um 19:30 schrieb Hans de Goede:
>>
>> [snip]
>>>>> If the kernel already handles the custom protocol into generic HID, the
>>>>> work for userspace is not too hard because they can deal with a known
>>>>> protocol and can be cross-platform in their implementation.
>>>>>
>>>>> I'm mentioning that cross-platform because SDL used to rely on the
>>>>> input, LEDs, and other Linux peculiarities and eventually fell back on
>>>>> using hidraw only because it's way more easier that way.
>>>>>
>>>>> The other advantage of LampArray is that according to Microsoft's
>>>>> document, new devices are going to support it out of the box, so they'll
>>>>> be supported out of the box directly.
>>>>>
>>>>> Most of the time my stance is "do not add new kernel API, you'll regret
>>>>> it later". So in that case, given that we have a formally approved
>>>>> standard, I would suggest to use it, and consider it your API.
>>>> The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
>
> I have my reserves with such a kill switch (see below).
>
>>> Actually we don't even need that. Typically there is a single HID
>>> driver handling both keys and the backlight, so userspace cannot
>>> just unbind the HID driver since then the keys stop working.
>
> I don't think Werner meant unbinding the HID driver, just a toggle to
> enable/disable the basic HID core processing of LampArray.
Right, what I was trying to say is that unbinding the driver
might be an alternative. I know things like the G15 keyboard
userspace daemons used to do this in the past.
But Werner is right that this won't be an option if the actual
keyboard presses and the LampArray parts are part of a single
HID device.
>
>>>
>>> But with a virtual LampArray HID device the only functionality
>>> for an in kernel HID driver would be to export a basic keyboard
>>> backlight control interface for simple non per key backlight control
>>> to integrate nicely with e.g. GNOME's backlight control.
>>
>> Don't forget that in the future there will be devices that natively support
>> LampArray in their firmware, so for them it is the same device.
>
> Yeah, the generic LampArray support will not be able to differentiate
> "emulated" devices from native ones.
>
>>
>> Regards,
>>
>> Werner
>>
>>> And then when OpenRGB wants to take over it can just unbind the HID
>>> driver from the HID device using existing mechanisms for that.
>
> Again no, it'll be too unpredicted.
>
>>>
>>> Hmm, I wonder if that will not also kill hidraw support though ...
>>> I guess getting hidraw support back might require then also manually
>>> binding the default HID input driver. Bentiss any input on this?
>
> To be able to talk over hidraw you need a driver to be bound, yes. But I
> had the impression that LampArray would be supported by default in
> hid-input.c, thus making this hard to remove. Having a separate driver
> will work, but as soon as the LampArray device will also export a
> multitouch touchpad, we are screwed and will have to make a choice
> between LampArray and touch...
The idea is to have the actual RGB support in userspace through hidraw,
I believe we all agreed on that higher up in the thread.
Werner would like for there to also be a simpler in kernel support
which treats the per key lighting as if it is a more standard
(e.g. thinkpad x1) style keyboard backlight so that existing desktop
environment integration for that will work for users who do not
install say openrgb.
The question is how do we disable the in kernel basic kbd_backlight support
when openrgb wants to take over and fully control the LEDs ?
Given that driver unbinding is out of the question I think that we are
back to having a sysfs attribute to disable / re-enable the in kernel
basic kbd_backlight support.
Note that the basic kbd_backlight support also allows e.g. GNOME to
set the brightness (not only monitor it) so at a minimum we need to
disable the "write" support when e.g. openrgb has control.
Regards,
Hans
On Mar 26 2024, Werner Sembach wrote:
> Hi all,
>
> Am 26.03.24 um 16:39 schrieb Benjamin Tissoires:
> > On Mar 26 2024, Werner Sembach wrote:
> > > Hi all,
> > >
> > > Am 25.03.24 um 19:30 schrieb Hans de Goede:
> > >
> > > [snip]
> > > > > > If the kernel already handles the custom protocol into generic HID, the
> > > > > > work for userspace is not too hard because they can deal with a known
> > > > > > protocol and can be cross-platform in their implementation.
> > > > > >
> > > > > > I'm mentioning that cross-platform because SDL used to rely on the
> > > > > > input, LEDs, and other Linux peculiarities and eventually fell back on
> > > > > > using hidraw only because it's way more easier that way.
> > > > > >
> > > > > > The other advantage of LampArray is that according to Microsoft's
> > > > > > document, new devices are going to support it out of the box, so they'll
> > > > > > be supported out of the box directly.
> > > > > >
> > > > > > Most of the time my stance is "do not add new kernel API, you'll regret
> > > > > > it later". So in that case, given that we have a formally approved
> > > > > > standard, I would suggest to use it, and consider it your API.
> > > > > The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
> > I have my reserves with such a kill switch (see below).
> >
> > > > Actually we don't even need that. Typically there is a single HID
> > > > driver handling both keys and the backlight, so userspace cannot
> > > > just unbind the HID driver since then the keys stop working.
> > I don't think Werner meant unbinding the HID driver, just a toggle to
> > enable/disable the basic HID core processing of LampArray.
> >
> > > > But with a virtual LampArray HID device the only functionality
> > > > for an in kernel HID driver would be to export a basic keyboard
> > > > backlight control interface for simple non per key backlight control
> > > > to integrate nicely with e.g. GNOME's backlight control.
> > > Don't forget that in the future there will be devices that natively support
> > > LampArray in their firmware, so for them it is the same device.
> > Yeah, the generic LampArray support will not be able to differentiate
> > "emulated" devices from native ones.
> >
> > > Regards,
> > >
> > > Werner
> > >
> > > > And then when OpenRGB wants to take over it can just unbind the HID
> > > > driver from the HID device using existing mechanisms for that.
> > Again no, it'll be too unpredicted.
> >
> > > > Hmm, I wonder if that will not also kill hidraw support though ...
> > > > I guess getting hidraw support back might require then also manually
> > > > binding the default HID input driver. Bentiss any input on this?
> > To be able to talk over hidraw you need a driver to be bound, yes. But I
> > had the impression that LampArray would be supported by default in
> > hid-input.c, thus making this hard to remove. Having a separate driver
> > will work, but as soon as the LampArray device will also export a
> > multitouch touchpad, we are screwed and will have to make a choice
> > between LampArray and touch...
> >
> > > > Background info: as discussed earlier in the thread Werner would like
> > > > to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
> > > > device, since those are automatically supported by GNOME (and others)
> > > > and will give basic kbd backlight brightness control in the desktop
> > > > environment. This could be a simple HID driver for
> > > > the hid_allocate_device()-ed virtual HID device, but userspace needs
> > > > to be able to move that out of the way when it wants to take over
> > > > full control of the per key lighting.
> > Do we really need to entirely unregister the led class device? Can't we
> > snoop on the commands and get some "mean value"?
> >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > > The control flow for the whole system would look something like this:
> > > > >
> > > > > - System boots
> > > > >
> > > > > ??? - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
> > > > >
> > > > > ??? - systemd-backlight restores last keyboard backlight brightness
> > > > >
> > > > > ??? - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
> > > > >
> > > > > - If the user wants more control they (auto-)start OpenRGB
> > > > >
> > > > > ??? - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
> > > > >
> > > > > ??? - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
> > > > >
> > > > > ??? - When OpenRGB closes it should reenable the sysfs leds entry
> > That's where your plan falls short: if OpenRGB crashes, or is killed it
> > will not reset that bit.
> >
> > Next question: is OpenRGB supposed to keep the hidraw node opened all
> > the time or not?
> TBH I didn't look at the OpenRGB code yet and LampArray there is currently
> only planned. I somewhat hope that until the kernel driver is ready someone
> else already picked up implementing LampArray in OpenRGB.
> >
> > If it has to keep it open, we should be able to come up with a somewhat
> > similar hack that we have with hid-steam: when the hidraw node is
> > opened, we disable the kernel processing of LampArray. When the node is
> > closed, we re-enable it.
> >
> > But that also means we have to distinguish steam/SDL from OpenRGB...
>
> My first thought here also: What is if something else is reading hidraw devices?
>
> Especially for hidraw devices that are not just LampArray.
>
> >
> > I just carefully read the LampArray spec. And it's simpler than what
> > I expected. But the thing that caught my attention was that it's
> > mentioned that there is no way for the host to query the current
> > color/illumination of the LEDs when the mode is set to
> > AutonomousMode=false. Which means that the kernel should be able to
> > snoop into any commands sent from OpenRGB to the device, compute a mean
> > value and update its internal state.
> >
> > Basically all we need is the "toggle" to put the led class in read-only
> > mode while OpenRGB kicks in. Maybe given that we are about to snoop on
> > the commands sent, we can detect that there is a LampArray command
> > emitted, attach this information to the struct file * in hidraw, and
> > then re-enable rw when that user closes the hidraw node.
>
> I think a read-only mode is not part of the current led class UAPI. Also I
> don't want to associate AutonomousMode=true with led class is used.
> AutonomousMode=true could for example mean that it is controlled via
> keyboard shortcuts that are directly handled in the keyboard firmware, aka a
> case where you want neither OpenRGB nor led class make any writes to the
> keyboard.
>
> Or AutonomousMode=true could mean that on a device that implements both a
> LampArray interface as well as a proprietary legacy interface is currently
> controlled via the proprietary legacy interface (a lot of which are
> supported by OpenRGB).
Then how is the kernel supposed to handle LampArrays?
If you need the kernel to use a ledclass, the kernel will have to set
the device into AutonomousMode=false. When the kernel is done
configuring the leds, it can not switch back to AutonomousMode=true or
its config will likely be dumped by the device.
OpenRGB can open the device, switch it to AutonomousMode=false and we
can rely on it to do the right things as long as it is alive. But I do
not see how the kernel could do the same.
FWIW, I also have a couple of crazy ideas currently boiling in my head
to "solve" that but I'd rather have a consensus on the high level side
of things before we go too deep into the technical workaround.
Cheers,
Benjamin
>
> Regards,
>
> Werner
>
> >
> > Cheers,
> > Benjamin
> >
> > > > > - System shutdown
> > > > >
> > > > > ??? - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
> > > > >
> > > > > Regards,
> > > > >
> > > > > Werner
> > > > >
> > > > > > Side note to self: I really need to resurrect the hidraw revoke series
> > > > > > so we could export those hidraw node to userspace with uaccess through
> > > > > > logind...
> > > > > >
> > > > > > Cheers,
> > > > > > Benjamin
Hi Benjamin,
Am 27.03.24 um 12:03 schrieb Benjamin Tissoires:
> On Mar 26 2024, Werner Sembach wrote:
>> Hi all,
>>
>> Am 26.03.24 um 16:39 schrieb Benjamin Tissoires:
>>> On Mar 26 2024, Werner Sembach wrote:
>>>> Hi all,
>>>>
>>>> Am 25.03.24 um 19:30 schrieb Hans de Goede:
>>>>
>>>> [snip]
>>>>>>> If the kernel already handles the custom protocol into generic HID, the
>>>>>>> work for userspace is not too hard because they can deal with a known
>>>>>>> protocol and can be cross-platform in their implementation.
>>>>>>>
>>>>>>> I'm mentioning that cross-platform because SDL used to rely on the
>>>>>>> input, LEDs, and other Linux peculiarities and eventually fell back on
>>>>>>> using hidraw only because it's way more easier that way.
>>>>>>>
>>>>>>> The other advantage of LampArray is that according to Microsoft's
>>>>>>> document, new devices are going to support it out of the box, so they'll
>>>>>>> be supported out of the box directly.
>>>>>>>
>>>>>>> Most of the time my stance is "do not add new kernel API, you'll regret
>>>>>>> it later". So in that case, given that we have a formally approved
>>>>>>> standard, I would suggest to use it, and consider it your API.
>>>>>> The only new UAPI would be the use_leds_uapi switch to turn on/off the backwards compatibility.
>>> I have my reserves with such a kill switch (see below).
>>>
>>>>> Actually we don't even need that. Typically there is a single HID
>>>>> driver handling both keys and the backlight, so userspace cannot
>>>>> just unbind the HID driver since then the keys stop working.
>>> I don't think Werner meant unbinding the HID driver, just a toggle to
>>> enable/disable the basic HID core processing of LampArray.
>>>
>>>>> But with a virtual LampArray HID device the only functionality
>>>>> for an in kernel HID driver would be to export a basic keyboard
>>>>> backlight control interface for simple non per key backlight control
>>>>> to integrate nicely with e.g. GNOME's backlight control.
>>>> Don't forget that in the future there will be devices that natively support
>>>> LampArray in their firmware, so for them it is the same device.
>>> Yeah, the generic LampArray support will not be able to differentiate
>>> "emulated" devices from native ones.
>>>
>>>> Regards,
>>>>
>>>> Werner
>>>>
>>>>> And then when OpenRGB wants to take over it can just unbind the HID
>>>>> driver from the HID device using existing mechanisms for that.
>>> Again no, it'll be too unpredicted.
>>>
>>>>> Hmm, I wonder if that will not also kill hidraw support though ...
>>>>> I guess getting hidraw support back might require then also manually
>>>>> binding the default HID input driver. Bentiss any input on this?
>>> To be able to talk over hidraw you need a driver to be bound, yes. But I
>>> had the impression that LampArray would be supported by default in
>>> hid-input.c, thus making this hard to remove. Having a separate driver
>>> will work, but as soon as the LampArray device will also export a
>>> multitouch touchpad, we are screwed and will have to make a choice
>>> between LampArray and touch...
>>>
>>>>> Background info: as discussed earlier in the thread Werner would like
>>>>> to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/
>>>>> device, since those are automatically supported by GNOME (and others)
>>>>> and will give basic kbd backlight brightness control in the desktop
>>>>> environment. This could be a simple HID driver for
>>>>> the hid_allocate_device()-ed virtual HID device, but userspace needs
>>>>> to be able to move that out of the way when it wants to take over
>>>>> full control of the per key lighting.
>>> Do we really need to entirely unregister the led class device? Can't we
>>> snoop on the commands and get some "mean value"?
>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> The control flow for the whole system would look something like this:
>>>>>>
>>>>>> - System boots
>>>>>>
>>>>>> - Kernel driver initializes keyboard (maybe stops rainbowpuke boot effects, sets brightness to a default value, or initializes a solid color)
>>>>>>
>>>>>> - systemd-backlight restores last keyboard backlight brightness
>>>>>>
>>>>>> - UPower sees sysfs leds entry and exposes it to DBus for DEs to do keyboard brightness handling
>>>>>>
>>>>>> - If the user wants more control they (auto-)start OpenRGB
>>>>>>
>>>>>> - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent double control of the same device by UPower
>>>>>>
>>>>>> - OpenRGB directly interacts with hidraw device via LampArray API to give fine granular control of the backlight
>>>>>>
>>>>>> - When OpenRGB closes it should reenable the sysfs leds entry
>>> That's where your plan falls short: if OpenRGB crashes, or is killed it
>>> will not reset that bit.
>>>
>>> Next question: is OpenRGB supposed to keep the hidraw node opened all
>>> the time or not?
>> TBH I didn't look at the OpenRGB code yet and LampArray there is currently
>> only planned. I somewhat hope that until the kernel driver is ready someone
>> else already picked up implementing LampArray in OpenRGB.
>>> If it has to keep it open, we should be able to come up with a somewhat
>>> similar hack that we have with hid-steam: when the hidraw node is
>>> opened, we disable the kernel processing of LampArray. When the node is
>>> closed, we re-enable it.
>>>
>>> But that also means we have to distinguish steam/SDL from OpenRGB...
>> My first thought here also: What is if something else is reading hidraw devices?
>>
>> Especially for hidraw devices that are not just LampArray.
>>
>>> I just carefully read the LampArray spec. And it's simpler than what
>>> I expected. But the thing that caught my attention was that it's
>>> mentioned that there is no way for the host to query the current
>>> color/illumination of the LEDs when the mode is set to
>>> AutonomousMode=false. Which means that the kernel should be able to
>>> snoop into any commands sent from OpenRGB to the device, compute a mean
>>> value and update its internal state.
>>>
>>> Basically all we need is the "toggle" to put the led class in read-only
>>> mode while OpenRGB kicks in. Maybe given that we are about to snoop on
>>> the commands sent, we can detect that there is a LampArray command
>>> emitted, attach this information to the struct file * in hidraw, and
>>> then re-enable rw when that user closes the hidraw node.
>> I think a read-only mode is not part of the current led class UAPI. Also I
>> don't want to associate AutonomousMode=true with led class is used.
>> AutonomousMode=true could for example mean that it is controlled via
>> keyboard shortcuts that are directly handled in the keyboard firmware, aka a
>> case where you want neither OpenRGB nor led class make any writes to the
>> keyboard.
>>
>> Or AutonomousMode=true could mean that on a device that implements both a
>> LampArray interface as well as a proprietary legacy interface is currently
>> controlled via the proprietary legacy interface (a lot of which are
>> supported by OpenRGB).
> Then how is the kernel supposed to handle LampArrays?
>
> If you need the kernel to use a ledclass, the kernel will have to set
> the device into AutonomousMode=false. When the kernel is done
> configuring the leds, it can not switch back to AutonomousMode=true or
> its config will likely be dumped by the device.
Yes, the kernel leds class driver will set AutonomousMode=false and keep it that
way.
The userspace driver/OpenRGB will most likely do the same unless there is a
proprietary API active in AutonomousMode=true it wants to use.
>
> OpenRGB can open the device, switch it to AutonomousMode=false and we
> can rely on it to do the right things as long as it is alive. But I do
> not see how the kernel could do the same.
AutonomousMode=false ^= LampArray API is used, AutonomousMode=true something
else (if I read the HID docs correctly).
Both the kernel leds class driver as well as OpenRGB probably will interact with
these devices via the LampArray API => AutonomousMode=false.
The kernel leds class driver is the fallback as long as userspace don't want to
control the lighting. Userspace should get priority over kernel space here. So
only kernel needs to know if userspace is controlling the device, not the other
way around. So from this perspective checking in kernel if the fd is in use
could be used, but what about userspace programs that open the hidraw device for
not controlling the leds?
So imho userspace needs some way to explicitly signal it's intentions, e.g. via
a sysfs attribute.
Best regards,
Werner
>
> FWIW, I also have a couple of crazy ideas currently boiling in my head
> to "solve" that but I'd rather have a consensus on the high level side
> of things before we go too deep into the technical workaround.
>
> Cheers,
> Benjamin
>
>> Regards,
>>
>> Werner
>>
>>> Cheers,
>>> Benjamin
>>>
>>>>>> - System shutdown
>>>>>>
>>>>>> - Since OpenRGB reenables the sysfs leds entry, systemd-backlight can correctly store a brightness value for next boot
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Werner
>>>>>>
>>>>>>> Side note to self: I really need to resurrect the hidraw revoke series
>>>>>>> so we could export those hidraw node to userspace with uaccess through
>>>>>>> logind...
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Benjamin
On Mon, Mar 25, 2024 at 07:38:46PM +0100, Miguel Ojeda wrote:
> On Mon, Mar 25, 2024 at 3:25 PM Hans de Goede <[email protected]> wrote:
> >
> > +Cc: Bentiss, Jiri
>
> Cc'ing Andy and Geert as well who recently became the
> maintainers/reviewers of auxdisplay, in case they are interested in
> these threads (one of the initial solutions discussed in a past thread
> a while ago was to extend auxdisplay).
Without diving into this, just sharing my view on auxdisplay subsystem:
I consider it _mostly_ (like lim->100% mathematically speaking) as for
7-segment and alike displays, not any comples RGB or so devices. If
those devices are capable of representing characters/digits in similar
way, we may export linedisp library for them to utilise.
--
With Best Regards,
Andy Shevchenko
