GNOME Bugzilla – Bug 767040
GsdPowerManager: always use current keyboard backlight value
Last modified: 2017-05-24 15:26:38 UTC
Properly toggle keyboard backlight in hardwired configurations (like in ThinkPad and dell systems) where backlight level is handled internally by the bios and not notified to userspace. This change applies on top of patch on Bug 767037, it requires recent UPower.
Created attachment 328761 [details] [review] GsdPowerManager: update cached keyboard backlight before using it This allows to properly Toggle and save/restore the backlight keyboard even in hardwired configurations, where no ACPI events are emitted to request a keyboard backlight change (and the bios handles the change silently). The change has no effect with old upower versions, as it needs https://bugs.freedesktop.org/show_bug.cgi?id=95457 https://bugs.freedesktop.org/show_bug.cgi?id=96215
Created attachment 328764 [details] [review] GsdPowerManager: don't cache kbd backlight current value Keyboard backlight is now always fetched from UPower, without keeping a cached value around. This allows to properly Toggle and save/restore the backlight keyboard even in hardwired configurations, where no ACPI events are emitted to request a keyboard backlight change (and the bios handles the change silently).
Added a new version of the patch, which I consider cleaner. We can get rid of the kbd_brightness_now value at all, and just fetch it from upower. This patch can be applied directly, and there's no more dependency on Bug 767037.
Don't assign yourself to stuff, or change the status of bugs, thank you.
Review of attachment 328764 [details] [review]: Against which version is that? It doesn't apply. Commit message style as well.
Review of attachment 328761 [details] [review]: Coding style and commit message style.
(In reply to Bastien Nocera from comment #5) > Review of attachment 328764 [details] [review] [review]: > > Against which version is that? It doesn't apply. As written in the comment this applies on top of https://bugzilla.gnome.org/attachment.cgi?id=328759 However, if you prefer the 2nd patch to go in, these two can be considered obsolete, so let me know which path you'd prefer me to follow (I'm for the latter).
(In reply to Marco Trevisan (Treviño) from comment #7) > (In reply to Bastien Nocera from comment #5) > > Review of attachment 328764 [details] [review] [review] [review]: > > > > Against which version is that? It doesn't apply. > > As written in the comment this applies on top of > https://bugzilla.gnome.org/attachment.cgi?id=328759 Oh, I'm sorry I meant the we were talking of attachment 328761 [details] [review]. attachment 328764 [details] [review] instead applies on top of g-s-d master. (In reply to Bastien Nocera from comment #5) > Commit message style as well. What's wrong with style? Is it because it exceeds 72 characters?
(In reply to Marco Trevisan (Treviño) from comment #8) > (In reply to Marco Trevisan (Treviño) from comment #7) > > (In reply to Bastien Nocera from comment #5) > > > Review of attachment 328764 [details] [review] [review] [review] [review]: > > > > > > Against which version is that? It doesn't apply. > > > > As written in the comment this applies on top of > > https://bugzilla.gnome.org/attachment.cgi?id=328759 > > Oh, I'm sorry I meant the we were talking of attachment 328761 [details] [review] > [review]. This is a bit of a mess. One of the patches depends on another bug, but the other one doesn't. Next time, please put everything in the same bug. > attachment 328764 [details] [review] [review] instead applies on top of g-s-d master. > > (In reply to Bastien Nocera from comment #5) > > Commit message style as well. > > What's wrong with style? Is it because it exceeds 72 characters? Same as in the other bug, you're not using the same style as the existing commits. "git log plugins/power/" should show you examples.
Review of attachment 328764 [details] [review]: This adds more calls, and more sync calls, which means more chances of this causing us problems. We won't be using this, sorry. ::: plugins/power/gsd-power-manager.c @@ +1192,3 @@ g_debug ("keyboard toggle on"); /* save the current value to restore later when untoggling */ + manager->priv->kbd_brightness_old = upower_kbd_get_brightness (manager); This adds round trips. Instead of making one call, we're making one call to upower to get the current value, then another to set the brightness.
Review of attachment 328764 [details] [review]: For being more clear, this patch has been done to fix an issue that is present with some current laptops (ThinkPad and Dell for sure) that have a keyboard backlight that is managed by the BIOS (in the sense that userspace has no ability to prevent a brightness change, but just to record it). Without always pulling the current backlight value from upower before toggling it, we can't be sure to restore it to the right value. A test case is explained here http://pad.lv/1583861 ::: plugins/power/gsd-power-manager.c @@ +1192,3 @@ g_debug ("keyboard toggle on"); /* save the current value to restore later when untoggling */ + manager->priv->kbd_brightness_old = upower_kbd_get_brightness (manager); Unfortunately there's no other way, to do this, since we can't be sure whether the user has changed the kbd brightness in the mean time, and thus if we're saving a wrong value. This is actually the line that fixes the main issue with gsd. Since Upower doesn't notify anything to the user (in hardwired configurations), we can't do anything different than this.
(In reply to Bastien Nocera from comment #10) > Review of attachment 328764 [details] [review] [review]: > > This adds more calls, and more sync calls, which means more chances of this > causing us problems. > We won't be using this, sorry. The number of calls of both the patches are almost the same (quite inexpensive and not really frequent, though), so I didn't see any sense of keeping the value cached, since we need to update its value anyway before using it. (In reply to Bastien Nocera from comment #9) > This is a bit of a mess. One of the patches depends on another bug, but the > other one doesn't. Next time, please put everything in the same bug. Yeah, you're right sorry... I should have done just one bug, but since these were changing different things, I split the thing.
For those laptops where the brightness is changed in the firmware and the firmware sends a key event when the brightness changed, you'll need to do much more work and not overload the brightness up/down keys. I already did this for touchpad toggles a long time ago. You'll need to: 1) Add a kernel event that gets sent when the firmware changes the brightness from underneath us (KEY_BRIGHTNESS_CHANGED for example) 2) Make those laptops use it, either through modifying the platform drivers, or through udev hwdb rules if they use standard keys (the keymap rules are the ones you want to change) 3) Modify the X keymaps to map this kernel key to an X keysym. 4) add support in gnome-settings-daemon for handling those key events, which will read the new brightness and just show the new brightness instead of trying to change it Look for "XF86TouchpadToggle", "XF86TouchpadOn" and "XF86TouchpadOff" in gnome-settings-daemon for an example. The first one is a key sent when gnome-settings-daemon will change the touchpad status itself, the latter two are when the firmware changes the values of whether the touchpad is disabled.
Hi, (In reply to Bastien Nocera from comment #13) > For those laptops where the brightness is changed in the firmware and the > firmware sends a key event when the brightness changed, you'll need to do > much more work and not overload the brightness up/down keys. > > I already did this for touchpad toggles a long time ago. You'll need to: > 1) Add a kernel event that gets sent when the firmware changes the > brightness from underneath us (KEY_BRIGHTNESS_CHANGED for example) > 2) Make those laptops use it, either through modifying the platform drivers, > or through udev hwdb rules if they use standard keys (the keymap rules are > the ones you want to change) > 3) Modify the X keymaps to map this kernel key to an X keysym. > 4) add support in gnome-settings-daemon for handling those key events, which > will read the new brightness and just show the new brightness instead of > trying to change it > > Look for "XF86TouchpadToggle", "XF86TouchpadOn" and "XF86TouchpadOff" in > gnome-settings-daemon for an example. The first one is a key sent when > gnome-settings-daemon will change the touchpad status itself, the latter two > are when the firmware changes the values of whether the touchpad is disabled. I've recently been working on fixing this, I've not gone the make the kernel send a KEY_BRIGHTNESS_CHANGED route since using an input event to notify something has changed feels wrong (esp. when we've other well established interfaces for this), also we're out of KEY codes under 256 and codes above 256 do not work with X. My fix consists of 3 parts: 1) Kernel patches to allow using poll on the sysfs brightness attribute of the led-class: http://www.spinics.net/lists/platform-driver-x86/msg09612.html This is v2, v1 has received a somewhat favorable review (some changes were needed but no blockers) so I expect this to go upstream more or less as is. Note that without the kernel changes / on older kernels the poll() will simply never wakeup so this is safe to do on older kernels. FYI: I've also implemented actually waking up the poll when a hardwired hotkey changes the keyboard backlight brightness for dell laptops: http://www.spinics.net/lists/platform-driver-x86/msg09615.html http://www.spinics.net/lists/platform-driver-x86/msg09613.html 2) Upower changes for listening with poll and emitting BrightnessChanged when the brightness is changed: https://bugs.freedesktop.org/show_bug.cgi?id=98404 3) gnome-settings-daemon fixes + support for monitoring for changes through upower: https://bugzilla.gnome.org/show_bug.cgi?id=773402 https://bugzilla.gnome.org/show_bug.cgi?id=773403 https://bugzilla.gnome.org/show_bug.cgi?id=773405 Note once these 3 patch-sets are merged this should be considered fixed from a g-s-d pov, and this bug can be closed. Regards, Hans
*** This bug has been marked as a duplicate of bug 773403 ***