Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/display/chromeos/display_configurator.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 17 matching lines @@
 namespace ui {
 
 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 delay to perform configuration after waking up from suspend when in dual
+// display mode. Should be bigger than |kConfigureDelayMs|. Generally big enough
+// for external displays to be detected and added.

[oshima, 2016/10/18 22:00:05]

please add reference to the bug

[afakhry, 2016/10/21 20:03:03]

Done.

+const int kConfigureDualDisplayAfterSuspendDelayMs = 2000;
+
 // The EDID specification marks the top bit of the manufacturer id as reserved.
 const int16_t kReservedManufacturerID = static_cast<int16_t>(1 << 15);
 
 struct DisplayState {
   DisplaySnapshot* display = nullptr;  // Not owned.
 
   // User-selected mode for the display.
   const DisplayMode* selected_mode = nullptr;
 
   // Mode used when displaying the same desktop on multiple displays.
@@ skipping 807 matching lines @@
       !(flags & kSetDisplayPowerForceProbe)) {
     callback.Run(true);
     return;
   }
 
   pending_power_state_ = power_state;
   has_pending_power_state_ = true;
   pending_power_flags_ = flags;
   queued_configuration_callbacks_.push_back(callback);
 
-  RunPendingConfiguration();
+  if (power_state == chromeos::DISPLAY_POWER_ALL_ON &&

[dnicoara, 2016/10/19 14:42:41]

I don't think this is the correct place to start the timer since
SetDisplayPowerInternal() is called in other circumstances as well. You want to
set the timer in ResumeDisplays() to limit it to the resume case.

[Daniel Erat, 2016/10/19 16:57:56]

good catch; this seems wrong to me too. when powerd has turned the displays off
due to the user being inactive but not yet suspended, and the user then hits a
key, powerd makes a d-bus call that results in SetDisplayPower being called. we
don't want to delay turning them on there.

mind trying to add a test that would've caught this regression?

[afakhry, 2016/10/21 20:03:03]

Done.

[afakhry, 2016/10/21 20:03:03]

Thanks for catching that. Done. I modified the unit tests to validate that the
delay is kept at a minimum.

+      (current_display_state_ == MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED ||
+       current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR)) {
+    // When waking up from suspend while being in a dual display mode, we
+    // perform a forced configuration after a delay to wait for all displays to
+    // be added and detected.
+    configure_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromMilliseconds(
+                               kConfigureDualDisplayAfterSuspendDelayMs),
+                           this, &DisplayConfigurator::ConfigureDisplays);
+  } else {
+    RunPendingConfiguration();
+  }
 }
 
 void DisplayConfigurator::SetDisplayPower(
     chromeos::DisplayPowerState power_state,
     int flags,
     const ConfigurationCallback& callback) {
   if (!configure_display_ || display_externally_controlled_) {
     callback.Run(false);
     return;
   }
@@ skipping 33 matching lines @@
   // Don't do anything if the displays are currently suspended.  Instead we will
   // probe and reconfigure the displays if necessary in ResumeDisplays().
   if (displays_suspended_) {
     VLOG(1) << "Displays are currently suspended.  Not attempting to "
             << "reconfigure them.";
     return;
   }
 
   // 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.

[afakhry, 2016/10/17 22:35:28]

Outdated comment? Do we still need the below Reset()?

[oshima, 2016/10/18 22:00:05]

+derat

+  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(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
         this,
         &DisplayConfigurator::ConfigureDisplays);
   }
 }
 
@@ skipping 224 matching lines @@
   last_virtual_display_id_ = max_display_id & 0xff;
 
   return true;
 }
 
 bool DisplayConfigurator::IsDisplayOn() const {
   return current_power_state_ != chromeos::DISPLAY_POWER_ALL_OFF;
 }
 
 }  // namespace ui