Make asus-wmi notify on hotkey kbd brightness changes, listen for
brightness events and update the brightness directly in the driver.
For this purpose, bound check on brightness in kbd_led_set must be
based on the same data type to prevent illegal value been set.
Update the brightness by led_classdev_notify_brightness_hw_changed.
This will allow userspace to monitor (poll) for brightness changes
on the LED without reporting via input keymapping.
Signed-off-by: Chris Chiu <[email protected]>
---
drivers/platform/x86/asus-nb-wmi.c | 2 --
drivers/platform/x86/asus-wmi.c | 21 +++++++++++++++++++--
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 136ff2b4cce5..7ce80e4bb5a3 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
{ KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */
{ KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */
{ KE_KEY, 0xB5, { KEY_CALC } },
- { KE_KEY, 0xC4, { KEY_KBDILLUMUP } },
- { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
{ KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */
{ KE_END, 0},
};
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1f6e68f0b646..b4915b7718c1 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
+ led_classdev_notify_brightness_hw_changed(&asus->kbd_led, asus->kbd_led_wk);
}
static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
@@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
- if (value > asus->kbd_led.max_brightness)
+ if ((int)value > (int)asus->kbd_led.max_brightness)
value = asus->kbd_led.max_brightness;
- else if (value < 0)
+ else if ((int)value < 0)
value = 0;
asus->kbd_led_wk = value;
@@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
asus->kbd_led_wk = led_val;
asus->kbd_led.name = "asus::kbd_backlight";
+ asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
asus->kbd_led.brightness_set = kbd_led_set;
asus->kbd_led.brightness_get = kbd_led_get;
asus->kbd_led.max_brightness = 3;
@@ -1700,6 +1702,13 @@ static int is_display_toggle(int code)
return 0;
}
+static int is_kbd_led_event(int code)
+{
+ if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN)
+ return 1;
+ return 0;
+}
+
static void asus_wmi_notify(u32 value, void *context)
{
struct asus_wmi *asus = context;
@@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context)
}
}
+ if (is_kbd_led_event(code)) {
+ if (code == NOTIFY_KBD_BRTDWN)
+ kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
+ else
+ kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
+ goto exit;
+ }
+
if (is_display_toggle(code) &&
asus->driver->quirks->no_display_toggle)
goto exit;
--
2.11.0
Some ASUS laptops like UX550GE has hotkey (Fn+F7) for keyboard
backlight toggle which would emit the scan code 0xc7 each keypress.
On the UX550GE, the max keyboard brightness level is 3 so the
toggle would not be simply on/off the led but need to be cyclic.
Per ASUS spec, it should increment the brightness for each keypress,
then toggle(off) the LED when it already reached the max level.
Signed-off-by: Chris Chiu <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index b4915b7718c1..100e13e0817e 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -67,6 +67,7 @@ MODULE_LICENSE("GPL");
#define NOTIFY_BRNDOWN_MAX 0x2e
#define NOTIFY_KBD_BRTUP 0xc4
#define NOTIFY_KBD_BRTDWN 0xc5
+#define NOTIFY_KBD_BRTTOGGLE 0xc7
/* WMI Methods */
#define ASUS_WMI_METHODID_SPEC 0x43455053 /* BIOS SPECification */
@@ -1704,7 +1705,9 @@ static int is_display_toggle(int code)
static int is_kbd_led_event(int code)
{
- if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN)
+ if (code == NOTIFY_KBD_BRTUP ||
+ code == NOTIFY_KBD_BRTDWN ||
+ code == NOTIFY_KBD_BRTTOGGLE)
return 1;
return 0;
}
@@ -1755,7 +1758,10 @@ static void asus_wmi_notify(u32 value, void *context)
}
if (is_kbd_led_event(code)) {
- if (code == NOTIFY_KBD_BRTDWN)
+ if (code == NOTIFY_KBD_BRTTOGGLE &&
+ asus->kbd_led_wk == asus->kbd_led.max_brightness)
+ kbd_led_set(&asus->kbd_led, 0);
+ else if (code == NOTIFY_KBD_BRTDWN)
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
else
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
--
2.11.0
Hi Chris.
On 04-06-18 14:32, Chris Chiu wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.
> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.
>
> Update the brightness by led_classdev_notify_brightness_hw_changed.
> This will allow userspace to monitor (poll) for brightness changes
> on the LED without reporting via input keymapping.
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).
Regards,
Hans
>
> Signed-off-by: Chris Chiu <[email protected]>
> ---
> drivers/platform/x86/asus-nb-wmi.c | 2 --
> drivers/platform/x86/asus-wmi.c | 21 +++++++++++++++++++--
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
> index 136ff2b4cce5..7ce80e4bb5a3 100644
> --- a/drivers/platform/x86/asus-nb-wmi.c
> +++ b/drivers/platform/x86/asus-nb-wmi.c
> @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
> { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */
> { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */
> { KE_KEY, 0xB5, { KEY_CALC } },
> - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } },
> - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
> { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */
> { KE_END, 0},
> };
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..b4915b7718c1 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
> ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>
> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, asus->kbd_led_wk);
> }
>
> static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> - if (value > asus->kbd_led.max_brightness)
> + if ((int)value > (int)asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
> - else if (value < 0)
> + else if ((int)value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>
> asus->kbd_led_wk = led_val;
> asus->kbd_led.name = "asus::kbd_backlight";
> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> asus->kbd_led.brightness_set = kbd_led_set;
> asus->kbd_led.brightness_get = kbd_led_get;
> asus->kbd_led.max_brightness = 3;
> @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code)
> return 0;
> }
>
> +static int is_kbd_led_event(int code)
> +{
> + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN)
> + return 1;
> + return 0;
> +}
> +
> static void asus_wmi_notify(u32 value, void *context)
> {
> struct asus_wmi *asus = context;
> @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context)
> }
> }
>
> + if (is_kbd_led_event(code)) {
> + if (code == NOTIFY_KBD_BRTDWN)
> + kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
> + else
> + kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
> + goto exit;
> + }
> +
> if (is_display_toggle(code) &&
> asus->driver->quirks->no_display_toggle)
> goto exit;
>
On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <[email protected]> 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?
Daniel
Hi,
On 04-06-18 15:51, Daniel Drake wrote:
> On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <[email protected]> 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.
###
With that all said lets look at a userspace solution grepping
through the kernel for KEY_KBDILLUMTOGGLE there are 4 users:
1) drivers/platform/x86/toshiba_acpi.c
I don't know how the key on Toshiba's behaves on models where
it is hardwired / under Windows
2) drivers/platform/x86/dell-wmi.c
All Dells I know of have the hotkey do a cycle, not a toggle on/off
3) Some apple specific drivers:
According to:
https://support.apple.com/en-us/HT202310
Apple has separate up/down keys, so no idea why the drivers sometimes
send a KEY_KBDILLUMTOGGLE event
4) drivers/hid/hid-input.c
Okay, so the HUT clearly defines the Consumer Page mapping 0x35
as "OOC – Toggles illumination of consumer control's buttons and controls on/off."
which is a bit of a bummer, because I was hoping that we
could just redefine KEY_KBDILLUMTOGGLE to mean cycle.
Still despite HID HUT being clear about this being an on/off
toggle. I'm thinking that cycling makes more sense everywhere
and that we should simply rename
gnome-settings-daemon/plugins/power/gsd-power-manager.c 's
upower_kbd_toggle() function to upower_kbd_cycle() and make
it behave accordingly. If we device the range up into 8 steps
(if there are more then 8 to being with) then I think this
will be more useful everywhere then just the on/off toggle.
The alternative would be to define a new keycode, but that will be
> 240 so it will only work on Wayland and not under X11.
Benjamin, Bastien, opinions?
Regards,
Hans
Hi there
Let me add my two cents on the Toshiba side.
2018-06-04 8:23 GMT-06:00 Hans de Goede <[email protected]>:
>
> 1) drivers/platform/x86/toshiba_acpi.c
>
> I don't know how the key on Toshiba's behaves on models where
> it is hardwired / under Windows
With Toshiba we have two types of hardware implementations:
1st gen keyboards, supporting AUTO and FN-Z
AUTO - Turns on/off automatically after some (configurable) time
FN-Z - Creates "toshiba::kbd_backlight" and it's toggled by userspace
2nd gen keyborads, supporting AUTO, ON and OFF
AUTO - Ditto
ON - Always on
OFF - Always off
The second gen keyboards are completely driven by hardware,
userspace must be checking sysfs for "kbd_backlight_mode"
changes, however, the Toshiba interface emits the 0x92
ACPI event when we have a kbd mode change, but it's not
currently being transmitted to userspace via netlink.
Saludos
Azael
--
-- El mundo apesta y vosotros apestais tambien --
On Mon, Jun 04, 2018 at 08:32:37PM +0800, Chris Chiu wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.
> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.
>
> Update the brightness by led_classdev_notify_brightness_hw_changed.
> This will allow userspace to monitor (poll) for brightness changes
> on the LED without reporting via input keymapping.
>
> Signed-off-by: Chris Chiu <[email protected]>
> ---
> drivers/platform/x86/asus-nb-wmi.c | 2 --
> drivers/platform/x86/asus-wmi.c | 21 +++++++++++++++++++--
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
> index 136ff2b4cce5..7ce80e4bb5a3 100644
> --- a/drivers/platform/x86/asus-nb-wmi.c
> +++ b/drivers/platform/x86/asus-nb-wmi.c
> @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
> { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */
> { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */
> { KE_KEY, 0xB5, { KEY_CALC } },
> - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } },
> - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
> { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */
> { KE_END, 0},
> };
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..b4915b7718c1 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
> ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>
> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, asus->kbd_led_wk);
> }
>
> static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> - if (value > asus->kbd_led.max_brightness)
> + if ((int)value > (int)asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
> - else if (value < 0)
> + else if ((int)value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>
> asus->kbd_led_wk = led_val;
> asus->kbd_led.name = "asus::kbd_backlight";
> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> asus->kbd_led.brightness_set = kbd_led_set;
> asus->kbd_led.brightness_get = kbd_led_get;
> asus->kbd_led.max_brightness = 3;
> @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code)
> return 0;
> }
>
> +static int is_kbd_led_event(int code)
> +{
> + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN)
> + return 1;
> + return 0;
I don't think this is worth breaking out into a separate function. This
isn't exactly a hot path, but I don't think this makes the code any more
readable really. The is_display_toggle was comparing 8 values in a
complex logic statement, so we don't need to follow that for something
this simple.
> +}
> +
> static void asus_wmi_notify(u32 value, void *context)
> {
> struct asus_wmi *asus = context;
> @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context)
> }
> }
>
> + if (is_kbd_led_event(code)) {
> + if (code == NOTIFY_KBD_BRTDWN)
Seems like we could eliminate the extra function and eliminate a level
of indentation by rewriting this as:
if (code == NOTIFY_KBD_BRTDWN)
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
else if (code == NOTIFY_KBD_BRTUP)
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
if (code == NOTIFY_KBD_BRTDWN || NOTIFY_KBD_BRTUP)
goto exit;
Or, just treat them as separate events:
if (code == NOTIFY_KBD_BRTDWN) {
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk - 1);
goto exit;
}
if (code == NOTIFY_KBD_BRTUP) {
kbd_led_set(&asus->kbd_led, asus->kbd_led_wk + 1);
goto exit;
}
> if (is_display_toggle(code) &&
> asus->driver->quirks->no_display_toggle)
> goto exit;
> --
> 2.11.0
>
>
--
Darren Hart
VMware Open Source Technology Center
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 <[email protected]> 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
On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <[email protected]> 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 <[email protected]> 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?
Hi,
On 05-06-18 04:31, Darren Hart 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 <[email protected]> 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?
I was thinking about doing that as a workaround myself, but I'm not sure
that is a good idea (because of e.g. races with userspace auto-correcting
the brightness based on ambient light and/or keyboard activity).
As I see it 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.
2) Add a new KEY_KBDILLUMCYCLE event and send that for the TOGGLE code
on these laptops.
Regards,
Hans
*) Rather then separate up / down hotkeys
>
> Something like:
>
> if (code == TOGGLE && ledval < ledmax)
> code = UP;
>
> sparse_keymap_report_event(..., code, ...)
>
> }
>
Hi,
On 05-06-18 05:18, Chris Chiu wrote:
> On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <[email protected]> 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 <[email protected]> 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.
2) Add a new KEY_KBDILLUMCYCLE event and send that for the TOGGLE code
on these laptops.
Yes both will take time to get into end-users hand, but so will a
kernel-only solution. In the mean time endless can always carry
downstream patches to make things work right now (while waiting for
the changes to trickle down from upstream).
Regards,
Hans
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 <[email protected]>
>>> 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
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 <[email protected]>
> > 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.
> and send that for the TOGGLE code
> on these laptops.
>
> Yes both will take time to get into end-users hand, but so will a
> kernel-only solution. In the mean time endless can always carry
> downstream patches to make things work right now (while waiting for
> the changes to trickle down from upstream).
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.
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
Hey,
On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> On 05-06-18 12:14, Bastien Nocera wrote:
> > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > [SNIP]
> > >
> > > 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.
If we are adding hwdb entries anyway to control the userspace
interpretation of the TOGGLE key, then we could also add the new CYCLE
key and explicitly re-map it to TOGGLE. That requires slightly more
logic in hwdb, but it does mean that we could theoretically just drop
the workaround if we ever stop caring about Xorg.
> 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.
Benjamin
Hi,
On 05-06-18 12:46, Benjamin Berg wrote:
> Hey,
>
> On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
>> On 05-06-18 12:14, Bastien Nocera wrote:
>>> On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
>>>> On 05-06-18 11:58, Bastien Nocera wrote:
>>>>> [SNIP]
>>>>
>>>> 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.
>
> If we are adding hwdb entries anyway to control the userspace
> interpretation of the TOGGLE key, then we could also add the new CYCLE
> key and explicitly re-map it to TOGGLE. That requires slightly more
> logic in hwdb, but it does mean that we could theoretically just drop
> the workaround if we ever stop caring about Xorg.
Hmm, interesting proposal, I say go for it :)
Regards,
Hans
On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede <[email protected]> wrote:
> Hi,
>
>
> On 05-06-18 12:46, Benjamin Berg wrote:
>>
>> Hey,
>>
>> On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
>>>
>>> On 05-06-18 12:14, Bastien Nocera wrote:
>>>>
>>>> On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
>>>>>
>>>>> On 05-06-18 11:58, Bastien Nocera wrote:
>>>>>>
>>>>>> [SNIP]
>>>>>
>>>>>
>>>>> 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.
>>
>>
>> If we are adding hwdb entries anyway to control the userspace
>> interpretation of the TOGGLE key, then we could also add the new CYCLE
>> key and explicitly re-map it to TOGGLE. That requires slightly more
>> logic in hwdb, but it does mean that we could theoretically just drop
>> the workaround if we ever stop caring about Xorg.
>
>
> Hmm, interesting proposal, I say go for it :)
>
> Regards,
>
> Hans
>
>
>
So maybe the next stop is that I can follow Darren's suggestion to eliminate
the is_kbd_led_event() and send a v2 for review?
Hi,
On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote:
> On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede <[email protected]>
> wrote:
> > Hi,
> >
> >
> > On 05-06-18 12:46, Benjamin Berg wrote:
> > >
> > > Hey,
> > >
> > > On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
> > > >
> > > > On 05-06-18 12:14, Bastien Nocera wrote:
> > > > >
> > > > > On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> > > > > >
> > > > > > On 05-06-18 11:58, Bastien Nocera wrote:
> > > > > > >
> > > > > > > [SNIP]
> > > > > >
> > > > > >
> > > > > > 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.
> > >
> > > If we are adding hwdb entries anyway to control the userspace
> > > interpretation of the TOGGLE key, then we could also add the new CYCLE
> > > key and explicitly re-map it to TOGGLE. That requires slightly more
> > > logic in hwdb, but it does mean that we could theoretically just drop
> > > the workaround if we ever stop caring about Xorg.
> >
> > Hmm, interesting proposal, I say go for it :)
> >
>
> So maybe the next stop is that I can follow Darren's suggestion to eliminate
> the is_kbd_led_event() and send a v2 for review?
I believe the best compromise we have right now is to do what Hans
suggested in an earlier proposal. That is implementing the two separate
behaviours in the kernel
1) handle this in the kernel as if the hardware changed it, and
2) send a new KEY_KBDILLUMCYCLE event [default].
Which one is used would be a compile time option for the kernel.
Then we have three different choices for handling these devices from a
userspace/distribution point of view:
1. Let the kernel handle these devices (quick fix)
2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE
(great if Xorg support is not a requirement)
3. For Xorg support:
- Add hwdb entry
- remap key to KEY_KBDILLUMTOGGLE
- set a flag on the keyboard
- detect the flag in userspace and handle KEY_KBDILLUMTOGGLE
as if KEY_KBDILLUMCYCLE was pressed
(yep, quite ugly)
The "beauty" of this approach is that the workaround from option 3 can
be simply removed again if we stop caring about Xorg (or should we find
a solution to handle high keycodes in Xorg).
Benjamin
Hi,
On 06-06-18 16:27, Benjamin Berg wrote:
> Hi,
>
> On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote:
>> On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede <[email protected]>
>> wrote:
>>> Hi,
>>>
>>>
>>> On 05-06-18 12:46, Benjamin Berg wrote:
>>>>
>>>> Hey,
>>>>
>>>> On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
>>>>>
>>>>> On 05-06-18 12:14, Bastien Nocera wrote:
>>>>>>
>>>>>> On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
>>>>>>>
>>>>>>> On 05-06-18 11:58, Bastien Nocera wrote:
>>>>>>>>
>>>>>>>> [SNIP]
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> If we are adding hwdb entries anyway to control the userspace
>>>> interpretation of the TOGGLE key, then we could also add the new CYCLE
>>>> key and explicitly re-map it to TOGGLE. That requires slightly more
>>>> logic in hwdb, but it does mean that we could theoretically just drop
>>>> the workaround if we ever stop caring about Xorg.
>>>
>>> Hmm, interesting proposal, I say go for it :)
>>>
>>
>> So maybe the next stop is that I can follow Darren's suggestion to eliminate
>> the is_kbd_led_event() and send a v2 for review?
>
> I believe the best compromise we have right now is to do what Hans
> suggested in an earlier proposal. That is implementing the two separate
> behaviours in the kernel
>
> 1) handle this in the kernel as if the hardware changed it, and
> 2) send a new KEY_KBDILLUMCYCLE event [default].
I think you mean or, not and, depending on a module option the
code should do either 1) or 2) not both :)
Darren, Andy could you live with a module option for this?
> Which one is used would be a compile time option for the kernel.
>
> Then we have three different choices for handling these devices from a
> userspace/distribution point of view:
> 1. Let the kernel handle these devices (quick fix)
> 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE
> (great if Xorg support is not a requirement)
Ack, although 2 will require some work in userspace, teach
all the layers like xkb about the new KEY_KBDILLUMCYCLE and
teach g-s-d to listen to it and do the right thing. But long
term 2. is the correct solution, so it would be good to start
working towards this.
> 3. For Xorg support:
> - Add hwdb entry
> - remap key to KEY_KBDILLUMTOGGLE
> - set a flag on the keyboard
> - detect the flag in userspace and handle KEY_KBDILLUMTOGGLE
> as if KEY_KBDILLUMCYCLE was pressed
> (yep, quite ugly)
I would just use 1. for Xorg compat and not bother with this mess.
Regards,
Hans
On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote:
> > > > > If we are adding hwdb entries anyway to control the userspace
> > > > > interpretation of the TOGGLE key, then we could also add the new CYCLE
> > > > > key and explicitly re-map it to TOGGLE. That requires slightly more
> > > > > logic in hwdb, but it does mean that we could theoretically just drop
> > > > > the workaround if we ever stop caring about Xorg.
> > > >
> > > > Hmm, interesting proposal, I say go for it :)
> > > >
> > >
> > > So maybe the next stop is that I can follow Darren's suggestion to eliminate
> > > the is_kbd_led_event() and send a v2 for review?
> >
> > I believe the best compromise we have right now is to do what Hans
> > suggested in an earlier proposal. That is implementing the two separate
> > behaviours in the kernel
> >
> > 1) handle this in the kernel as if the hardware changed it, and
> > 2) send a new KEY_KBDILLUMCYCLE event [default].
>
> I think you mean or, not and, depending on a module option the
> code should do either 1) or 2) not both :)
>
> Darren, Andy could you live with a module option for this?
We are of course strongly opposed to adding module options.
I agree we can't ignore Xorg.
I agree policy in general should not be in the kernel.
I also see many of these drivers as the last mile to getting a platform
fully working. If there is a place for one-off fixes, it's in these
drivers. I'd love to refactor and use proper abstractions and all that
as the patterns make those abstractions clear - but I don't want to
delay getting something working waiting for the ideal solution.
So I have two questions I'd like to confirm before saying "OK" to a
module option.
1) Hans I think you said that doing the code conversion from TOGGLE to
UP based on the LED value and the max value was racy with userspace.
What is the failure mode here? Is it not easily recoverable? And how do
I enter it? Do I have to simultaneously modify the software brightness
control AND press the keyboard brightness control? How practical is
that? If recoverable AND hard to trigger, I think there is value in the
very simple 3 level brightness cycle being handled in the kernel.
2) Why is a module option preferable to a compile time option? It seems
to me the policy will be largely distro dependent, and the same kernel
needing to support both modes seems likely to be pretty rare.
--
Darren Hart
VMware Open Source Technology Center
Hi,
On 09-06-18 02:33, Darren Hart wrote:
> On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote:
>
>>>>>> If we are adding hwdb entries anyway to control the userspace
>>>>>> interpretation of the TOGGLE key, then we could also add the new CYCLE
>>>>>> key and explicitly re-map it to TOGGLE. That requires slightly more
>>>>>> logic in hwdb, but it does mean that we could theoretically just drop
>>>>>> the workaround if we ever stop caring about Xorg.
>>>>>
>>>>> Hmm, interesting proposal, I say go for it :)
>>>>>
>>>>
>>>> So maybe the next stop is that I can follow Darren's suggestion to eliminate
>>>> the is_kbd_led_event() and send a v2 for review?
>>>
>>> I believe the best compromise we have right now is to do what Hans
>>> suggested in an earlier proposal. That is implementing the two separate
>>> behaviours in the kernel
>>>
>>> 1) handle this in the kernel as if the hardware changed it, and
>>> 2) send a new KEY_KBDILLUMCYCLE event [default].
>>
>> I think you mean or, not and, depending on a module option the
>> code should do either 1) or 2) not both :)
>>
>> Darren, Andy could you live with a module option for this?
>
> We are of course strongly opposed to adding module options.
>
> I agree we can't ignore Xorg.
>
> I agree policy in general should not be in the kernel.
>
> I also see many of these drivers as the last mile to getting a platform
> fully working. If there is a place for one-off fixes, it's in these
> drivers. I'd love to refactor and use proper abstractions and all that
> as the patterns make those abstractions clear - but I don't want to
> delay getting something working waiting for the ideal solution.
>
> So I have two questions I'd like to confirm before saying "OK" to a
> module option.
>
> 1) Hans I think you said that doing the code conversion from TOGGLE to
> UP based on the LED value and the max value was racy with userspace.
> What is the failure mode here? Is it not easily recoverable? And how do
> I enter it?
g-s-d can currently already auto-dim the brightness of the
kbd backlight when idle (if there are enough brightness levels).
Lets say that the brightness is at its highest setting and the user
wants to cycle the backlight to off.
But just before the user hits the cycle key, the timeout expires
and userspace dimms the backlight, now the kernel processes
the cycle key event, sees it is not max and sends an up, instead
of the expected toggle.
Note we already have this problem on machines where the cycle
behavior is implemented in the firmware/hardware rather then
inside the kernel.
A bigger problem with sending up-up-up-toggle is that g-s-d saves
the current brightness (max) on the first toggle and restores that
on the second toggle, so the second up-up-up-toggle sequence we
end up restoring the max, going from max to max on the toggle.
So the user needs to press toggle twice at max brightness to get
to off (and then once for the next cycle, then 2 times again for
the cycle after that, etc.).
So I think it is fair to say that sending up-up-up-toggle is not a
good idea.
> Do I have to simultaneously modify the software brightness
> control AND press the keyboard brightness control? How practical is
> that? If recoverable AND hard to trigger, I think there is value in the
> very simple 3 level brightness cycle being handled in the kernel.
>
> 2) Why is a module option preferable to a compile time option? It seems
> to me the policy will be largely distro dependent, and the same kernel
> needing to support both modes seems likely to be pretty rare.
Because lets say that we have everything in place in a recent Fedora
to handle the cycle-key in userspace, so we have a mapping for the
new event at all levels and g-s-d code to handle it. Then this will
still only work for GNOME3 and possibly other wayland based desktop
environments. While some of our users will keep using the X11 based
XFCE or mate desktop environments.
So what we actually want is a module option, with a configurable
default. So that we can make the default send the cycle event in
a future Fedora, while XFCE / mate users can override the default
using the module option.
###
So typing all of the above has made me think about this once more.
Specifically about how most popular brands handle the cycle behavior
in firmware/hardware already and that userspace already needs to
deal with this and that sofar this does not seem to be a problem.
Combine this with the ugliness of adding a module option +
adding a new cycle input event requiring a lot of work at various
layers to actually work and I think that taking this series as
is, is not so bad. Esp. since I don't see anyone doing this work
soon.
This comes down to faking the cycling being done inside the firmware/
hardware as e.g. Thinkpads and the Dell XPS series actually do,
so, as said, userspace already needs to deal with this.
If we in the future actually get around to implementing a kbd-illum-cycle
input event and have userspace support in place we can always add the
module option then.
TL;DR: lets just go with this series as is for now, we can always
add a module option to opt-out of handling this in the kernel and
sending a new cycle input event later.
Regards,
Hans