Received-SPF: pass (google.com: domain of linux-kernel+bounces-45020-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <952409e1-2f0e-4d7a-a7a9-3b78f2eafec7@redhat.com>
Date: Tue, 30 Jan 2024 18:10:37 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Implement per-key keyboard backlight as auxdisplay?
Content-Language: en-US, nl
To: Werner Sembach <wse@tuxedocomputers.com>, Pavel Machek <pavel@ucw.cz>
Cc: Jani Nikula <jani.nikula@linux.intel.com>, jikos@kernel.org,
 Jelle van der Waa <jelle@vdwaa.nl>,
 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>, Lee Jones <lee@kernel.org>,
 linux-kernel@vger.kernel.org,
 "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
 linux-input@vger.kernel.org, ojeda@kernel.org, linux-leds@vger.kernel.org
References: <aac81702-df1e-43a2-bfe9-28e9cb8d2282@tuxedocomputers.com>
 <ZSmg4tqXiYiX18K/@duo.ucw.cz>
 <CANiq72mfP+dOLFR352O0UNVF8m8yTi_VmOY1zzQdTBjPWCRowg@mail.gmail.com>
 <87sf61bm8t.fsf@intel.com> <ZVvHG/Q+V6kCnfKZ@duo.ucw.cz>
 <f4137e34-c7fb-4f21-bc93-1496cbf61fdf@tuxedocomputers.com>
 <8096a042-83bd-4b9f-b633-79e86995c9b8@redhat.com>
 <f416fbca-589b-4f6a-aad6-323b66398273@tuxedocomputers.com>
 <4222268b-ff44-4b7d-bf11-e350594bbe24@redhat.com>
 <ac02143c-d417-49e5-9c6e-150cbda71ba7@tuxedocomputers.com>
 <ZaljwLe7P+dXHEHb@duo.ucw.cz>
 <6bbfdd62-e663-4a45-82f4-445069a8d690@redhat.com>
 <0cdb78b1-7763-4bb6-9582-d70577781e61@tuxedocomputers.com>
 <7228f2c6-fbdd-4e19-b703-103b8535d77d@redhat.com>
 <730bead8-6e1d-4d21-90d2-4ee73155887a@tuxedocomputers.com>
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <730bead8-6e1d-4d21-90d2-4ee73155887a@tuxedocomputers.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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