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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 05-06-18 11:58, Bastien Nocera wrote:
> On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 05-06-18 05:18, Chris Chiu wrote:
>>> On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <dvhart@infradead.org>
>>> wrote:
>>>> On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 04-06-18 15:51, Daniel Drake wrote:
>>>>>> On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <hdegoede@redha
>>>>>> t.com> wrote:
>>>>>>> Is this really a case of the hardware itself processing the
>>>>>>> keypress and then changing the brightness *itself* ?
>>>>>>>
>>>>>>>    From the "[PATCH 2/2] platform/x86: asus-wmi: Add
>>>>>>> keyboard backlight
>>>>>>> toggle support" patch I get the impression that the driver
>>>>>>> is
>>>>>>> modifying the brightness from within the kernel rather then
>>>>>>> the
>>>>>>> keyboard controller are ACPI embeddec-controller doing it
>>>>>>> itself.
>>>>>>>
>>>>>>> If that is the case then the right fix is for the driver to
>>>>>>> stop
>>>>>>> mucking with the brighness itself, it should simply report
>>>>>>> the
>>>>>>> right keyboard events and export a led interface and then
>>>>>>> userspace
>>>>>>> will do the right thing (and be able to offer flexible
>>>>>>> policies
>>>>>>> to the user).
>>>>>>
>>>>>> Before this modification, the driver reports the brightness
>>>>>> keypresses
>>>>>> to userspace and then userspace can respond by changing the
>>>>>> brightness
>>>>>> level, as you describe.
>>>>>>
>>>>>> You are right in that the hardware doesn't change the
>>>>>> brightness
>>>>>> directly itself, which is the normal usage of
>>>>>> LED_BRIGHT_HW_CHANGED.
>>>>>>
>>>>>> However this approach was suggested by Benjamin Berg and
>>>>>> Bastien
>>>>>> Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi:
>>>>>> Add
>>>>>> keyboard backlight toggle support
>>>>>> https://marc.info/?l=linux-kernel&m=152639169210655&w=2
>>>>>>
>>>>>> The issue is that we need to support a new "keyboard
>>>>>> backlight
>>>>>> brightness cycle" key (in the patch that follows this one)
>>>>>> which
>>>>>> doesn't fit into any definitions of keys recognised by the
>>>>>> kernel and
>>>>>> likewise there's no userspace code to handle it.
>>>>>>
>>>>>> If preferred we could leave the standard brightness keys
>>>>>> behaving as
>>>>>> they are (input events) and make the new special key type
>>>>>> directly
>>>>>> handled by the kernel?
>>>>>
>>>>> I'm sorry that Benjamin and Bastien steered you in this
>>>>> direction,
>>>>> IMHO none of it should be handled in the kernel.
>>>>>
>>>>> Anytime any sort of input is directly responded to by the
>>>>> kernel
>>>>> it is a huge PITA to deal with from userspace. The kernel will
>>>>> have
>>>>> a simplistic implementation which almost always is wrong.
>>>>>
>>>>> Benjamin, remember the pain we went through with rfkill hotkey
>>>>> presses being handled in the kernel ?
>>>>>
>>>>> And then there is the whole
>>>>> acpi_video.brightness_switch_enabled
>>>>> debacle, which is an option which defaults to true which causes
>>>>> the kernel to handle LCD brightness key presses, which all
>>>>> distros
>>>>> have been patching to default to off for ages.
>>>>>
>>>>> To give a concrete example, we may want to implement software
>>>>> dimming / auto-off of the kbd backlight when the no keys are
>>>>> touched for x seconds. This would seriously get in the way of
>>>>> that.
>>>>>
>>>>> So sorry, but NACK to this series.
>>>>
>>>> So if instead of modifying the LED value, the kernel platform
>>>> drivers
>>>> converted the TOGGLE into a cycle even by converting to an UP
>>>> event
>>>> based on awareness of the plaform specific max value and the read
>>>> current value, leaving userspace to act on the TOGGLE/UP events -
>>>> would
>>>> that be preferable?
>>>>
>>>> Something like:
>>>>
>>>>           if (code == TOGGLE && ledval < ledmax)
>>>>                   code = UP;
>>>>
>>>>           sparse_keymap_report_event(..., code, ...)
>>>>
>>>> }
>>>> --
>>>> Darren Hart
>>>> VMware Open Source Technology Center
>>>
>>> That's what I was trying to do in  [PATCH v2] platform/x86: asus-
>>> wmi: Add
>>> keyboard backlight toggle support. However, that brought another
>>> problem
>>> discussed in the thread.
>>> https://marc.info/?l=linux-kernel&m=152639169210655&w=2
>>>
>>> So I moved the brightness change in the driver without passing to
>>> userspace.
>>> Per Hans, seems there're some other concerns and I also wonder if
>>> the
>>> TOGGLE event happens in ASUS HID (asus-hid.c) which also convert
>>> and
>>> pass the keycode to userspace but no TOGGLE key support yet What
>>> should
>>> we do then?
>>
>> As I mentioned in my reply to Darren, there are 2 proper solutions to
>> this:
>>
>> 1) Make userspace treat KEY_KBDILLUMTOGGLE as a cycle key, this is
>> what the kbd-backlight on most laptops with a single hotkey (*) does
>> in cases where this is handled in firmware, rather then left to the
>> OS. The handled in firmware is the case which I created the
>> led_classdev_notify_brightness_hw_changed() API for. This would be
>> my preferred solution and I believe that Benjamin is discussing this
>> with Bastien ATM.
> 
> It isn't on Macs, at least. Toggle is a toggle, not a cycle key. It
> turns the keyboard backlight off and on, restoring the backlight level
> when turned back on.
> 
>> 2) Add a new KEY_KBDILLUMCYCLE event
> 
> Which won't be accessible to Xorg.

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.

Regards,

Hans