GNOME Bugzilla – Bug 773403
PATCH: gsd-power-manager: Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal
Last modified: 2017-05-25 13:50:39 UTC
Created attachment 338320 [details] [review] [PATCH 4/5] gsd-power-manager: Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal, together with kernel + upower changes to detect kbdbacklight changes triggered by hardwired keyboard backlight control buttons. This will e.g. make the "Keyboard Brightness" slider in the control-center respond to brightness changes, caused by pressing such buttons, and always show to correct brightness. This patch sits on top of the bug-fixes I've submitted in bug 773402, hence the attachment is [PATCH 4/5]. This patch is useful even without the kernel changes, to e.g. detect other software changing the kbd brightness through upower, but significantly less so. Here is the bug for tracking the upower changes to poll on the sysfs brightness attr and emit org.freedesktop.UPower.KbdBacklight.BrightnessChanged when the poll wakesup: https://bugs.freedesktop.org/show_bug.cgi?id=98404 The main kernel patch adding poll() support for the brightness sysfs attr is here: 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, but you should probably wait with merging the 2nd patch till this is at least acked upstream. Note that without the kernel changes / on older kernels the poll() will simply never wakeup so the upower changes are 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
(In reply to Hans de Goede from comment #0) > This is v2, v1 has received a somewhat favorable review (some changes were > needed but no blockers) > so I expect this to go upstream, but you should probably wait with merging > the 2nd patch till this is at least acked upstream. Note that without the > kernel changes / on older kernels the poll() will simply never wakeup so the > upower changes are safe to do on older kernels. Erm, the "but you should probably wait with merging the 2nd patch till this is at least acked upstream" is a copy and paste fail from the upower bug, please ignore.
Review of attachment 338320 [details] [review]: s/gsd-power-manager/power/ Instead of re-explaining what the other patches do, link to them, for example. > Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal > together with kernel + upower changes to detect kbdbacklight changes > triggered by hardwired keyboard backlight control buttons. > > This will e.g. make the "Keyboard Brightness" slider in the control-center > respond to brightness changes, caused by pressing such buttons, and always > show to correct brightness. " Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal. Along with kernel[1] and upower[2] changes, this will make the slider in the Power Settings panel respond to brightness changes caused by pressing backlight buttons that change the brightness directly. [1]: <link to kernel patches> [2]: See https://... " Remove the SOB
Created attachment 344493 [details] [review] [PATCH 5/7] power: Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal Thanks for the reviews I'm attaching a new set of patches, addressing all review remarks.
Review of attachment 344493 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +1477,3 @@ + g_variant_get (parameters, "(i)", &new_brightness); + + if (manager->priv->kbd_brightness_now == new_brightness) How do we avoid out-of-order brightness notifications? Eg. UPower sends a "brightness changed" signal when we're in the process of setting a new one? @@ +2005,3 @@ } else { /* Keyboard brightness is not available */ + g_signal_handlers_disconnect_by_data (manager->priv->upower_kbd_proxy, manager); That's not necessary. @@ +2740,3 @@ + if (manager->priv->upower_kbd_proxy) + g_signal_handlers_disconnect_by_data (manager->priv->upower_kbd_proxy, manager); This neither, the signal will be disconnected when the proxy is destroyed.
Hi, (In reply to Bastien Nocera from comment #4) > Review of attachment 344493 [details] [review] [review]: > > ::: plugins/power/gsd-power-manager.c > @@ +1477,3 @@ > + g_variant_get (parameters, "(i)", &new_brightness); > + > + if (manager->priv->kbd_brightness_now == new_brightness) > > How do we avoid out-of-order brightness notifications? Eg. UPower sends a > "brightness changed" signal when we're in the process of setting a new one? Setting brightness through upower also causes upower to send a BrightnessChanged event (the main reason we need this check), so we will always get the last set value reported through BrightnessChanged and if we race in the worst case we briefly report an intermediate value. > > @@ +2005,3 @@ > } else { > /* Keyboard brightness is not available */ > + g_signal_handlers_disconnect_by_data > (manager->priv->upower_kbd_proxy, manager); > > That's not necessary. > > @@ +2740,3 @@ > > + if (manager->priv->upower_kbd_proxy) > + g_signal_handlers_disconnect_by_data > (manager->priv->upower_kbd_proxy, manager); > > This neither, the signal will be disconnected when the proxy is destroyed. Ok, will fix. Regards, Hans
(In reply to Hans de Goede from comment #5) > Hi, > > (In reply to Bastien Nocera from comment #4) > > Review of attachment 344493 [details] [review] [review] [review]: > > > > ::: plugins/power/gsd-power-manager.c > > @@ +1477,3 @@ > > + g_variant_get (parameters, "(i)", &new_brightness); > > + > > + if (manager->priv->kbd_brightness_now == new_brightness) > > > > How do we avoid out-of-order brightness notifications? Eg. UPower sends a > > "brightness changed" signal when we're in the process of setting a new one? > > Setting brightness through upower also causes upower to send a > BrightnessChanged event (the main reason we need this check), so we will > always get the last set value reported through BrightnessChanged and if we > race in the worst case we briefly report an intermediate value. This only works right now because our calls are sync, and we *will* change this in the future. If the kernel or hardware is particularly slow handling the changes, we shouldn't make other power related functionality block during that time. Just because we did this as a matter of expediency when we implemented it doesn't mean we should be building on top of something so brittle.
HI, (In reply to Bastien Nocera from comment #6) > (In reply to Hans de Goede from comment #5) > > Hi, > > > > (In reply to Bastien Nocera from comment #4) > > > Review of attachment 344493 [details] [review] [review] [review] [review]: > > > > > > ::: plugins/power/gsd-power-manager.c > > > @@ +1477,3 @@ > > > + g_variant_get (parameters, "(i)", &new_brightness); > > > + > > > + if (manager->priv->kbd_brightness_now == new_brightness) > > > > > > How do we avoid out-of-order brightness notifications? Eg. UPower sends a > > > "brightness changed" signal when we're in the process of setting a new one? > > > > Setting brightness through upower also causes upower to send a > > BrightnessChanged event (the main reason we need this check), so we will > > always get the last set value reported through BrightnessChanged and if we > > race in the worst case we briefly report an intermediate value. > > This only works right now because our calls are sync, and we *will* change > this in the future. If the kernel or hardware is particularly slow handling > the changes, we shouldn't make other power related functionality block > during that time. Just because we did this as a matter of expediency when we > implemented it doesn't mean we should be building on top of something so > brittle. Ok, so another option would be to extend upower's BrightnessChanged signal with a "source" argument like the BrightnessChanged signal my patches from bug 773405 add to gsd-power-manager. Do you know if it is ok to extend a dbus signal with extra arguments, or will this break existing users? Regards, Hans
(In reply to Hans de Goede from comment #7) <snip> > Ok, so another option would be to extend upower's BrightnessChanged signal > with a "source" argument like the BrightnessChanged signal my patches from > bug 773405 add to gsd-power-manager. > > Do you know if it is ok to extend a dbus signal with extra arguments, or > will this break existing users? It would break the API to change BrightnessChanged, but we can add a "BrightnessChangedWithSource" signal and capture that in g-s-d. Nothing broken, and we can deprecate the incomplete signal. Might need to find a better signal name, but that's a smaller problem :)
Created attachment 345354 [details] [review] [PATCH 1/3] power: Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChangedWithSource signal Hi, Here is a new version addressing the review comments and using the new upower BrightnessChangedWithSource signal to address to race concerns. Regards, Hans
Created attachment 348070 [details] [review] [PATCH 1/3] power: Monitor org.freedesktop.UPower.KbdBacklight.BrightnessChangedWithSource signal Updated patch to match the upower changes as merged upstream.
Ping? Anything blocking these from getting merged ? The upower and kernel patches are all in place so it would be nice to get this merged and the issues with keyboard brightness fixed.
Hi, can you rebase these patches? They don't apply cleanly anymore on master
*** Bug 767037 has been marked as a duplicate of this bug. ***
*** Bug 767040 has been marked as a duplicate of this bug. ***
Review of attachment 348070 [details] [review]: looks good
(In reply to Rui Matos from comment #12) > Hi, can you rebase these patches? They don't apply cleanly anymore on master Never mind this comment. I got this bug mixed with bug 77305
Hi, Thank you for the reviews. I was about to say that I don't have commit access and ask you to commit these, but I see that they have been commited now, thank you. Regards, Hans