Hi,
On 05-06-18 12:14, Bastien Nocera wrote:
On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
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@r
edha
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.
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.
I guess alternatively I could live with hardcoding this in the
kernel as these 2 patches do, but that solves it just for *this*
laptop, I've a feeling that if we do that we end up with similar
code in all laptop vendor drivers under drivers/platform/x86
soon. Which really is the acpi_video.brightness_event thing
again, where the kernel would handle brightness key-presses
but only if the acpi_video backlight interface was in use
and not on models with a vendor specific or native-hardware
backlight driver. Hmm, so writing this, I'm still quite sure
the kernel approach is actually a bad idea.
Regards,
Hans
