Move check for suspended displays out of common path

Checking if the displays are suspended in
DisplayConfigurator::RunPendingConfiguration is exposing a race
condition in the interaction between chrome and powerd.  The original
reason for adding the check was to ensure that chrome did not attempt to
perform any modesets while the chrome os device was in a dark resume.
Since this really only happens when the display configuration changes,
we can move the check into OnConfigurationChanged.  Also, to deal with
the possbility that the configuration timer was started right before the
system suspended, stop the timer in SuspendDisplays.  The
DisplayConfigurator will force a probe and re-configuration on resume
anyway so stopping the timer at suspend time will not really make a
difference.

BUG=chrome-os-partner:36723,459783

Review URL: https://codereview.chromium.org/932203002

Cr-Commit-Position: refs/heads/master@{#316918}

ui/display/chromeos/display_configurator.cc

@@ -814,6 +814,14 @@ void DisplayConfigurator::SetDisplayMode(MultipleDisplayState new_state) {
 }
 
 void DisplayConfigurator::OnConfigurationChanged() {
+  // 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()) {
@@ -859,6 +867,10 @@ void DisplayConfigurator::SuspendDisplays() {
   }
 
   displays_suspended_ = true;
+
+  // Stop |configure_timer_| because we will force probe and configure all the
+  // displays at resume time anyway.
+  configure_timer_.Stop();
 }
 
 void DisplayConfigurator::ResumeDisplays() {
@@ -880,14 +892,6 @@ void DisplayConfigurator::ConfigureDisplays() {
 }
 
 void DisplayConfigurator::RunPendingConfiguration() {
-  // 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;
-  }
-
   // Configuration task is currently running. Do not start a second
   // configuration.
   if (configuration_task_)

ui/display/chromeos/display_configurator_unittest.cc

@@ -975,10 +975,59 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
   configurator_.SuspendDisplays();
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
+  // The configuration timer should not be started when the displays
+  // are suspended.
   configurator_.OnConfigurationChanged();
-  EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
+  EXPECT_FALSE(test_api_.TriggerConfigureTimeout());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
+  // Calls to SetDisplayPower and SetDisplayMode should be successful.
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_OFF,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(small_mode_.size(), &outputs_[0], NULL).c_str(),
+          GetCrtcAction(outputs_[0], NULL, gfx::Point(0, 0)).c_str(),
+          kUngrab,
+          NULL),
+      log_->GetActionsAndClear());
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(small_mode_.size(), &outputs_[0], NULL).c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          kForceDPMS,
+          kUngrab,
+          NULL),
+      log_->GetActionsAndClear());
+
+  UpdateOutputs(2, false);
+  configurator_.SetDisplayMode(MULTIPLE_DISPLAY_STATE_DUAL_MIRROR);
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(small_mode_.size(), &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], &small_mode_, gfx::Point(0, 0)).c_str(),
+          kUngrab,
+          NULL),
+      log_->GetActionsAndClear());
+
+  // The DisplayConfigurator should force a probe and reconfiguration at resume
+  // time.
+  UpdateOutputs(1, false);
   configurator_.ResumeDisplays();
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
   EXPECT_EQ(
@@ -992,12 +1041,12 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
       log_->GetActionsAndClear());
 
   // If a configuration task is pending when the displays are suspended, that
-  // task should not run either.
+  // task should not run either and the timer should be stopped.
   configurator_.OnConfigurationChanged();
   configurator_.SuspendDisplays();
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
-  EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
+  EXPECT_FALSE(test_api_.TriggerConfigureTimeout());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
   configurator_.ResumeDisplays();