Message-ID: <CAEc3jaDsv0hv8gcqsMdcrMf4WK-Nq0LMgPcVHzv87x+J0M=V2Q@mail.gmail.com>
Date: Wed, 31 Jan 2024 07:52:17 -0800
From: Roderick Colenbrander <thunderbird2k@...il.com>
To: Werner Sembach <wse@...edocomputers.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, 
	jikos@...nel.org, linux-kernel@...r.kernel.org, 
	Jelle van der Waa <jelle@...aa.nl>, Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, 
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, linux-input@...r.kernel.org, 
	ojeda@...nel.org, linux-leds@...r.kernel.org
Subject: Re: Future handling of complex RGB devices on Linux

On Wed, Jan 31, 2024 at 3:42 AM Werner Sembach <wse@...edocomputerscom> 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

Powered by blists - more mailing lists