Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

From: Hans de Goede
Date: Wed Jun 06 2018 - 11:33:01 EST


Hi,

On 06-06-18 16:27, Benjamin Berg wrote:
Hi,

On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote:
On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede <hdegoede@xxxxxxxxxx>
wrote:
Hi,


On 05-06-18 12:46, Benjamin Berg wrote:

Hey,

On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:

On 05-06-18 12:14, Bastien Nocera wrote:

On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:

On 05-06-18 11:58, Bastien Nocera wrote:

[SNIP]


Ok, so what are you suggestion, do you really want to
hardcode
the cycle behavior in the kernel as these 2 patches are
doing,
without any option to intervene from userspace?

As mentioned before in the thread there are several example
of the kernel deciding to handle key-presses itself,
putting
policy in the kernel and they have all ended poorly (think
e.g. rfkill, acpi-video dealing with LC brightnesskey
presses
itself).

I guess one thing we could do here is code out both
solutions,
have a module option which controls if we:

1) Handle this in the kernel as these patches do
2) Or send a new KEY_KBDILLUMCYCLE event

Combined with a Kconfig option to select which is the
default
behavior. Then Endless can select 1 for now and then in
Fedora (which defaults to Wayland now) we could default to
2. once all the code for handling 2 is in place.

This is ugly (on the kernel side) but it might be the best
compromise we can do.


I don't really mind which option is used, I'm listing the
problems with
the different options. If you don't care about Xorg, then
definitely go
for adding a new key. Otherwise, processing it in the kernel
is the
least ugly, especially given that the key goes through the
same driver
that controls the brightness anyway. There's no crazy cross
driver
interaction as there was in the other cases you listed.


Unfortunately not caring about Xorg is not really an option.

Ok, new idea, how about we make g-s-d behavior upon detecting a
KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
toggle, otherwise do a cycle.

Or we could do this through hwdb, then we could add a hwdb entry
for this laptop setting the udev property to do a cycle instead of
a toggle on receiving the keypress.

If we are adding hwdb entries anyway to control the userspace
interpretation of the TOGGLE key, then we could also add the new CYCLE
key and explicitly re-map it to TOGGLE. That requires slightly more
logic in hwdb, but it does mean that we could theoretically just drop
the workaround if we ever stop caring about Xorg.

Hmm, interesting proposal, I say go for it :)


So maybe the next stop is that I can follow Darren's suggestion to eliminate
the is_kbd_led_event() and send a v2 for review?

I believe the best compromise we have right now is to do what Hans
suggested in an earlier proposal. That is implementing the two separate
behaviours in the kernel

1) handle this in the kernel as if the hardware changed it, and
2) send a new KEY_KBDILLUMCYCLE event [default].

I think you mean or, not and, depending on a module option the
code should do either 1) or 2) not both :)

Darren, Andy could you live with a module option for this?

Which one is used would be a compile time option for the kernel.

Then we have three different choices for handling these devices from a
userspace/distribution point of view:
1. Let the kernel handle these devices (quick fix)
2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE
(great if Xorg support is not a requirement)

Ack, although 2 will require some work in userspace, teach
all the layers like xkb about the new KEY_KBDILLUMCYCLE and
teach g-s-d to listen to it and do the right thing. But long
term 2. is the correct solution, so it would be good to start
working towards this.

3. For Xorg support:
- Add hwdb entry
- remap key to KEY_KBDILLUMTOGGLE
- set a flag on the keyboard
- detect the flag in userspace and handle KEY_KBDILLUMTOGGLE
as if KEY_KBDILLUMCYCLE was pressed
(yep, quite ugly)

I would just use 1. for Xorg compat and not bother with this mess.

Regards,

Hans