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

[Benjamin Berg]

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].

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)
 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)

The "beauty" of this approach is that the workaround from option 3 can
be simply removed again if we stop caring about Xorg (or should we find
a solution to handle high keycodes in Xorg).

Benjamin

Attachment: signature.asc
Description: This is a digitally signed message part