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

[Bastien Nocera]

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.