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

[Hans de Goede]

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