|
Description
Hans de Goede
2016-10-24 07:27:39 UTC
Created attachment 127510 [details] [review] [PATCH 2/2] UpKbdBacklight: Watch for POLL_PRI on the brightness fd Comment on attachment 127509 [details] [review] [PATCH 1/2] UpKbdBacklight: Emit BrightnessChanged when brightness is changed on GetBrightness Review of attachment 127509 [details] [review]: ----------------------------------------------------------------- ::: src/up-kbd-backlight.c @@ +142,5 @@ > up_exported_kbd_backlight_complete_get_brightness (skeleton, invocation, > brightness); > + if (brightness != kbd_backlight->priv->brightness) { > + kbd_backlight->priv->brightness = brightness; > + up_exported_kbd_backlight_emit_brightness_changed ( One line, we have wide screens. @@ +268,4 @@ > kbd_backlight->priv->fd = open (path_now, O_RDWR); > > /* read brightness and check if it has an acceptable value */ > + kbd_backlight->priv->brightness = up_kbd_backlight_brightness_read (kbd_backlight); Is it such a good idea to save errors? I'd save it in a temp variable, and only set this value if there were no errors. Comment on attachment 127510 [details] [review] [PATCH 2/2] UpKbdBacklight: Watch for POLL_PRI on the brightness fd Review of attachment 127510 [details] [review]: ----------------------------------------------------------------- ::: src/up-kbd-backlight.c @@ +211,5 @@ > /** > + * up_kbd_backlight_event_io: > + **/ > +static gboolean > +up_kbd_backlight_event_io (GIOChannel *channel, GIOCondition condition, gpointer data) Please check the condition, and make sure to free it when exiting. Comment on attachment 127509 [details] [review] [PATCH 1/2] UpKbdBacklight: Emit BrightnessChanged when brightness is changed on GetBrightness Review of attachment 127509 [details] [review]: ----------------------------------------------------------------- > Since commit 793642bfb7("pKbdBacklight: don't cache the brightness level, Typo in commit subject. Please also remove the SOB from the commit messages. Hi, Thanks for the review, I've prepared a new version of both patches addressing the review comments. As for the second patch, I've also changed it to match some changes on the kernel side. Allowing poll on the regular brightness attribute was causing high CPU-load in some embedded systems. So there now is a new kernel patch-set which instead adds a current_brightness sysfs file, which is pollable and will only be available on some LEDs (with some triggers) to avoid the CPU load issues. Please review, but wait a bit with merging the second patch until it is clear the kernel patches are really final (I'll add another comment at that time). Regards, Hans Created attachment 128051 [details] [review] UpKbdBacklight: Emit BrightnessChanged when brightness is changed on GetBrightness Created attachment 128052 [details] [review] UpKbdBacklight: Try the new current_brightness attr and watch for POLL_PRI Comment on attachment 128051 [details] [review] UpKbdBacklight: Emit BrightnessChanged when brightness is changed on GetBrightness Review of attachment 128051 [details] [review]: ----------------------------------------------------------------- > re-gets the actual brightness gets the current brightness again And s/upower/UPower/ Looks fine otherwise Comment on attachment 128052 [details] [review] UpKbdBacklight: Try the new current_brightness attr and watch for POLL_PRI Review of attachment 128052 [details] [review]: ----------------------------------------------------------------- > New kernels have a current_brightness attribute which can be polled to State the version, or the commit and link to it, "new" will mean something else in 6 months time. Looks fine otherwise. ::: src/up-kbd-backlight.c @@ +212,5 @@ > + **/ > +static gboolean > +up_kbd_backlight_event_io (GIOChannel *channel, GIOCondition condition, gpointer data) > +{ > + UpKbdBacklight *kbd_backlight = (UpKbdBacklight*) data; No need for the cast from a gpointer. @@ +213,5 @@ > +static gboolean > +up_kbd_backlight_event_io (GIOChannel *channel, GIOCondition condition, gpointer data) > +{ > + UpKbdBacklight *kbd_backlight = (UpKbdBacklight*) data; > + gint brightness; Use "int" please (the rest of the file doesn't, but we'll move away little by little). @@ +286,4 @@ > goto out; > } > > + /* You can put this 4-line comment on two-lines, we're not following kernel-style comments (even that has one too much I think). Do those patches avoid the double-set when pressing a button, or feedback loops with the brightness slider in the Power panel? Hi, (In reply to Bastien Nocera from comment #10) > Comment on attachment 128052 [details] [review] [review] > UpKbdBacklight: Try the new current_brightness attr and watch for POLL_PRI > > Review of attachment 128052 [details] [review] [review]: > ----------------------------------------------------------------- > > > New kernels have a current_brightness attribute which can be polled to > > State the version, or the commit and link to it, "new" will mean something > else in 6 months time. Ack, will do so once the kernel bits are actually merged upstream (or are in -next at least). > Looks fine otherwise. > > ::: src/up-kbd-backlight.c > @@ +212,5 @@ > > + **/ > > +static gboolean > > +up_kbd_backlight_event_io (GIOChannel *channel, GIOCondition condition, gpointer data) > > +{ > > + UpKbdBacklight *kbd_backlight = (UpKbdBacklight*) data; > > No need for the cast from a gpointer. > > @@ +213,5 @@ > > +static gboolean > > +up_kbd_backlight_event_io (GIOChannel *channel, GIOCondition condition, gpointer data) > > +{ > > + UpKbdBacklight *kbd_backlight = (UpKbdBacklight*) data; > > + gint brightness; > > Use "int" please (the rest of the file doesn't, but we'll move away little > by little). Ok. > > @@ +286,4 @@ > > goto out; > > } > > > > + /* > > You can put this 4-line comment on two-lines, we're not following > kernel-style comments (even that has one too much I think). Ok. (In reply to Bastien Nocera from comment #11) > Do those patches avoid the double-set when pressing a button There is no double set when pressing a button, the buttons on the laptops I'm looking at do not generate a keypress atm. I'm adding poll() support to the LED sysfs interface so that userspace can still detect changes done through the hotkey, even though the hardware/firmware already changes the brightness autonomously. Generating a keypress in that case would be wrong, hence the poll() support instead. So in a way yes, these avoid the double-set by not emitting a keypress event and instead notifying userspace of the brightness change directly through the LED sysfs API. > or feedback loops with the brightness slider in the Power panel? Feedback loops are avoided by up_kbd_backlight_event_io() checking that the brightness it reads on a poll() wake-up is different then the previous brightness. Regards, Hans Created attachment 129440 [details] [review] [PATCH 1/3] UpKbdBacklight: Add new BrightnessChangedWithSource signal Hi, Here is a new set of the patches using the new brightness_hw_changed sysfs attribute which upstream eventually settled on (it is in linux-next heading for 4.11). This also introduces a new BrightnessChangedWithSource signal so that listeners can distinguish between changes caused by SetBrightness getting called and changed detected through the new brightness_hw_changed sysfs attribute. Regards, Hans Created attachment 129441 [details] [review] [PATCH 2/3] UpKbdBacklight: up_kbd_backlight_brightness_read: pass in fd Created attachment 129442 [details] [review] [PATCH 3/3] UpKbdBacklight: Try the new brightness_hw_changed attr and watch for POLL_PRI Comment on attachment 129440 [details] [review] [PATCH 1/3] UpKbdBacklight: Add new BrightnessChangedWithSource signal Review of attachment 129440 [details] [review]: ----------------------------------------------------------------- ::: dbus/org.freedesktop.UPower.KbdBacklight.xml @@ +106,5 @@ > + </arg> > + <arg name="source" direction="out" type="s"> > + <doc:doc> > + <doc:summary> > + Source of the keyboard backlight brightness change. Needs to list the possible values, if they are known, eg. : Source of the keyboard backlight brightness change, such as "XXX" for something or other events, and "XXX" for something else entirely. @@ +114,5 @@ > + <doc:doc> > + <doc:description> > + <doc:para> > + The keyboard backlight brightness level has changed including > + information of the source of the change. including information about the source of the change. Comment on attachment 129441 [details] [review] [PATCH 2/3] UpKbdBacklight: up_kbd_backlight_brightness_read: pass in fd Review of attachment 129441 [details] [review]: ----------------------------------------------------------------- Looks good, but the commit message isn't quite up to scratch. I can handle that when I'm committing though. Comment on attachment 129442 [details] [review] [PATCH 3/3] UpKbdBacklight: Try the new brightness_hw_changed attr and watch for POLL_PRI Review of attachment 129442 [details] [review]: ----------------------------------------------------------------- > Try the new brightness_hw_changed attr and watch for POLL_PRI Send notifications about hardware brightness changes ::: src/up-kbd-backlight.c @@ +47,2 @@ > gint max_brightness; > + GIOChannel *channel; A better name would be nice, such as "channel_hw_changed", and put next to fd_hw_changed in the struct. Created attachment 129640 [details] [review] UpKbdBacklight: Add new BrightnessChangedWithSource signal There are multiple possible causes for the keyboard brightness to change, e.g. SetBrightness may get called, or the brightness may get changed through a hotkey which is handled in firmware. Created attachment 129641 [details] [review] UpKbdBacklight: Allow reading from alternate fd in _brightness_read() Pass the fd to use to up_kbd_backlight_brightness_read() so that it can be used with multiple fds. This will be used to support the "brightness_hw_changed" sysfs attribute. Created attachment 129642 [details] [review] UpKbdBacklight: Send notifications about hardware brightness changes Backlights, starting with kernel 4.11 [1] can have a "brightness_hw_changed" sysfs attribute, which can be polled to detect hardware initiated brightness changes, such as ones done through firmware-handled keyboard backlight hotkeys, or firmware controlled changes (eg. turn off backlight on low battery). [1] https://patchwork.kernel.org/patch/9544111/ Created attachment 129644 [details] [review] UpKbdBacklight: Add new BrightnessChangedWithSource signal There are multiple possible causes for the keyboard brightness to change, e.g. SetBrightness may get called, or the brightness may get changed through a hotkey which is handled in firmware. Created attachment 129645 [details] [review] UpKbdBacklight: Allow reading from alternate fd in _brightness_read() Pass the fd to use to up_kbd_backlight_brightness_read() so that it can be used with multiple fds. This will be used to support the "brightness_hw_changed" sysfs attribute. Created attachment 129646 [details] [review] UpKbdBacklight: Send notifications about hardware brightness changes Backlights, starting with kernel 4.11 [1] can have a "brightness_hw_changed" sysfs attribute, which can be polled to detect hardware initiated brightness changes, such as ones done through firmware-handled keyboard backlight hotkeys, or firmware controlled changes (eg. turn off backlight on low battery). [1] https://patchwork.kernel.org/patch/9544111/ Attachment 129644 [details] pushed as 112814f - UpKbdBacklight: Add new BrightnessChangedWithSource signal Attachment 129645 [details] pushed as d28ee53 - UpKbdBacklight: Allow reading from alternate fd in _brightness_read() Attachment 129646 [details] pushed as 0e256ec - UpKbdBacklight: Send notifications about hardware brightness changes |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.