Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

[Armin Wolf <W_Armin@gmx.de>]

Am 22.10.24 um 11:47 schrieb Pavel Machek:

> Hi!
>
>>> Sorry for taking a bit long to respond.
>>>
>>> This "illumination" subsystem would (from my perspective) act like some sort of LED subsystem
>>> for devices with a high count of LEDs, like some RGB keyboards.
>>>
>>> This would allow us too:
>>> - provide an abstract interface for userspace applications like OpenRGB
>>> - provide an generic LED subsystem emulation on top of the illumination device (optional)
>>> - support future RGB controllers in a generic way
>>>
>>> Advanced features like RGB effects, etc can be added later should the need arise.
>>>
>>> I would suggest that we model it after the HID LampArray interface:
>>>
>>> - interface for querying:
>>>   - number of LEDs
>>>   - supported colors, etc of those LEDs
>>>   - position of those LEDs if available
>>>   - kind (keyboard, ...)
>>>   - latency, etc
>>> - interface for setting multiple LEDs at once
>>> - interface for setting a range of LEDs at once
> How are LEDs ordered? I don't believe range makes much sense.

Range would allow for efficiently changing the color of all LEDs. But i agree
that this can be considered optional and can be added later.

Should we ever prototype such an interface, then providing a method for setting
multiple LEDs at once would be enough.

>>> I do not know if mixing sysfs (for controller attributes like number of LEDs, etc) and IOCTL
>>> (for setting/getting LED colors) is a good idea, any thoughts?
>> I wonder what the advantage of this approach is over simply using HID LampArray
>> (emulation), openRGB is already going to support HID LampArray and since Microsoft
>> is pushing this we will likely see it getting used more and more.
> There's nothing simple about "HID LampArray". Specification is long
> ang ugly... and we don't want to be stuck with with OpenRGB (links to QT!).

And HID LampArray its not easily extendable.

>
>> Using HID LampArray also has the advantage that work has landed and is landing
>> to allow safely handing over raw HID access to userspace programs or even
>> individual graphical apps with the option to revoke that access when it is
>> no longer desired for the app to have access.
> HID raw is not suitable kernel interface.

I agree, using HID raw in this case would be like amdgpu emulating a i915 GPU to
support applications working with a i915 GPU.

>> Personally I really like the idea to just emulate a HID LampArray device
>> for this instead or rolling our own API.  I believe there need to be
>> strong arguments to go with some alternative NIH API and I have not
>> heard such arguments yet.
> If you don't want "some alternative API", we already have perfectly
> working API for 2D arrays of LEDs. I believe I mentioned it before
> :-). Senzrohssre.
>
> 								Pavel

We may have to support 3D arrays of LEDs, so using a simple framebuffer
would likely cause trouble.

I think of something like this:

illumination class:

sysfs attrs:

  - lamp_count
  - kind (optional)
  - width, height, length (all optional)
  - latency (optional)
  - driver-defined attributes like firmware_version, ... (optional)

ioctl interface:

  - get LED info (id, supported colors, position (optional), key code (optional), ...)
  - get current color of LEDs
  - set multiple LEDs (by ID)

This interface is similar the the HID LampArray interface except that:

  - we can read the current color
  - we can omit optional information
  - we can extend the interface later (animations, etc)

Thanks,
Armin Wolf