leds: rgb: Implement per-key keyboard backlight for several TUXEDO devices

From: Christoffer Sandberg <cs@tuxedo.de>

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 <wse@tuxedocomputers.com>
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>
---
 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

Hi,

Am 11.10.23 um 21:00 schrieb Werner Sembach:
> From: Christoffer Sandberg <cs@tuxedo.de>
>
> 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 <wse@tuxedocomputers.com>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>

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 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/202310122012.C2mSREZ7-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231012/202310122012.C2mSREZ7-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310122012.C2mSREZ7-lkp@intel.com/

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

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

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

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

On Fri, Oct 13, 2023 at 9:56 PM Pavel Machek <pavel@ucw.cz> 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/202310161944.966gHsq4-lkp@intel.com/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/202310161944.966gHsq4-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310161944.966gHsq4-lkp@intel.com/

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

On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <jani.nikula@linux.intel.com> 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

On Mon 2023-10-23 13:44:46, Miguel Ojeda wrote:
> On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <jani.nikula@linux.intel.com> 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

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/544484b9-c0ac-2fd0-1f41-8fa94cb94d4b@redhat.com/

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/544484b9-c0ac-2fd0-1f41-8fa94cb94d4b@redhat.com/
>
> 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/544484b9-c0ac-2fd0-1f41-8fa94cb94d4b@redhat.com/
>>
>> 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

Am 17.01.24 um 17:50 schrieb Hans de Goede:

> 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,

i can understand your concerns, but i strongly advise against a generic WMI userspace API.
The reasons for this are:

1. We are still unable to parse (and use) the binary MOF buffers describing the WMI devices,
so all of that would require the driver parsing a raw byte buffer. In this case a separate
misc device managed by the driver would basically do the same.

2. Many WMI implementations are like RWEverything implemented inside the ACPI firmware, so
most devices will require the driver to do excessive filtering. Many implementations also do
no proper input validation either so the driver has to know all possible use cases since he
has to protect the buggy ACPI firmware from userspace attacks.

Regarding point number 2, i just had to contact Asus so that they remove a broken WMI interface
from my motherboard or else a simple application could crash the Windows kernel. This firmware
is (sadly) being designed as an internal API and thus unstable beyond all imagination.

For HID devices, a userspace driver might be OK since they are somewhat isolated from the remaining
hardware, but WMI is basically a kernel bypass for ACPI firmware calls, so userspace could easily
attack the kernel is way.

Personally, i would prefer extending the LED subsystem to support zone-like devices with many LEDs,
as this would prevent userspace from having to tinker with the hardware behind the kernels back.
Other highly device-specific features could be implemented with a driver-specific misc device.

Regarding the speed, it depends on the underlying WMI interface design if smooth animations are
even possible, since many WMI interfaces are quite slow. Can you share the Binary MOF buffers
describing the WMI interfaces in question?

Thanks,
Armin Wolf

Hi Hans and Armin,

Am 17.01.24 um 20:03 schrieb Armin Wolf:
> Am 17.01.24 um 17:50 schrieb Hans de Goede:
>
>> 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,
>
> i can understand your concerns, but i strongly advise against a generic WMI 
> userspace API.
> The reasons for this are:
>
> 1. We are still unable to parse (and use) the binary MOF buffers describing 
> the WMI devices,
> so all of that would require the driver parsing a raw byte buffer. In this 
> case a separate
> misc device managed by the driver would basically do the same.
>
> 2. Many WMI implementations are like RWEverything implemented inside the ACPI 
> firmware, so
> most devices will require the driver to do excessive filtering. Many 
> implementations also do
> no proper input validation either so the driver has to know all possible use 
> cases since he
> has to protect the buggy ACPI firmware from userspace attacks.

Or the WMI has a straight forward arbitrary read/write function into EC ram 
(e.g. all Uniwill/TongFang devices).

The filtering would need to be explicit whitelisting of wmi-calls+arguments. 
Don't know if this would reduce complexity for the kernel.

>
> Regarding point number 2, i just had to contact Asus so that they remove a 
> broken WMI interface
> from my motherboard or else a simple application could crash the Windows 
> kernel. This firmware
> is (sadly) being designed as an internal API and thus unstable beyond all 
> imagination.
>
> For HID devices, a userspace driver might be OK since they are somewhat 
> isolated from the remaining
> hardware, but WMI is basically a kernel bypass for ACPI firmware calls, so 
> userspace could easily
> attack the kernel is way.
>
> Personally, i would prefer extending the LED subsystem to support zone-like 
> devices with many LEDs,
> as this would prevent userspace from having to tinker with the hardware behind 
> the kernels back.
> Other highly device-specific features could be implemented with a 
> driver-specific misc device.

Something like my earlier suggestion "[...] adds a new entry zones_count. 
multi_intensity has now colors count * zones_count entries. aka a RGB keyboard 
with 126 leds would take 378 values for multi_intensity [...]"?

Setting all with one file access to multi_intensity could make it somewhat 
performant as Hans already mentioned, but also would violate the one file one 
led paradigm.

Or formulated differently: How should the sysfs folder look:

leds/
     rgb:kbd_backlight_a/
         brightness
         multi_intensity
     rgb:kbd_backlight_b/
         brightness
         multi_intensity
     ...

or

leds/
     rgb:kbd_backlight/
         brightness
         multi_intensity_a
         multi_intensity_b
         ...

or

leds/
     rgb:kbd_backlight/
         brightness
         zones_count
         multi_intensity

Personally I don't really like the idea of having the color set in 
/sys/class/leds/*:rgb:kbd_backlight/multi_intensity and e.g. the breathing mode 
in /sys/class/misc/<some_random_name>/<some_random_attribute>. Or at least there 
should be a hint in /sys/class/leds/*:rgb:kbd_backlight/ for the userspace to 
know where to look for associated additional attributes.

>
> Regarding the speed, it depends on the underlying WMI interface design if 
> smooth animations are
> even possible, since many WMI interfaces are quite slow. Can you share the 
> Binary MOF buffers
> describing the WMI interfaces in question?
Taking a colleague in the loop who currently has the device at hand. 
@Christoffer can you extract it? Is it one wmi call per key or is there a "set 
all" wmi call (because performance)?
>
> Thanks,
> Armin Wolf
>
Kind regards,

Werner

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

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 <hdegoede@redhat.com> 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
>
>
>

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 <hdegoede@redhat.com> 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 <hdegoede@redhat.com> 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.

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

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

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

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@tuxedocomputers.com> 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

On Wed, 21 Feb 2024 23:17:52 +0100
Pavel Machek <pavel@ucw.cz> 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

> 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,

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

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

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 <pavel@ucw.cz> 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

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
>>>

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

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 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
>

On Mon, Mar 25, 2024 at 3:25 PM Hans de Goede <hdegoede@redhat.com> 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 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

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

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 <hdegoede@redhat.com> 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.