Message-ID: <8d77246d-ec1f-4106-8a33-b6e93bdbab45@redhat.com>
Date: Wed, 17 Jan 2024 16:26:37 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Werner Sembach <wse@...edocomputers.com>, Pavel Machek <pavel@....cz>,
 Jani Nikula <jani.nikula@...ux.intel.com>, jikos@...nel.org,
 Jelle van der Waa <jelle@...aa.nl>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, Lee Jones
 <lee@...nel.org>, linux-kernel@...r.kernel.org,
 "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: Implement per-key keyboard backlight as auxdisplay?

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

Powered by blists - more mailing lists