Messages in this thread | | | Subject | Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change | From | Hans de Goede <> | Date | Tue, 5 Jun 2018 12:05:37 +0200 |
| |
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
| |