Make powerd support keyboard backlights announced after boot |
||||||||
Issue descriptionpowerd should support keyboard backlights that are announced after the system boots (http:/b/37971411). I think that these will show up via udev "add" events about */leds/*::kbd_backlight. We already have a has_keyboard_backlight powerd pref. When it's set, I think Daemon::Init() should initialize KeyboardBacklightController even if it's not possible to create a KeyboardBacklight then. KeyboardBacklightController needs to be updated to support not having a backlight to control, and to support adding one later after it's already been initialized. Alternately, maybe KeyboardBacklightController should be dynamically created and destroyed.
,
Jul 28 2017
There are many different ways this could be implemented: a) InternalBacklight::Init() could take an optional UdevInterface*. When it's non-null, Init() would return true even if the device isn't found and monitor udev events to watch for a backlight showing up later. We'd create a single KeyboardBacklightController at startup and use that throughout, which is a probably a good thing, since it means we won't lose user-requested levels, adjustment counts, etc. if the backlight temporarily goes away. The downside of this is that KeyboardBacklightController::Init() currently caches the current and max levels from the device, and it doesn't expect these to change dynamically. I'd probably need to add a BacklightObserver interface and use it so that InternalBacklight can notify KBC when the underlying device changes. KBC will need additional code to update its state when this happens. b) KeyboardBacklightController::Init() could support receiving a null BacklightInterface*, and Daemon could watch for the udev events and update KBC's backlight via a new SetBacklight method as needed. I don't know if this has any advantages over a). The code changes needed in KBC will be similar. Some udev-related code will go into Daemon, which is already way too big of a class. And there will be more null pointers flying around, which doesn't seem like a good thing. c) Daemon could dynamically create and destroy backlight and KeyboardBacklightController objects as needed. This has the same downside that more udev code goes into Daemon, and we'll lose the controller's state every time it needs to be recreated. The KBC also gets passed into MetricsCollector, which doesn't currently expect it to change later. The only upside is that InternalBacklight and KBC wouldn't need to be modified. So I think I'm leaning towards a).
,
Jul 28 2017
,
Jul 28 2017
,
Jul 29 2017
,
Aug 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/416ee65f6279b7618278ba30828a182c5ede0cd4 commit 416ee65f6279b7618278ba30828a182c5ede0cd4 Author: Daniel Erat <derat@chromium.org> Date: Tue Aug 01 09:39:54 2017 power: Track keyboard backlight brightness as percent. Make KeyboardBacklightController track the current brightness as a percentage instead of as a hardware level and not cache the maximum level. This isn't intended to have any effect right now, but it'll make it easy to support changes to the underlying backlight device in the future. BUG= chromium:749539 TEST=existing tests pass Change-Id: I784f4e3dd23c353f81b647e98c92bddaa93f800e Reviewed-on: https://chromium-review.googlesource.com/591953 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/416ee65f6279b7618278ba30828a182c5ede0cd4/power_manager/powerd/policy/keyboard_backlight_controller.cc [modify] https://crrev.com/416ee65f6279b7618278ba30828a182c5ede0cd4/power_manager/powerd/policy/keyboard_backlight_controller.h
,
Aug 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/24ae88723c36bb6f5600b9a460d21371b676af2c commit 24ae88723c36bb6f5600b9a460d21371b676af2c Author: Daniel Erat <derat@chromium.org> Date: Tue Aug 01 09:39:54 2017 power: Clean up duplicated backlight code. Move code for reading and writing files containing 64-bit integers into shared utility functions. BUG= chromium:749539 TEST=tests pass Change-Id: Ie9516f1411d443cde03991896893fab9f8a6fa3f Reviewed-on: https://chromium-review.googlesource.com/594718 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/24ae88723c36bb6f5600b9a460d21371b676af2c/power_manager/common/util.h [modify] https://crrev.com/24ae88723c36bb6f5600b9a460d21371b676af2c/power_manager/common/util.cc [modify] https://crrev.com/24ae88723c36bb6f5600b9a460d21371b676af2c/power_manager/powerd/system/internal_backlight_unittest.cc [modify] https://crrev.com/24ae88723c36bb6f5600b9a460d21371b676af2c/power_manager/powerd/system/internal_backlight.cc
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/dcb195c4390beb3ec2d5279857f335b317b2c446 commit dcb195c4390beb3ec2d5279857f335b317b2c446 Author: Daniel Erat <derat@chromium.org> Date: Fri Aug 04 22:39:07 2017 power: Fix misleading backlight logging. Remove a "Cannot initialize display backlight" error that powerd also logged when the *keyboard* backlight couldn't be initialized. Instead, log more-useful messages that describe the directories that were searched and the patterns that were used. BUG= chromium:749539 ,b:64125218 TEST=none Change-Id: Ic7bdc02310d8e1d814f3dda83da4477f3271201e Reviewed-on: https://chromium-review.googlesource.com/601407 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Aaron Durbin <adurbin@chromium.org> [modify] https://crrev.com/dcb195c4390beb3ec2d5279857f335b317b2c446/power_manager/powerd/daemon.cc [modify] https://crrev.com/dcb195c4390beb3ec2d5279857f335b317b2c446/power_manager/powerd/main.cc
,
Aug 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/ba60746eadabefe18c9e257d2e4cded3241c77fc commit ba60746eadabefe18c9e257d2e4cded3241c77fc Author: Daniel Erat <derat@chromium.org> Date: Tue Aug 29 01:21:32 2017 power: Add PluggableInternalBacklight class. Add a new class (not yet instantiated) that handles hotpluggable backlights by listening for udev events and maintaining an InternalBacklight object for interacting with the underlying device. Also introduce a BacklightObserver interface and update BacklightInterface and its implementations to support observers. BUG= chromium:749539 TEST=added tests Change-Id: I5c52213043703c5156f986b7fa3938d71dfe49bb Reviewed-on: https://chromium-review.googlesource.com/631217 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/backlight_stub.h [add] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/backlight_observer.h [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/common/power_constants.cc [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/backlight_interface.h [add] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/pluggable_internal_backlight.h [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/power_manager.gyp [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/common/power_constants.h [add] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/pluggable_internal_backlight_unittest.cc [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/udev/93-powerd-late.rules [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/internal_backlight_unittest.cc [add] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/pluggable_internal_backlight.cc [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/udev.h [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/backlight_stub.cc [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/internal_backlight.cc [modify] https://crrev.com/ba60746eadabefe18c9e257d2e4cded3241c77fc/power_manager/powerd/system/internal_backlight.h
,
Sep 13 2017
With CL: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/646873 I think powerd need some delay after replug. ---> Initial bootup [0913/043237:INFO:daemon.cc(1496)] Received request to stop forcing backlights off [0913/043251:INFO:keyboard_backlight_controller.cc(527)] Setting brightness to 40 (40%) over 200 ms ---> Unplug [0913/043302:INFO:pluggable_internal_backlight.cc(93)] Got udev remove event for hammer::kbd_backlight on subsystem leds [0913/043302:INFO:pluggable_internal_backlight.cc(77)] No backlight found under /sys/class/leds matching pattern *:kbd_backlight ---> Replug [0913/043310:INFO:pluggable_internal_backlight.cc(93)] Got udev add event for hammer::kbd_backlight on subsystem leds [0913/043310:WARNING:internal_backlight.cc(68)] Can't write to /sys/class/leds/hammer::kbd_backlight/brightness [0913/043310:INFO:pluggable_internal_backlight.cc(77)] No backlight found under /sys/class/leds matching pattern *:kbd_backlight and the LED entries actually exists afterwards, the driver need sometime to create the LED interface. You probably want to add a delay after getting the udev add event.
,
Sep 13 2017
How long does it take for the LED interface to be created? Would it be possible for the kernel to instead only send the add event after the device is actually usable?
,
Sep 13 2017
,
Sep 14 2017
ok, did more debug and it turns out to be caused by the permission of the 'brigtness' file: localhost ~ # ls -l /sys/class/leds/hammer::kbd_backlight/brightness -rw-r--r--. 1 root root 4096 Sep 13 20:46 /sys/class/leds/hammer::kbd_backlight/brightness after replug the file belongs to root:root. We need to add udev rules to chown it to powerd:powerd.
,
Sep 14 2017
Here's where we change ownership of existing devices when starting powerd: https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/init/shared/powerd-pre-start.sh#37
,
Sep 14 2017
Yeah, but when you replug hammer, the LED entries will be recreated, so you need to change the ownership again.
,
Sep 14 2017
Yeah, I understand that that's why we need udev rules for this -- I was just linking to the init script to show what the permissions should be. :-) Are you planning to do that, or should I? If I do it, I'll probably need your help testing it (but I can just include it in my powerd change).
,
Sep 15 2017
Can you do it, since you are more familiar with how powerd work. I can help verify it. I already verified that changing the permission manually works.
,
Sep 16 2017
Mind testing the change again? I've added a udev rule.
,
Sep 18 2017
(I don't see anything that warrants R-V-G)
,
Sep 18 2017
re #19 drinkcat, can you help test this? I'm in travel in China and won't be back until next week.
,
Sep 18 2017
Setting: USE=has_keyboard_backlight The patch works fine, that is: backlight_dbus_tool --increase_keyboard changes the brightness reported in /sys/class/leds/hammer::kbd_backlight/brightness
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd commit 2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd Author: Daniel Erat <derat@chromium.org> Date: Tue Sep 19 07:36:50 2017 power: Use PluggableInternalBacklight for keyboards. Update powerd's KeyboardBacklightController to support keyboard backlight devices appearing and disappearing. This also adds a udev rule that runs a chown-sysfs-backlight-dir.sh to make newly-added keyboard backlights owned by powerd. BUG= chromium:749539 TEST=added tests Change-Id: I2d1daa5a92b0ccdaf378bb98eede86855829de0e Reviewed-on: https://chromium-review.googlesource.com/646873 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> [add] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/udev/chown-sysfs-backlight-dir.sh [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/main.cc [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/policy/keyboard_backlight_controller_unittest.cc [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/daemon_delegate.h [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/daemon.cc [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/policy/keyboard_backlight_controller.cc [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/policy/keyboard_backlight_controller.h [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/udev/99-powerd-permissions.rules [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/system/pluggable_internal_backlight.cc [modify] https://crrev.com/2ab7556fe97a544d6c59c1a6f3075fa81b1be6cd/power_manager/powerd/daemon_unittest.cc
,
Sep 19 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by derat@chromium.org
, Jul 27 2017