This would be a bad hack to workaround a kernel bug. IMHO the asus WMI driver should be fixed to actually emit proper key events, then user space will handle this just fine.

I leave the decision to close the bug to the maintainers, though.

As Martin said, why can't the Asus WMI driver pass on the key event, and have the brightness handling done separately? Unless there's a technical reason for user-space having to jump through hoopses, I'd rather the driver be fixed, and GNOME handling this automatically.

Recent linux kernel seems to send brightness change uevent on every backlight device drivers.
But it is not to send brightness up/down key event on some backlight drivers such as asus-wmi.
As I know, thinkpad_acpi also sends brightness change uevent.
If all backlight device drivers send brightness change uevent, why don't we just implement by this way for better compatibility?

Whether or not they send out uvevents isn't the problem (That means that upower doesn't need to poll for brightness changes, that's nice but not the point). The problem is that the Asus WMI driver handles the keyboard presses itself instead of letting user-space handle it.

Why can't the Asus WMI driver pass the key press to user-space instead of handling it itself?

It's not a big deal to send out the brightness adjustment key event from the Asus WMI driver, but using uevent is a more generic way to notify userspace app that brightness is changed. The uevent is not only send out from WMI drivers, but from ACPI video driver.

And we are suffering from the brightness be changed twice problem by a single key stroke for a long time, that's because of the keyevents. EC reports the keyevent, so g-s-d adjusts the brightness, and ACPI video driver changes the brightness again, if the BIOS supports ACPI brightness functions.
The dirty hack we can only use to fix this issue is to avoid ACPI video driver to change the brightness and pack the dirty code becomes a dkms package, but mostly we do nothing, there is no easy and acceptable way to fix it.
And WMI drivers will try not send out the brightness keyevent or try to do nothing when receives the keyevent if it supports ACPI brightness functions.

Please consider to add this feature into g-s-d, so that WMI drivers can get rid of this issue and those dirty code.

FYI.
https://lwn.net/Articles/341289/

ubuntu@ubuntu-Vostro-V131 ~/workspace/linux-2.6/drivers (git)-[platform_next] % grep -rn backlight_force_update {platform/x86/*,acpi/*}
platform/x86/asus-laptop.c:784: backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
platform/x86/asus-wmi.c:1191:   backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
platform/x86/eeepc-laptop.c:1165:       backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
platform/x86/ideapad-laptop.c:683:      backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY);
platform/x86/thinkpad_acpi.c:6095:      backlight_force_update(ibm_backlight_device,
platform/x86/thinkpad_acpi.c:6403:              backlight_force_update(ibm_backlight_device,
acpi/video.c:1264:              backlight_force_update(device->backlight,

(In reply to comment #4)
> Whether or not they send out uvevents isn't the problem (That means that upower
> doesn't need to poll for brightness changes, that's nice but not the point).
> The problem is that the Asus WMI driver handles the keyboard presses itself
> instead of letting user-space handle it.
> 
> Why can't the Asus WMI driver pass the key press to user-space instead of
> handling it itself?

Because what not how it *should be done*. Keys are not notifications at all. Keys are emitted if a user press something that should be interpreted by userspace as "in reponse to this key, please adjust the brightness".

In this case, what we get from the hardware is only a brightness change notification, and it's what we will relay to userspace. It's just how the Linux backlight class works. And, really, the OSD should only pop in response to backlight notification, and never in response to keys, keys should only be used to adjust the backlight (which will pop a notification by itself).

Created attachment 212764 [details] [review]
proposed patch to listen to the uevent for the brigthtness OSD

IMHO, it's a reasonable behavior for the platform drivers not to emit keyevents especially when the hardware already takes the job, and the driver can do nothing about it.

(In reply to comment #7)
> Created an attachment (id=212764) [details] [review]
> proposed patch to listen to the uevent for the brigthtness OSD
> 
> IMHO, it's a reasonable behavior for the platform drivers not to emit keyevents
> especially when the hardware already takes the job, and the driver can do
> nothing about it.

Agreed!

Hello, is there any update for this issue?

(In reply to comment #7)
> IMHO, it's a reasonable behavior for the platform drivers not to emit keyevents
> especially when the hardware already takes the job, and the driver can do
> nothing about it.

No, I don't like this approach. What's supposed to happen is for the driver to send the keypress event, and then let the session perform the action. It could be that the keypress needs to increase the brightness by 15%, rather than just one level, for instance where the hardware has a lot of output states. If the hardware wants to do it itself, then it shouldn't send key events and g-s-d doesn't get involved.

By mixing the uevents and the keypresses we get into the same muddle we had with HAL, i.e. nobody is sure what events mean what, and what state the hardware is in. Some drivers also send uevents *and* keys and this patch is going to cause a double event for those. I think it makes much more sense to just fix the kernel driver, sorry.

Richard Hughes, (In reply to comment #10)
> (In reply to comment #7)
> > IMHO, it's a reasonable behavior for the platform drivers not to emit keyevents
> > especially when the hardware already takes the job, and the driver can do
> > nothing about it.
> 
> No, I don't like this approach. What's supposed to happen is for the driver to
> send the keypress event, and then let the session perform the action. It could
> be that the keypress needs to increase the brightness by 15%, rather than just
> one level, for instance where the hardware has a lot of output states. If the
> hardware wants to do it itself, then it shouldn't send key events and g-s-d
> doesn't get involved.

Here is how it works currently, and that's how the kernel work, you can't change that (well you can, but go ask kernel dev).
- If the hardware report keypress, the driver will relay keys to userspace
- If the hardware handle keys itself, but send change notification, the driver will send an uevent (telling it was probably comming from a key)
- Each time the brightness is changed using sysfs, an uevent is sent.

Here is how userspace should handle that:
- Keypress should be handled, and sysfs backlight interface should be used to adapt the backlight, but nothing should be displayed here since you can't know if the brightness was really changed
- Each time an uevent is received (reflecting a hardware or sysfs change), a progress bar should be displayed to the user reflecting the current state of the backlight.

> By mixing the uevents and the keypresses we get into the same muddle we had
> with HAL, i.e. nobody is sure what events mean what, and what state the
> hardware is in. Some drivers also send uevents *and* keys and this patch is
> going to cause a double event for those. I think it makes much more sense to
> just fix the kernel driver, sorry.

Asus kernel drivers are not broken in this case, there are doing exactly what there are supposed to do, and uevents for backlight are well defined:
- If a keypress is received, do something: change the backlight, but display nothing
- If an uevent is received, it's just a notification of the current backlight state, you should do nothing except reporting the state to the user

If a kernel driver is broken, please report the bug, and we will try to fix it.

Key events are *NOT* supposed to be backlight change notifcations, they are only *KEYS*

For more informations, take a look at backlight_force_update() https://github.com/torvalds/linux/blob/master/drivers/video/backlight/backlight.c#L256 which is part of the backlight subsystem.

This function has to be called if a platform driver detects that hardware has changed the backlight state. It's how the backlight interface work, it's not a bug.

Note that if you reall don't want to use uevent, another real solution to check if the brightness has changed is simply to use inotify or similar on 'actual_brightness' file (*NOT* 'brightness' !). But again, keys have nothing to do with notifications, they are just keys.

Corentin, this isn't however how the user expects things to be handled, or how it was implemented.

When a key is pressed, there should be a notification. Which means that we cannot wait for the kernel to do something about it. We assume (and hope) that it worked. It's a lack of error handling that I'm perfectly happy with.

We will not show the OSD based on kernel events, because we cannot be sure that the event will have been generated in response to an action by the user.

Hence, the kernel driver works in a different way to all the other brightness drivers, and we're not going to add specific code for it.

I'm afraid you'll have to go and find a kernel developer with access to the hardware, so that they can change and test the driver to not behave like the user-space can't handle brightness keys. Not everyone runs window managers from the 90's as a desktop environment, and there's behaviour expectations that can't be worked around.

Let me try to re-explain.

The initial description may be confusing. The WMI driver receive an hardware notification, not a keycode.

Keys will be sent only when the driver is *sure* that a key was pressed.

In most case: intel, nvidia_bl, radeon, acpi/video, the driver has no clue about keys, all it will do is update the backlight interface (not the backlight ! just the sysfs interface) if they receive an hardware event telling them they should update it.

Platform drivers like asus/eeepc drivers will only handle the backlight if acpi_video=vendor is set. And even in this case there is no way to tell if the received event is a keypress, or just a BIOS notification that the backlight has been changed, and most of the time the backlight is handled in the BIOS.

When the BIOS (or any other firmware) already updated the backlight, sending a keypress is a bug, because user-space will handle it and update the backlight again.

If you want to monitor actual key press, and execute action, some drivers will be able to provide that if they think it's a keypress and not juste a brightness change notification.

If you want to monitor backlight changes, monitor uevents, this *is* the official kernel interface for backlight change *notifications*.

I think I understood correctly the first time around. The BIOS/firmware shouldn't be updating the brightness in response to user key presses. Key presses should go to user-space. I mean, if I wanted to reassign the brightness keys to start the video of a singing cat, the BIOS shouldn't be changing the brightness anyway.

If you can, in the driver, make the BIOS stop acting on the key presses, and send them all to user space. User-space will handle them.

No, some brightness adjustments are directly made by BIOS itself, and they are not controllable by kernel modules.

(In reply to comment #16)
> No, some brightness adjustments are directly made by BIOS itself, and they are
> not controllable by kernel modules.

In which cases would it do that?

For example, there is an all-in-one ASUS PC which has physical brightness up/down buttons on the side of the monitor.
When we use them to adjust brightness, BIOS will inform the backlight controller of monitor to adjust brightness directly and then it will use wmi to inform OS about the brightness changes.
So OS will not receive brightness button event, but brightness change event instead.

(In reply to comment #17)
> (In reply to comment #16)
> > No, some brightness adjustments are directly made by BIOS itself, and they are
> > not controllable by kernel modules.
> 
> In which cases would it do that?

And well we got BIOS that intercept keypress, but graphic cards are also exporting backlight interfaces these days. And there is also ACPI backlight. For those there is no way you'll get a keypress if the backlight is changed (for whatever reason, sysfs or hardware).

That uevent source field can contain source= sysfs, hotkey or unknown, depending on how much the driver knows.

We do not care about notifications. Really. We don't care. You have them, fine, we won't use them.

Just make sure 1) the key events are sent to user-space 2) that the key presses don't do anything "in hardware" (eg. the BIOS changes things itself).

If that's the case, then I'm not going to bother touching the backlight sub-system.

You don't care when the user presses the brightness up/down buttons on the monitor and the action does change the brightness of monitor but there is no UI notification in GNOME. Right?

(In reply to comment #21)
> You don't care when the user presses the brightness up/down buttons on the
> monitor and the action does change the brightness of monitor but there is no UI
> notification in GNOME. Right?

We didn't handle that case before, we don't either now.

Adding this as a new functionality would certainly be doable, but I don't think that's what was being asked for at the start of this bug.

You should care about the user experience, we are looking forward to fix this issue.

There is a patch in comment 7, please review it.
Like I say at the beginning, it won't harm to monitor the uevent and read the brightness value from actual_brightness and shows the OSD, even if there is already a keyevent be reported.

I'm requesting to add this feature, please monitor the brightness changed outside of the g-s-d.

Review of attachment 212764 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1240,3 @@
+        if (g_strcmp0 ("change", action) != 0 ||
+            g_strcmp0 ("backlight", g_udev_device_get_subsystem (device)) != 0 ||
+            g_strcmp0 ("hotkey", g_udev_device_get_property (device, "SOURCE")) != 0)

If we've already received the key event to user-space, and we changed it, we don't need to wait for uevents, they're not useful to us.

Given that the driver doesn't seem to be able to make the difference between:
- the brightness button on the case of an all-in-one which doesn't report key events to user-space, and changes the brightness on its own
- the brightness key on a laptop keyboard which will report key events to user-space and not change the brightness
I don't think that this is implementable. The first case is the one that needs handling and that I'm willing to accept code changes for, the second one is already handled just fine.

@@ +1752,3 @@
         }
 
+#ifndef HAVE_GUDEV

Don't do that.

I don't want to change the normal case. We already calculate the updated brightness ourselves, I don't need to wait for the uevent.

I'm not really care about how you implement it.
Maybe the current architecture is not suitable for adding this feature, but keep in mind that brightness may be changed outside of g-s-d and we need OSD shown while it happens.

For your question, actually, in current asus-wmi driver implementation, we can know the event is coming from aio sidekey or from laptop hotkey.

(In reply to comment #25)
> I'm not really care about how you implement it.
> Maybe the current architecture is not suitable for adding this feature, but
> keep in mind that brightness may be changed outside of g-s-d and we need OSD
> shown while it happens.

The OSD should only be shown in response to key presses, and as g-s-d handles the brightness keys, all should work nicely if the key press is reported to user space and user space handles the brightness change.

> For your question, actually, in current asus-wmi driver implementation, we can
> know the event is coming from aio sidekey or from laptop hotkey.

I've not had an answer yet on whether the kernel driver can be changed to not change the brightness itself.

> > For your question, actually, in current asus-wmi driver implementation, we can
> > know the event is coming from aio sidekey or from laptop hotkey.
> 
> I've not had an answer yet on whether the kernel driver can be changed to not
> change the brightness itself.

The driver doesn't change the brightness, the firmware does. The driver only update the backlight interface in sysfs to make it match the state of the firmare (and the backlight interface sends an event saying it has been updated).

(In reply to comment #27)
> > > For your question, actually, in current asus-wmi driver implementation, we can
> > > know the event is coming from aio sidekey or from laptop hotkey.
> > 
> > I've not had an answer yet on whether the kernel driver can be changed to not
> > change the brightness itself.
> 
> The driver doesn't change the brightness, the firmware does. The driver only
> update the backlight interface in sysfs to make it match the state of the
> firmare (and the backlight interface sends an event saying it has been
> updated).

You corrected my statement and didn't answer the question.

Let me rephrase:
I've not had an answer yet on whether the kernel driver can be changed to not have the firmware
change the brightness itself.

No, there is no known way to do that.

Hello.
Before write anything: I'm a Gnome user, I love Gnome, I spread Gnome, I left Ubuntu going back to Debian because of Gnome, I fiddle every day with Debian experimental because I want the latest Gnome. So, I'm one of Gnome user you say you care about so much.

Still, I cannot understand nor justify the attitude "The real world is different from our vision of it, so of course the whole world is wrong". You are developing an *user application* which works on a *platform*. It seems quite obvious that the *platform* takes precedence and the *application* should adapt itself to *platform* mechanisms. Of course you may want to change these, then go ask for or propose a change on LKML, but until the change happens things work as they work (and Corentin Chary has perfectly clearly explained how they work).

Apart from platform specifications, it also makes perfectly sense to me what Chary wrote:

"Key events are *NOT* supposed to be backlight change notifcations, they are
only *KEYS*"

while I really cannot believe Bastien Nocera says something like:

"We do not care about notifications. Really. We don't care."

while talking about a *notification* system.

I'm a "beloved user" and I can only say that I don't care at all to be notified if *I* pressed a key: I already know it (unless I were on heavy drugs, in which case I guess I would miss the notification too). What I care about is to be notified if the status of my computer changed.

Bastien says: "Yeah, but I want to remap my brightness-up key to a nuke missile launcher trigger". Legitimate, but:

1. Sometimes you have *no control at all* on what a key does *in hardware* (my brightness keys work even during POST).

2. Still the model "notify the change, not the action" still works: you can launch your nuke bomb pressing the brightness-up key, *then* notify the launch *if* and *when* it happens (i.e. you got the uevent from the launcher driver).

Moreover, Gnome is not the whole world: things can be changed *outside* the Gnome scope, and still the user need to be notified. Better, Gnome itself needs to acknowledge the change to stay in sync with the real system state. To verify that, it is simply matter to issue an:

echo 5 > /sys/class/backlight/acpi_video0/brightness

You just changed the LCD brightness but the Gnome brightness slider stays stuck where it was (at least on my computer). And this despite the fact the kernel correctly notified the change with a uevent (and updated the file /sys/class/backlight/acpi_video0/actual_brightness):

KERNEL[7255.651129] change   /devices/pci0000:00/0000:00:01.0/0000:01:00.0/backlight/acpi_video0 (backlight)
UDEV  [7255.651818] change   /devices/pci0000:00/0000:00:01.0/0000:01:00.0/backlight/acpi_video0 (backlight)

My two cents. And sorry if I've been a bit harsh, but I hate when a really simple but really really annoying problem could be cleanly (yes, cleanly IMHO) solved (and someone said also how) and nothing is done because sophistry.

Peace and (Gnome) love.

P.S.: you could be interested in this bug report: https://bugzilla.kernel.org/show_bug.cgi?id=47841

*** Bug 694938 has been marked as a duplicate of this bug. ***

(In reply to comment #5)
> 
> And we are suffering from the brightness be changed twice problem by a single
> key stroke for a long time, that's because of the keyevents. EC reports the
> keyevent, so g-s-d adjusts the brightness, and ACPI video driver changes the
> brightness again, if the BIOS supports ACPI brightness functions.

The video.ko module has a parameter named brightness_switch_enabled that is designed for this purpose, set it to 0 will disable the in kernel manipulating of the backlight level.

(In reply to comment #32)
> (In reply to comment #5)
> > 
> > And we are suffering from the brightness be changed twice problem by a single
> > key stroke for a long time, that's because of the keyevents. EC reports the
> > keyevent, so g-s-d adjusts the brightness, and ACPI video driver changes the
> > brightness again, if the BIOS supports ACPI brightness functions.
> 
> The video.ko module has a parameter named brightness_switch_enabled that is
> designed for this purpose, set it to 0 will disable the in kernel manipulating
> of the backlight level.

Note: that works only for systems with a fully functional ACPI backlight. That is not the case for systems affected by this bug where a vendor driver has to handle the backlight.

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #5)
> > > 
> > > And we are suffering from the brightness be changed twice problem by a single
> > > key stroke for a long time, that's because of the keyevents. EC reports the
> > > keyevent, so g-s-d adjusts the brightness, and ACPI video driver changes the
> > > brightness again, if the BIOS supports ACPI brightness functions.
> > 
> > The video.ko module has a parameter named brightness_switch_enabled that is
> > designed for this purpose, set it to 0 will disable the in kernel manipulating
> > of the backlight level.
> 
> Note: that works only for systems with a fully functional ACPI backlight. That
> is not the case for systems affected by this bug where a vendor driver has to
> handle the backlight.

Right, as I wrote in comment #32, this parameter is for video.ko, and doesn't apply to other driver modules(like the vendor driver module asus-wmi.ko in this case).

And I agree with Corentin that platform driver can do nothing here but sending out an uevent message if firmware decides to change the backlight level directly when responding to the hotkey press.

And for acpi video driver, there is also no way to tell BIOS not to control backlight level directly. Luckily, for most BIOS I've seen, they will send out notification instead of manipulate backlight directly, and they seem to decide this base on OS version. BTW, if video.ko modifies the backlight level on responding to hotkey, it doesn't send out uevent, perhaps it should do so.

*** This bug has been marked as a duplicate of bug 773405 ***