Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
From: Hans de Goede
Date: Tue Jun 05 2018 - 06:05:47 EST
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@xxxxxxxxxxxxx>
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