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.
     configure_timer_.Reset();
   } else {
     configure_timer_.Start(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
         this,
         &DisplayConfigurator::ConfigureDisplays);
   }
 }
 
@@ skipping 23 matching lines @@
   // 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() {
-  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);

[Daniel Erat, 2016/10/25 16:05:32]

this is part of why i'm confused (see question in test file). should the
callback be queued in this case instead of passed to SetDisplayPower below?

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

(Also see my answer in the test file).

This is a bit tricky! The way I understand it is that (for tests) the delayed
configuration callback is only interesting when SetDisplayPower() is not a no-op
(i.e. when there is a change). When SetDisplayPower() is not a no-op, it will
enqueue the callback itself.
See the following example:

1- We call ResumeDisplays() while in dual display mode, power is still off (no
change from current/pending). Configuration is scheduled 2 seconds later.
   1.1 - SetDisplayPower() is called. It's a no-op. callback is invoked
immediately as success. We don't care about the delayed configuration (yet).
2- Now we simulate ChromeDisplayPowerServiceProviderDelegate::SetDisplayPower()
turning the displays on by calling SetDisplayPower() manually with ALL_ON.
   2.1 - This is not a no-op. Configuration timer is reset, callback is enqueued
and will be invoked when the configuration is done later.

Sorry if it's confusing. That's the best way I can explain it.

+  }
+
   // 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() {
   if (!configure_display_ || display_externally_controlled_)
     return;
 
   force_configure_ = true;
   RunPendingConfiguration();
 }
 
@@ skipping 170 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