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 13 matching lines @@
 #include "ui/display/types/display_snapshot.h"
 #include "ui/display/types/native_display_delegate.h"
 #include "ui/display/util/display_util.h"
 
 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 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 13 matching lines @@
 bool DisplayConfigurator::TestApi::TriggerConfigureTimeout() {
   if (configurator_->configure_timer_.IsRunning()) {
     configurator_->configure_timer_.user_task().Run();
     configurator_->configure_timer_.Stop();
     return true;
   } else {
     return false;
   }
 }
 
+base::TimeDelta DisplayConfigurator::TestApi::GetConfigureDelay() const {
+  return configurator_->configure_timer_.IsRunning()
+             ? configurator_->configure_timer_.GetCurrentDelay()
+             : base::TimeDelta();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // DisplayConfigurator::DisplayLayoutManagerImpl implementation
 
 class DisplayConfigurator::DisplayLayoutManagerImpl
     : public DisplayLayoutManager {
  public:
   DisplayLayoutManagerImpl(DisplayConfigurator* configurator);
   ~DisplayLayoutManagerImpl() override;
 
   // DisplayConfigurator::DisplayLayoutManager:
@@ skipping 774 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);
 
+  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();
 }
 
 void DisplayConfigurator::SetDisplayPower(
     chromeos::DisplayPowerState power_state,
     int flags,
     const ConfigurationCallback& callback) {
   if (!configure_display_ || display_externally_controlled_) {
     callback.Run(false);
     return;
@@ skipping 34 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.
+  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.

[Daniel Erat, 2016/10/31 16:20:01]

i don't understand this condition or comment. the timer appears to only be
started within this class with kConfigureDelayMs (500) or
kResumeConfigureMultiDisplayDelayMs (2000). this condition starts the timer here
with its current delay (which i think would always be 500), and then the 'else'
block also starts the timer with 500. it seems like this if/else block can just
be replaced with a single call to Start() using 500.

or did you mean '>', so we'll preserve already-set longer delays?

[afakhry, 2016/10/31 17:37:19]

Your observation here is correct. We don't need this condition.
No, I didn't want to preserve the longer delays, rather, I didn't want to make
them even longer by resetting (e.g. when we have been waiting for 1.5s [out of
2s] and OnConfigurationChanged() event is received, I didn't want to reset the
timer back to 2s [to avoid a long delay of 3.5s]).

     configure_timer_.Reset();
   } else {
     configure_timer_.Start(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
         this,
         &DisplayConfigurator::ConfigureDisplays);
   }
 }
 
@@ skipping 29 matching lines @@
   // completes before we return.
   native_display_delegate_->SyncWithServer();
 }
 
 void DisplayConfigurator::ResumeDisplays() {
   if (!configure_display_ || display_externally_controlled_)
     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.

[Daniel Erat, 2016/10/31 16:20:02]

nit: also mention why this is done at the end of the comment: "... so we won't
immediately resize the desktops and the windows on it to fit on a single
display."

[afakhry, 2016/10/31 17:37:19]

Done.

+    configure_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
+                                          kResumeConfigureMultiDisplayDelayMs),
+                           this, &DisplayConfigurator::ConfigureDisplays);
+  }
+
   // 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));
 }
 
 void DisplayConfigurator::ConfigureDisplays() {
   if (!configure_display_ || display_externally_controlled_)
     return;
 
@@ skipping 174 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