Input devices shouldn't wake from S3 while in tablet mode |
||||
Issue description
powerd's powerd/policy/wakeup_controller.h file defines the following enum:
// Describes which mode the system is currently in, depending on e.g. the state
// of the lid. Currently, WakeupController only tracks CLOSED and OPEN.
enum WakeupMode {
WAKEUP_MODE_CLOSED = 0, // Lid closed, no external monitor attached.
WAKEUP_MODE_DOCKED, // Lid closed, external monitor attached.
WAKEUP_MODE_DISPLAY_OFF, // Internal display off, external monitor attached.
WAKEUP_MODE_LAPTOP, // Lid open.
WAKEUP_MODE_TABLET, // Tablet mode, e.g. lid open more than 180 degrees.
};
The WakeupController class has never contained any code to put it into WAKEUP_MODE_TABLET, though. As a result, even when a device enumerated by udev contains "wakeup" and "wakeup_only_when_usable" tags and lacks "usable_when_tablet", we don't disable wakeups from it while in tablet mode.
It seems straightforward to go into WAKEUP_MODE_TABLET when powerd learns that we've entered this mode from a tablet mode switch. Do we want to do that? Is it safe to do?
I can think of some risks:
a) Some convertibles have touchscreen devices that are managed by powerd but that lack the usable_when_tablet flag, so their touchscreens no longer work in tablet mode (whoops).
b) Disabling keyboard wakeups while in tablet mode also disables power button wakeups on some devices (maybe this is impossible), so there's no way to wake up the device.
c) Disabling the keyboard while in tablet mode breaks Julius's "smart keyboard" face-down-tablet-with-external-display use case. :-) Hopefully we can avoid this by also checking for docked mode?
d) Users go back to clamshell mode while suspended and are confused when the keyboard and touchpad no longer wake the device. Does the EC already handle this on some/all devices, by e.g. waking in response to the hinge exceeding a threshold? I believe there's powerd code for setting this.
cyan appears to currently not wake from keyboard after going into S3 from tablet mode, while kevin *does* wake from keyboard, so I suspect that we already have divergent EC behavior here. Please educate me further, though. :-)
,
Nov 2 2016
Gwendal on issue 620633: "Looks like kevin is missing CONFIG_LID_ANGLE_UPDATE: That would disable touch and keyboard is S3 when lid angle is > 180." Would that make this powerd change unnecessary? If so, I'll delete the WAKEUP_MODE_TABLET code from powerd.
,
Nov 3 2016
> c) Disabling the keyboard while in tablet mode breaks Julius's "smart keyboard" face-down-tablet-with-external-display use case. :-) Hopefully we can avoid this by also checking for docked mode? This is WAKEUP_MODE_DISPLAY_OFF, which is intentionally distinct from and prioritized over WAKEUP_MODE_TABLET for this reason. > cyan appears to currently not wake from keyboard after going into S3 from tablet mode, while kevin *does* wake from keyboard, so I suspect that we already have divergent EC behavior here. Please educate me further, though. :-) AFAIK the intended way (at least how it worked on Minnie, although of course all of our EC images are different and we probably do not have as much conformance testing as we should have there) is that the EC wake interrupt stays enabled by the kernel no matter what, and the EC decides whether to wake up by querying the current lid angle and comparing it to its KB_WAKE_ANGLE property. KB_WAKE_ANGLE defaults to 180, but while powerd is in WAKEUP_MODE_DISPLAY_OFF it changes the EC's KB_WAKE_ANGLE to 360. I think having a separate (working) WAKEUP_MODE_TABLE in powerd would still make sense. It just shouldn't inhibit the EC wake interrupt, but it might still be useful to inhibit other wake sources (e.g. touchpad, which is not routed through the EC on all boards).
,
Nov 4 2016
From issue 620633, https://chromium-review.googlesource.com/406739 updates kevin's EC firmware to support disabling the touchpad and keyboard while in tablet mode.
,
Nov 29 2016
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/7dcb8dde104a816a7e893a6497c6b6c5724f440a commit 7dcb8dde104a816a7e893a6497c6b6c5724f440a Author: Daniel Erat <derat@chromium.org> Date: Tue Nov 29 17:58:31 2016 power: Clean up handling of initial tablet mode. Don't report a fake tablet mode change from within Daemon::Init(). Also add Daemon tests to verify that we're no longer doing this and that wifi transmit power is updated in response to tablet mode changes. BUG= chromium:661368 TEST=added tests Change-Id: I147924487988481aa78e3000f55631bd9a4fdc28 Reviewed-on: https://chromium-review.googlesource.com/414593 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/7dcb8dde104a816a7e893a6497c6b6c5724f440a/power_manager/powerd/daemon.cc [modify] https://crrev.com/7dcb8dde104a816a7e893a6497c6b6c5724f440a/power_manager/powerd/daemon.h [modify] https://crrev.com/7dcb8dde104a816a7e893a6497c6b6c5724f440a/power_manager/powerd/daemon_unittest.cc
,
Dec 1 2016
Re #2: What we are doing with the touchpad in Kevin board file is wrong, as EC does not know how to properly initialize it. So simply cutting off power to it and hope for the best is not the solution. The kernel needs to be involved and decide if the device needs to be fully reinitialized, or if it was supposed to keep the state and should be simply woken up. We definitely need tablet support in powerd.
,
Dec 2 2016
Julius, hopefully-quick question about your "smart keyboard" change at https://chromium-review.googlesource.com/329948: In addition to marking internal_keyboard, internal_touchpad, and external_input as usable_when_display_off, it also marked internal_touchscreen as usable_when_display_off. Was that accidental? I don't see how the touchscreen fits in with the use case you were trying to support -- wouldn't it be face down and ideally disabled?
,
Dec 2 2016
I've uploaded a pretty-straightforward-but-untested change to make powerd configure input devices for tablet mode at https://chromium-review.googlesource.com/416373. It should be tested against existing convertible devices before it's submitted, as I'm not very trusting that everything is configured correctly on the udev tags side (since this was a no-op before).
,
Dec 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/8c86776a369296d094706099720764ed1060f5e0 commit 8c86776a369296d094706099720764ed1060f5e0 Author: Daniel Erat <derat@chromium.org> Date: Fri Oct 07 23:39:21 2016 power: Make WakeupController handle tablet mode. Make powerd's policy::WakeupController inhibit and configure S3 wakeup for input devices while in tablet mode. It previously had code for handling the mode, but was never wired up to be notified about tablet switch events. It's possible/likely that the udev tags around tablet mode will need to be updated, as this code has never been exercised. BUG= chromium:661368 TEST=added unit tests; also checked powerd log after switching in and out of tablet mode and saw that devices were inhibited and configured for wakeup as expected Change-Id: Ifb6f9e04b42ef21e3e667f420dbcdf77049fc533 Reviewed-on: https://chromium-review.googlesource.com/416373 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@google.com> [modify] https://crrev.com/8c86776a369296d094706099720764ed1060f5e0/power_manager/powerd/daemon.cc [modify] https://crrev.com/8c86776a369296d094706099720764ed1060f5e0/power_manager/powerd/policy/wakeup_controller.cc [modify] https://crrev.com/8c86776a369296d094706099720764ed1060f5e0/power_manager/powerd/policy/wakeup_controller.h [modify] https://crrev.com/8c86776a369296d094706099720764ed1060f5e0/power_manager/powerd/policy/wakeup_controller_unittest.cc
,
Jan 10 2017
,
Jan 11 2017
Is the bug only applied to Kevin device? I am able to verify the fix in Kevin in Chrome OS 9000.58.0, 56.0.2924.58. However Minnie in table mode can still be waken with keyboard or mouse input.
,
Jan 13 2017
> Julius, hopefully-quick question about your "smart keyboard" change at https://chromium-review.googlesource.com/329948: In addition to marking internal_keyboard, internal_touchpad, and external_input as usable_when_display_off, it also marked internal_touchscreen as usable_when_display_off. Was that accidental? I don't see how the touchscreen fits in with the use case you were trying to support -- wouldn't it be face down and ideally disabled? Whoops, sorry, must have missed this. I probably just tried to "enable everything" without thinking... you're right, the touchscreen makes no sense in that case. (Note that the touchscreen is still effectively inhibited in that mode... I think Chrome is doing that whenever the display is off.) > However Minnie in table mode can still be waken with keyboard or mouse input. Not quite sure what you mean... Minnie in tablet configuration should only wake from the keyboard or touchpad if it was in "display off" mode when it suspended (with an external display connected and the internal brightness at 0). If it wakes from keyboard/touchpad in other cases that's a bug.
,
Jan 13 2017
#13: Thanks! In the interest of correctness, I've uploaded https://chromium-review.googlesource.com/427288 to remove the usable_when_display_off tag from internal_touchscreen and internal_stylus.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/cc054fa3c0ec1eb2f36ad333362d606b8cc503dd commit cc054fa3c0ec1eb2f36ad333362d606b8cc503dd Author: Daniel Erat <derat@chromium.org> Date: Fri Jan 13 02:11:18 2017 power: Disable touchscreen and stylus when display is off. Remove the usable_when_display_off udev tag from internal_touchscreen and internal_stylus devices. BUG= chromium:661368 TEST=none Change-Id: I4982ecffd5c99381e6169e3ba8a7bc87647b7875 Reviewed-on: https://chromium-review.googlesource.com/427288 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org> [modify] https://crrev.com/cc054fa3c0ec1eb2f36ad333362d606b8cc503dd/power_manager/udev/91-powerd-tags.rules
,
Jan 26 2017
Verified on build 9000.76.0 |
||||
►
Sign in to add a comment |
||||
Comment 1 by derat@chromium.org
, Nov 1 2016