chromeos: Save display power state on config failure.

Make DisplayConfigurator save the requested display power
state even when it fails to configure CRTCs. This lets the
previously-requested state be used for future mode changes.

BUG=chrome-os-partner:31571

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

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

chrome/browser/chromeos/display/display_preferences.cc

@@ -308,7 +308,8 @@ void StoreDisplayPowerState(DisplayPowerState power_state) {
 
 void StoreCurrentDisplayPowerState() {
   StoreDisplayPowerState(
-      ash::Shell::GetInstance()->display_configurator()->power_state());
+      ash::Shell::GetInstance()->display_configurator()->
+          requested_power_state());
 }
 
 void StoreCurrentDisplayRotationLockPrefs() {

chrome/browser/chromeos/display/display_preferences_unittest.cc

@@ -191,7 +191,7 @@ TEST_F(DisplayPreferencesTest, PairedLayoutOverrides) {
   LoadDisplayPreferences(true);
   // DisplayPowerState should be ignored at boot.
   EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
-            shell->display_configurator()->power_state());
+            shell->display_configurator()->requested_power_state());
 
   shell->display_manager()->UpdateDisplays();
   // Check if the layout settings are notified to the system properly.
@@ -623,7 +623,8 @@ TEST_F(DisplayPreferencesTest, DisplayPowerStateAfterRestart) {
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
   LoadDisplayPreferences(false);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            ash::Shell::GetInstance()->display_configurator()->power_state());
+            ash::Shell::GetInstance()->display_configurator()->
+                requested_power_state());
 }
 
 TEST_F(DisplayPreferencesTest, DontSaveAndRestoreAllOff) {
@@ -633,12 +634,12 @@ TEST_F(DisplayPreferencesTest, DontSaveAndRestoreAllOff) {
   LoadDisplayPreferences(false);
   // DisplayPowerState should be ignored at boot.
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->power_state());
+            shell->display_configurator()->requested_power_state());
 
   StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_ALL_OFF);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->power_state());
+            shell->display_configurator()->requested_power_state());
   EXPECT_EQ("internal_off_external_on",
             local_state()->GetString(prefs::kDisplayPowerState));
 
@@ -646,7 +647,7 @@ TEST_F(DisplayPreferencesTest, DontSaveAndRestoreAllOff) {
   local_state()->SetString(prefs::kDisplayPowerState, "all_off");
   LoadDisplayPreferences(false);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->power_state());
+            shell->display_configurator()->requested_power_state());
 }
 
 // Tests that display configuration changes caused by MaximizeModeController

ui/display/chromeos/display_configurator.cc

@@ -160,7 +160,8 @@ DisplayConfigurator::DisplayConfigurator()
       is_panel_fitting_enabled_(false),
       configure_display_(base::SysInfo::IsRunningOnChromeOS()),
       display_state_(MULTIPLE_DISPLAY_STATE_INVALID),
-      power_state_(chromeos::DISPLAY_POWER_ALL_ON),
+      requested_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
+      current_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
       next_display_protection_client_id_(1) {}
 
 DisplayConfigurator::~DisplayConfigurator() {
@@ -179,7 +180,7 @@ void DisplayConfigurator::SetDelegateForTesting(
 void DisplayConfigurator::SetInitialDisplayPower(
     chromeos::DisplayPowerState power_state) {
   DCHECK_EQ(display_state_, MULTIPLE_DISPLAY_STATE_INVALID);
-  power_state_ = power_state;
+  requested_power_state_ = current_power_state_ = power_state;
 }
 
 void DisplayConfigurator::Init(bool is_panel_fitting_enabled) {
@@ -206,9 +207,10 @@ void DisplayConfigurator::ForceInitialConfigure(
   UpdateCachedDisplays();
   if (cached_displays_.size() > 1 && background_color_argb)
     native_display_delegate_->SetBackgroundColor(background_color_argb);
-  const MultipleDisplayState new_state = ChooseDisplayState(power_state_);
-  const bool success =
-      EnterStateOrFallBackToSoftwareMirroring(new_state, power_state_);
+  const MultipleDisplayState new_state = ChooseDisplayState(
+      requested_power_state_);
+  const bool success = EnterStateOrFallBackToSoftwareMirroring(
+      new_state, requested_power_state_);
 
   // Force the DPMS on chrome startup as the driver doesn't always detect
   // that all displays are on when signing out.
@@ -452,7 +454,8 @@ bool DisplayConfigurator::SetDisplayPower(
           << DisplayPowerStateToString(power_state) << " flags=" << flags
           << ", configure timer="
           << (configure_timer_.IsRunning() ? "Running" : "Stopped");
-  if (power_state == power_state_ && !(flags & kSetDisplayPowerForceProbe))
+  if (power_state == current_power_state_ &&
+      !(flags & kSetDisplayPowerForceProbe))
     return true;
 
   native_display_delegate_->GrabServer();
@@ -480,7 +483,7 @@ bool DisplayConfigurator::SetDisplayPower(
   native_display_delegate_->UngrabServer();
   if (attempted_change)
     NotifyObservers(success, new_state);
-  return true;
+  return success;
 }
 
 bool DisplayConfigurator::SetDisplayMode(MultipleDisplayState new_state) {
@@ -501,8 +504,8 @@ bool DisplayConfigurator::SetDisplayMode(MultipleDisplayState new_state) {
 
   native_display_delegate_->GrabServer();
   UpdateCachedDisplays();
-  const bool success =
-      EnterStateOrFallBackToSoftwareMirroring(new_state, power_state_);
+  const bool success = EnterStateOrFallBackToSoftwareMirroring(
+      new_state, requested_power_state_);
   native_display_delegate_->UngrabServer();
 
   NotifyObservers(success, new_state);
@@ -542,7 +545,7 @@ void DisplayConfigurator::SuspendDisplays() {
   // suspending.  This shouldn't be very noticeable to the user since the
   // backlight is off at this point, and doing this lets us resume directly
   // into the "on" state, which greatly reduces resume times.
-  if (power_state_ == chromeos::DISPLAY_POWER_ALL_OFF) {
+  if (requested_power_state_ == chromeos::DISPLAY_POWER_ALL_OFF) {
     SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
                     kSetDisplayPowerOnlyIfSingleInternalDisplay);
 
@@ -561,7 +564,7 @@ void DisplayConfigurator::ResumeDisplays() {
       base::TimeDelta::FromMilliseconds(kResumeDelayMs),
       base::Bind(base::IgnoreResult(&DisplayConfigurator::SetDisplayPower),
                  base::Unretained(this),
-                 power_state_,
+                 requested_power_state_,
                  kSetDisplayPowerForceProbe));
 }
 
@@ -713,9 +716,10 @@ void DisplayConfigurator::ConfigureDisplays() {
 
   native_display_delegate_->GrabServer();
   UpdateCachedDisplays();
-  const MultipleDisplayState new_state = ChooseDisplayState(power_state_);
-  const bool success =
-      EnterStateOrFallBackToSoftwareMirroring(new_state, power_state_);
+  const MultipleDisplayState new_state = ChooseDisplayState(
+      requested_power_state_);
+  const bool success = EnterStateOrFallBackToSoftwareMirroring(
+      new_state, requested_power_state_);
   native_display_delegate_->UngrabServer();
 
   NotifyObservers(success, new_state);
@@ -741,7 +745,7 @@ bool DisplayConfigurator::EnterStateOrFallBackToSoftwareMirroring(
     bool enable_software_mirroring = false;
     if (!success && display_state == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR) {
       if (display_state_ != MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ||
-          power_state_ != power_state)
+          current_power_state_ != power_state)
         EnterState(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED, power_state);
       enable_software_mirroring = success =
           display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED;
@@ -759,6 +763,9 @@ bool DisplayConfigurator::EnterState(MultipleDisplayState display_state,
   VLOG(1) << "EnterState: display=" << DisplayStateToString(display_state)
           << " power=" << DisplayPowerStateToString(power_state);
 
+  // Save the requested state so we'll try to use it next time even if we fail.
+  requested_power_state_ = power_state;
+
   // Framebuffer dimensions.
   gfx::Size size;
 
@@ -925,7 +932,7 @@ bool DisplayConfigurator::EnterState(MultipleDisplayState display_state,
 
   if (all_succeeded) {
     display_state_ = display_state;
-    power_state_ = power_state;
+    current_power_state_ = power_state;
     framebuffer_size_ = size;
   }
   return all_succeeded;

ui/display/chromeos/display_configurator.h

@@ -137,7 +137,9 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   virtual ~DisplayConfigurator();
 
   MultipleDisplayState display_state() const { return display_state_; }
-  chromeos::DisplayPowerState power_state() const { return power_state_; }
+  chromeos::DisplayPowerState requested_power_state() const {
+    return requested_power_state_;
+  }
   const gfx::Size framebuffer_size() const { return framebuffer_size_; }
   const std::vector<DisplayState>& cached_displays() const {
     return cached_displays_;
@@ -174,7 +176,8 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // Called when powerd notifies us that some set of displays should be turned
   // on or off.  This requires enabling or disabling the CRTC associated with
   // the display(s) in question so that the low power state is engaged.
-  // |flags| contains bitwise-or-ed kSetDisplayPower* values.
+  // |flags| contains bitwise-or-ed kSetDisplayPower* values. Returns true if
+  // the system successfully enters (or was already in) |power_state|.
   bool SetDisplayPower(chromeos::DisplayPowerState power_state, int flags);
 
   // Force switching the display mode to |new_state|. Returns false if
@@ -320,8 +323,11 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
 
   gfx::Size framebuffer_size_;
 
-  // The current power state.
-  chromeos::DisplayPowerState power_state_;
+  // The last-requested and current power state. These may differ if
+  // configuration fails: SetDisplayMode() needs the last-requested state while
+  // SetDisplayPower() needs the current state.
+  chromeos::DisplayPowerState requested_power_state_;
+  chromeos::DisplayPowerState current_power_state_;
 
   // Most-recently-used display configuration. Note that the actual
   // configuration changes asynchronously.

ui/display/chromeos/display_configurator_unittest.cc

@@ -1233,8 +1233,6 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
     outputs_[i].set_native_mode(modes[0]);
   }
 
-  configurator_.Init(false);
-
   // First test simply fails in MULTIPLE_DISPLAY_STATE_SINGLE mode. This is
   // probably unrealistic but we want to make sure any assumptions don't creep
   // in.
@@ -1302,4 +1300,42 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
       log_->GetActionsAndClear());
 }
 
+// Tests that power state requests are saved after failed configuration attempts
+// so they can be reused later: http://crosbug.com/p/31571
+TEST_F(DisplayConfiguratorTest, SaveDisplayPowerStateOnConfigFailure) {
+  // Start out with two displays in extended mode.
+  state_controller_.set_state(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED);
+  configurator_.Init(false);
+  configurator_.ForceInitialConfigure(0);
+  log_->GetActionsAndClear();
+
+  // Turn off the internal display, simulating docked mode.
+  EXPECT_TRUE(configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags));
+  log_->GetActionsAndClear();
+
+  // Make all subsequent configuration requests fail and try to turn the
+  // internal display back on.
+  native_display_delegate_->set_max_configurable_pixels(1);
+  EXPECT_FALSE(configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_ALL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags));
+  log_->GetActionsAndClear();
+
+  // Simulate the external display getting disconnected and check that the
+  // internal display is turned on (i.e. DISPLAY_POWER_ALL_ON is used) rather
+  // than the earlier DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON state.
+  native_display_delegate_->set_max_configurable_pixels(0);
+  UpdateOutputs(1, true);
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(small_mode_.size(), &outputs_[0], NULL).c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          kUngrab,
+          NULL),
+      log_->GetActionsAndClear());
+}
+
 }  // namespace ui