Delay display configuration after waking up from suspend with multi displays

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 6c3062317944388f13615d2e7ae7d6db5a276a5a..0887bd57f176e48852fbf9d215d8192cd08fb2a3 100644
--- a/ui/display/chromeos/display_configurator.cc
+++ b/ui/display/chromeos/display_configurator.cc
@@ -31,10 +31,6 @@ namespace {
 
 typedef std::vector<const DisplayMode*> DisplayModeList;
 
-// The delay to perform configuration after RRNotify. See the comment for
-// |configure_timer_|.
-const int kConfigureDelayMs = 500;
-
 // The EDID specification marks the top bit of the manufacturer id as reserved.
 const int16_t kReservedManufacturerID = static_cast<int16_t>(1 << 15);
 
@@ -68,6 +64,12 @@ bool DisplayConfigurator::TestApi::TriggerConfigureTimeout() {
   }
 }
 
+base::TimeDelta DisplayConfigurator::TestApi::GetConfigureDelay() const {
+  return configurator_->configure_timer_.IsRunning()
+             ? configurator_->configure_timer_.GetCurrentDelay()
+             : base::TimeDelta();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // DisplayConfigurator::DisplayLayoutManagerImpl implementation
 
@@ -862,6 +864,14 @@ void DisplayConfigurator::SetDisplayPowerInternal(
   pending_power_flags_ = flags;
   queued_configuration_callbacks_.push_back(callback);
 
+  if (configure_timer_.IsRunning()) {
+    // If there is a configuration task scheduled, avoid performing
+    // configuration immediately. Instead reset the timer to wait for things to
+    // settle.
+    configure_timer_.Reset();
+    return;
+  }
+
   RunPendingConfiguration();
 }
 
@@ -916,13 +926,11 @@ void DisplayConfigurator::OnConfigurationChanged() {
 
   // Configure displays with |kConfigureDelayMs| delay,
   // so that time-consuming ConfigureDisplays() won't be called multiple times.
-  if (configure_timer_.IsRunning()) {
-    // Note: when the timer is running it is possible that a different task
-    // (RestoreRequestedPowerStateAfterResume()) is scheduled. In these cases,
-    // prefer the already scheduled task to ConfigureDisplays() since
-    // ConfigureDisplays() performs only basic configuration while
-    // RestoreRequestedPowerStateAfterResume() will perform additional
-    // operations.
+  if (configure_timer_.IsRunning() &&
+      configure_timer_.GetCurrentDelay() <=
+          base::TimeDelta::FromMilliseconds(kConfigureDelayMs)) {
+    // Avoid resetting the timer to a huge delay if we are currently pending
+    // the delayed configuration after waking up from suspend.
     configure_timer_.Reset();
   } else {
     configure_timer_.Start(
@@ -966,16 +974,31 @@ void DisplayConfigurator::SuspendDisplays(
   native_display_delegate_->SyncWithServer();
 }
 
-void DisplayConfigurator::ResumeDisplays() {
-  if (!configure_display_ || display_externally_controlled_)
+void DisplayConfigurator::ResumeDisplays(
+    const ConfigurationCallback& callback) {
+  if (!configure_display_ || display_externally_controlled_) {
+    callback.Run(false);
     return;
+  }
 
   displays_suspended_ = false;
 
+  if (current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR ||
+      current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ||
+      current_display_state_ == MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED) {
+    // When waking up from suspend while being in a multi display mode, we
+    // schedule a delayed forced configuration, which will make
+    // SetDisplayPowerInternal() avoid performing the configuration immediately.
+    // This gives a chance to wait for all displays to be added and detected
+    // before configuration is performed.
+    configure_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
+                                          kResumeConfigureMultiDisplayDelayMs),
+                           this, &DisplayConfigurator::ConfigureDisplays);
+  }

[marcheu1, 2016/10/25 00:41:11]

why not do this unconditionally? For example if you have a chromebox with 1
monitor, the postponed configuration would also be interesting.

[afakhry, 2016/10/25 20:14:10]

+derat@ What do you think of this suggestion?
I think the long delay here isn't very useful when we have a single monitor.

[Daniel Erat, 2016/10/26 15:51:55]

it seems okay to me if it simplifies the code or makes it more robust. are there
any situations where a chromebox resumes with a single external display and
fails to find it, where we might benefit from scheduling this extra config?

[afakhry, 2016/10/27 17:37:12]

I just discussed with Oshima. We think that the delayed configuration only makes
sense when we have 2+ monitors. The issue we're trying to solve is preventing us
from detecting a single display only first (wrongly) then later detect other
displays (windows will move to the wrong display and stay there). But when we
have a single monitor, there's no problem detecting only one first, and
connecting others later if something was connected during suspend.

+
   // If requested_power_state_ is ALL_OFF due to idle suspend, powerd will turn
   // the display power on when it enables the backlight.
-  SetDisplayPower(requested_power_state_, kSetDisplayPowerNoFlags,
-                  base::Bind(&DoNothing));
+  SetDisplayPower(requested_power_state_, kSetDisplayPowerNoFlags, callback);
 }
 
 void DisplayConfigurator::ConfigureDisplays() {