chromeos: Turn off displays on suspend

ui/display/chromeos/display_configurator.cc

Index: ui/display/chromeos/display_configurator.cc
diff --git a/ui/display/chromeos/display_configurator.cc b/ui/display/chromeos/display_configurator.cc
index e1def2037131a0edf7611ec5def3d6eca7f2aba3..3bf06e7435b78d3853aa2e4e63a43cdcf6f14308 100644
--- a/ui/display/chromeos/display_configurator.cc
+++ b/ui/display/chromeos/display_configurator.cc
@@ -35,12 +35,6 @@ typedef std::vector<const DisplayMode*> DisplayModeList;
 // |configure_timer_|.
 const int kConfigureDelayMs = 500;
 
-// The delay spent before reading the display configuration after coming out of
-// suspend. While coming out of suspend the display state may be updating. This
-// is used to wait until the hardware had a chance to update the display state
-// such that we read an up to date state.
-const int kResumeDelayMs = 500;
-
 // The EDID specification marks the top bit of the manufacturer id as reserved.
 const int16_t kReservedManufacturerID = static_cast<int16_t>(1 << 15);
 
@@ -476,8 +470,9 @@ DisplayConfigurator::DisplayConfigurator()
       current_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
       requested_display_state_(MULTIPLE_DISPLAY_STATE_INVALID),
       requested_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
-      requested_power_state_change_(false),
-      requested_power_flags_(kSetDisplayPowerNoFlags),
+      config_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
+      power_state_change_(false),
+      config_power_flags_(kSetDisplayPowerNoFlags),
       force_configure_(false),
       next_display_protection_client_id_(1),
       display_externally_controlled_(false),
@@ -844,6 +839,24 @@ void DisplayConfigurator::PrepareForExit() {
   configure_display_ = false;
 }
 
+void DisplayConfigurator::SetDisplayPowerInternal(
+    chromeos::DisplayPowerState power_state,
+    int flags,
+    const ConfigurationCallback& callback) {
+  if (power_state == current_power_state_ &&
+      !(flags & kSetDisplayPowerForceProbe)) {
+    callback.Run(true);
+    return;
+  }
+
+  config_power_state_ = power_state;
+  power_state_change_ = true;
+  config_power_flags_ = flags;
+  queued_configuration_callbacks_.push_back(callback);
+
+  RunPendingConfiguration();
+}
+
 void DisplayConfigurator::SetDisplayPower(
     chromeos::DisplayPowerState power_state,
     int flags,
@@ -857,18 +870,9 @@ void DisplayConfigurator::SetDisplayPower(
           << DisplayPowerStateToString(power_state) << " flags=" << flags
           << ", configure timer="
           << (configure_timer_.IsRunning() ? "Running" : "Stopped");
-  if (power_state == requested_power_state_ &&
-      !(flags & kSetDisplayPowerForceProbe)) {
-    callback.Run(true);
-    return;
-  }
 
   requested_power_state_ = power_state;
-  requested_power_state_change_ = true;
-  requested_power_flags_ = flags;
-  queued_configuration_callbacks_.push_back(callback);
-
-  RunPendingConfiguration();
+  SetDisplayPowerInternal(requested_power_state_, flags, callback);
 }
 
 void DisplayConfigurator::SetDisplayMode(MultipleDisplayState new_state) {
@@ -931,38 +935,37 @@ void DisplayConfigurator::RemoveObserver(Observer* observer) {
 
 void DisplayConfigurator::SuspendDisplays(
     const ConfigurationCallback& callback) {
-  // If the display is off due to user inactivity and there's only a single
-  // internal display connected, switch to the all-on state before
-  // suspending.  This shouldn't be very noticeable to the user since the
-  // backlight is off at this point, and doing this lets us resume directly
-  // into the "on" state, which greatly reduces resume times.
-  if (requested_power_state_ == chromeos::DISPLAY_POWER_ALL_OFF) {
-    SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
-                    kSetDisplayPowerOnlyIfSingleInternalDisplay, callback);
-
-    // We need to make sure that the monitor configuration we just did actually
-    // completes before we return, because otherwise the X message could be
-    // racing with the HandleSuspendReadiness message.
-    native_display_delegate_->SyncWithServer();
-  } else {
-    callback.Run(true);
-  }
-
   displays_suspended_ = true;
 
   // Stop |configure_timer_| because we will force probe and configure all the
   // displays at resume time anyway.
   configure_timer_.Stop();
+
+  // Turn off the displays for suspend. This way, if we wake up for lucid sleep,
+  // the displays will not turn on (all displays should be off for lucid sleep
+  // unless explicitly requested by lucid sleep code). Use
+  // SetDisplayPowerInternal so requested_power_state_ is maintained.
+  SetDisplayPowerInternal(chromeos::DISPLAY_POWER_ALL_OFF,
+                          kSetDisplayPowerNoFlags, callback);
+
+  // We need to make sure that the monitor configuration we just did actually
+  // completes before we return.
+  native_display_delegate_->SyncWithServer();
 }
 
 void DisplayConfigurator::ResumeDisplays() {
-  displays_suspended_ = false;
+  chromeos::DisplayPowerState power_state = chromeos::DISPLAY_POWER_ALL_ON;
+
+  // Restore requested_power_state_ unless it's DISPLAY_POWER_ALL_OFF.
+  if (requested_power_state_ != chromeos::DISPLAY_POWER_ALL_OFF) {

[Daniel Erat, 2016/04/21 22:16:41]

can you explain this to me again (and also update this comment to explain it,
instead of just describing what the next two lines do :-P)? i didn't fully
understand it the previous time. if the user manually set the brightness to zero
before suspending, isn't that getting dropped here?

[dbasehore, 2016/04/21 22:35:10]

Might be able to get rid of this. If we call SetDisplayPower in powerd when we
turn on the backlight, that may have similar latency to setting it here. We turn
the backlight back on in powerd right after announcing resume. We may have to
resume the backlight before un-muting the audio in powerd since the audio call
blocks, though.

I'll remove if it turns out to work well.

+    power_state = requested_power_state_;
+  }
 
-  configure_timer_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromMilliseconds(kResumeDelayMs),
-      base::Bind(&DisplayConfigurator::RestoreRequestedPowerStateAfterResume,
-                 base::Unretained(this)));
+  displays_suspended_ = false;
+  // If we aren't restoring requested_power_state_, we need to make sure we
+  // overwrite it with DISPLAY_POWER_ALL_ON, so call SetDisplayPower.
+  SetDisplayPower(power_state, kSetDisplayPowerForceProbe,

[Daniel Erat, 2016/04/21 22:16:41]

it seems weird to call the public method here. if you really want to update
requested_power_state_, why not just do that in an else block above instead of
by calling the public function for something that wasn't a user request?

[dbasehore, 2016/04/21 22:35:10]

Works for me. Will do.

+                  base::Bind(&DoNothing));
 }
 
 void DisplayConfigurator::ConfigureDisplays() {
@@ -988,7 +991,7 @@ void DisplayConfigurator::RunPendingConfiguration() {
 
   configuration_task_.reset(new UpdateDisplayConfigurationTask(
       native_display_delegate_.get(), layout_manager_.get(),
-      requested_display_state_, requested_power_state_, requested_power_flags_,
+      requested_display_state_, config_power_state_, config_power_flags_,
       0, force_configure_, base::Bind(&DisplayConfigurator::OnConfigured,
                                       weak_ptr_factory_.GetWeakPtr())));
   configuration_task_->set_virtual_display_snapshots(
@@ -997,8 +1000,8 @@ void DisplayConfigurator::RunPendingConfiguration() {
   // Reset the flags before running the task; otherwise it may end up scheduling
   // another configuration.
   force_configure_ = false;
-  requested_power_flags_ = kSetDisplayPowerNoFlags;
-  requested_power_state_change_ = false;
+  config_power_flags_ = kSetDisplayPowerNoFlags;
+  power_state_change_ = false;
   requested_display_state_ = MULTIPLE_DISPLAY_STATE_INVALID;
 
   DCHECK(in_progress_configuration_callbacks_.empty());
@@ -1038,7 +1041,7 @@ void DisplayConfigurator::OnConfigured(
     // gets updated as well since the last requested value may have been
     // dependent on certain conditions (ie: if only the internal monitor was
     // present).
-    if (!requested_power_state_change_)
+    if (!power_state_change_)
       requested_power_state_ = new_power_state;
 
     if (old_power_state != current_power_state_)
@@ -1072,7 +1075,7 @@ bool DisplayConfigurator::ShouldRunConfigurationTask() const {
     return true;
 
   // Schedule if there is a request to change the power state.
-  if (requested_power_state_change_)
+  if (power_state_change_)
     return true;
 
   return false;
@@ -1092,13 +1095,6 @@ void DisplayConfigurator::CallAndClearQueuedCallbacks(bool success) {
   queued_configuration_callbacks_.clear();
 }
 
-void DisplayConfigurator::RestoreRequestedPowerStateAfterResume() {
-  // Force probing to ensure that we pick up any changes that were made while
-  // the system was suspended.
-  SetDisplayPower(requested_power_state_, kSetDisplayPowerForceProbe,
-                  base::Bind(&DoNothing));
-}
-
 void DisplayConfigurator::NotifyDisplayStateObservers(
     bool success,
     MultipleDisplayState attempted_state) {