Fix queuing of display power state change requests

If a display power change request arrives before the previous one
finishes or if a previous power change request failed the state becomes
inconsistent and future requests are optimized out.

BUG=627795
TEST=New unittest in display_unittests and manually checked on Samus

Review-Url: https://codereview.chromium.org/2154743003
Cr-Commit-Position: refs/heads/master@{#405990}

ui/display/chromeos/display_configurator.cc

@@ -844,7 +844,13 @@ void DisplayConfigurator::SetDisplayPowerInternal(
     chromeos::DisplayPowerState power_state,
     int flags,
     const ConfigurationCallback& callback) {
+  // Only skip if the current power state is the same and the latest requested
+  // power state is the same. If |pending_power_state_ != current_power_state_|
+  // then there is a current task pending or the last configuration failed. In
+  // either case request a new configuration to make sure the state is
+  // consistent with the expectations.
   if (power_state == current_power_state_ &&
+      power_state == pending_power_state_ &&
       !(flags & kSetDisplayPowerForceProbe)) {
     callback.Run(true);
     return;

ui/display/chromeos/display_configurator_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/chromeos/test/action_logger_util.h"
 #include "ui/display/chromeos/test/test_display_snapshot.h"
@@ -1675,5 +1676,166 @@ TEST_F(DisplayConfiguratorTest, ExternalControl) {
             log_->GetActionsAndClear());
 }
 
+TEST_F(DisplayConfiguratorTest,
+       SetDisplayPowerWhilePendingConfigurationTaskRunning) {
+  // Start out with two displays in extended mode.
+  state_controller_.set_state(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED);
+  Init(false);
+  configurator_.ForceInitialConfigure(0);
+  log_->GetActionsAndClear();
+  observer_.Reset();
+
+  native_display_delegate_->set_run_async(true);
+
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_OFF,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+
+  EXPECT_EQ(CALLBACK_NOT_CALLED, PopCallbackResult());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_EQ(1, observer_.num_changes());
+  EXPECT_EQ(0, observer_.num_failures());
+
+  const int kDualHeight = small_mode_.size().height() +
+                          DisplayConfigurator::kVerticalGap +
+                          big_mode_.size().height();
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
+                               &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], nullptr, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], nullptr,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          kUngrab, NULL),
+      log_->GetActionsAndClear());
+
+  EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_EQ(2, observer_.num_changes());
+  EXPECT_EQ(0, observer_.num_failures());
+
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
+                               &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], &big_mode_,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          kForceDPMS, kUngrab, NULL),
+      log_->GetActionsAndClear());
+}
+
+TEST_F(DisplayConfiguratorTest,
+       SetDisplayPowerAfterFailedDisplayConfiguration) {
+  // Start out with two displays in extended mode.
+  state_controller_.set_state(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED);
+  Init(false);
+  configurator_.ForceInitialConfigure(0);
+  log_->GetActionsAndClear();
+  observer_.Reset();
+
+  // Fail display configuration.
+  native_display_delegate_->set_max_configurable_pixels(-1);
+
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_OFF,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+
+  EXPECT_EQ(CALLBACK_FAILURE, PopCallbackResult());
+  EXPECT_EQ(0, observer_.num_changes());
+  EXPECT_EQ(1, observer_.num_failures());
+
+  const int kDualHeight = small_mode_.size().height() +
+                          DisplayConfigurator::kVerticalGap +
+                          big_mode_.size().height();
+
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
+                               &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], nullptr, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], nullptr,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          kUngrab, NULL),
+      log_->GetActionsAndClear());
+
+  // This configuration should trigger a display configuration since the
+  // previous configuration failed.
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags,
+      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
+                 base::Unretained(this)));
+
+  EXPECT_EQ(0, observer_.num_changes());
+  EXPECT_EQ(2, observer_.num_failures());
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
+                               &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], &big_mode_,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          GetCrtcAction(outputs_[1], &small_mode_,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          kUngrab, NULL),
+      log_->GetActionsAndClear());
+
+  // Allow configuration to succeed.
+  native_display_delegate_->set_max_configurable_pixels(0);
+
+  // Validate that a configuration event has the proper power state (displays
+  // should be on).
+  configurator_.OnConfigurationChanged();
+  EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
+
+  EXPECT_EQ(1, observer_.num_changes());
+  EXPECT_EQ(2, observer_.num_failures());
+
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
+                               &outputs_[0], &outputs_[1])
+              .c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          GetCrtcAction(outputs_[1], &big_mode_,
+                        gfx::Point(0, small_mode_.size().height() +
+                                          DisplayConfigurator::kVerticalGap))
+              .c_str(),
+          kUngrab, NULL),
+      log_->GetActionsAndClear());
+}
+
 }  // namespace test
 }  // namespace ui