Reconfigure displays even if the output count didn't change

chromeos/display/output_configurator.cc

 // Copyright (c) 2012 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 "chromeos/display/output_configurator.h"
 
 #include <cmath>
 
 #include <X11/Xatom.h>
 #include <X11/Xlib.h>
@@ skipping 354 matching lines @@
         // such that one is primary and another is correctly positioned as
         // secondary.  If any of these assumptions are false, this is an unknown
         // configuration.
         bool primary_native = (primary_mode == outputs[0].native_mode) ||
             (primary_mode == None);
         bool secondary_native = (secondary_mode == outputs[1].native_mode) ||
             (secondary_mode == None);
         if (primary_native && secondary_native) {
           // Just check the relative locations.
           int secondary_offset = outputs[0].height + kVerticalGap;
-          int primary_offset = outputs[1].height + kVerticalGap;
           if ((outputs[0].y == 0) && (outputs[1].y == secondary_offset)) {
-            state = STATE_DUAL_PRIMARY_ONLY;
-          } else if ((outputs[1].y == 0) && (outputs[0].y == primary_offset)) {
-            state = STATE_DUAL_SECONDARY_ONLY;
+            state = STATE_DUAL_EXTENDED;
           } else {
             // Unexpected locations.
             state = STATE_DUAL_UNKNOWN;
           }
         } else {
           // Mode assumptions don't hold.
           state = STATE_DUAL_UNKNOWN;
         }
       }
       break;
@@ skipping 15 matching lines @@
     case 0:
       state = STATE_HEADLESS;
       break;
     case 1:
       state = STATE_SINGLE;
       break;
     case 2: {
       bool mirror_supported = (0 != outputs[0].mirror_mode) &&
           (0 != outputs[1].mirror_mode);
       switch (current_state) {
-        case STATE_DUAL_PRIMARY_ONLY:
+        case STATE_DUAL_EXTENDED:
           state =
-              mirror_supported ? STATE_DUAL_MIRROR : STATE_DUAL_PRIMARY_ONLY;
+              mirror_supported ? STATE_DUAL_MIRROR : STATE_DUAL_EXTENDED;
           break;
         case STATE_DUAL_MIRROR:
-          state = STATE_DUAL_PRIMARY_ONLY;
+          state = STATE_DUAL_EXTENDED;
           break;
         default:
           // Default to primary only.
-          state = STATE_DUAL_PRIMARY_ONLY;
+          state = STATE_DUAL_EXTENDED;
       }
       break;
     }
     default:
       CHECK(false);
   }
   return state;
 }
 
 RRCrtc GetNextCrtcAfter(Display* display,
@@ skipping 366 matching lines @@
 
   Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
   CHECK(display != NULL);
   XGrabServer(display);
   Window window = DefaultRootWindow(display);
   XRRScreenResources* screen = GetScreenResourcesAndRecordUMA(display, window);
   CHECK(screen != NULL);
 
   std::vector<OutputSnapshot> outputs = GetDualOutputs(display, screen);
   int new_output_count = outputs.size();
-  bool changed = false;
-  if (new_output_count != connected_output_count_) {
-    connected_output_count_ = new_output_count;
-    OutputState new_state =
-        GetNextState(display, screen, STATE_INVALID, outputs);
-    changed = EnterState(display, screen, window, new_state, outputs);
-    if (changed)
-      output_state_ = new_state;
-  }
+  // Don't skip even if the output counts didn't change because
+  // a display might have been swapped during the suspend.
+  connected_output_count_ = new_output_count;
+  OutputState new_state =
+      GetNextState(display, screen, STATE_INVALID, outputs);
+  // When a display was swapped, the state moves from
+  // STATE_DUAL_EXTENDED to STATE_DUAL_EXTENDED, so don't rely on
+  // the state chagne to tell if it was successful.
+  bool success = EnterState(display, screen, window, new_state, outputs);

[Jun Mukai, 2013/02/14 22:13:03]

Does this mean that the output configuration happens even though it's not
necessary?  like, recover from suspend but no display configuration has changed?
I suspect it will cause unnecessary display flickering or something like that in
that case, right?  Is there any way in early exit in EnterState?

[oshima, 2013/02/14 22:33:12]

Hmm, this wasn't the issue as there used to be no RRNotify_OutputChange event
for resume, but I now see the RRNotify_OutputChange on resume.

Stuart, is this something to do with your recent change?

[oshima, 2013/02/15 23:14:10]

Looks like this was temporary regression on TOT and it now get no OutputChange
upon resume. This shouldn't cause any extra work as RRNotify_OutputChange does
mean something has changed in outputs.

   bool is_projecting = IsProjecting(outputs);
   XRRFreeScreenResources(screen);
   XUngrabServer(display);
 
-  if (changed)
+  if (success) {
+    output_state_ = new_state;
     NotifyOnDisplayChanged();
+  }
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->
       SetIsProjecting(is_projecting);
 }
 
 void OutputConfigurator::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void OutputConfigurator::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
@@ skipping 413 matching lines @@
           return false;
         }
 
         int primary_height = primary_mode_info->height;
         int secondary_height = secondary_mode_info->height;
         CrtcConfig config1(primary_crtc, 0, 0, outputs[0].native_mode,
                            outputs[0].output);
         CrtcConfig config2(secondary_crtc, 0, 0, outputs[1].native_mode,
                            outputs[1].output);
 
-        if (new_state == STATE_DUAL_PRIMARY_ONLY)
+        if (new_state == STATE_DUAL_EXTENDED)
           config2.y = primary_height + kVerticalGap;
         else
           config1.y = secondary_height + kVerticalGap;
 
 
         int width =
             std::max<int>(primary_mode_info->width, secondary_mode_info->width);
         int height = primary_height + secondary_height + kVerticalGap;
 
         CreateFrameBuffer(display, screen, window, width, height, &config1,
@@ skipping 41 matching lines @@
       UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.single_duration", duration);
       break;
     case STATE_DUAL_MIRROR:
       if (mirror_mode_preserved_aspect_)
         UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.mirror_aspect_duration",
                                  duration);
       else
         UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.mirror_fallback_duration",
                                  duration);
       break;
-    case STATE_DUAL_PRIMARY_ONLY:
+    case STATE_DUAL_EXTENDED:
       UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.dual_primary_duration",

[Jun Mukai, 2013/02/14 22:13:03]

change it to "Display.EnterState.extended" or something?

[oshima, 2013/02/14 22:33:12]

I won't touch UMA name as we have historical data.

                                duration);
       break;
-    case STATE_DUAL_SECONDARY_ONLY:
-      UMA_HISTOGRAM_LONG_TIMES("Display.EnterState.dual_secondary_duration",
-                               duration);
     default:
       break;
   }
 
   mirror_mode_preserved_aspect_ = mirror_mode_will_preserve_aspect_;
   last_enter_state_time_ = base::TimeTicks::Now();
 }
 
 // static
 bool OutputConfigurator::IsInternalOutput(const XRROutputInfo* output_info) {
   return IsInternalOutputName(std::string(output_info->name));
 }
 
 // static
 RRMode OutputConfigurator::GetOutputNativeMode(
     const XRROutputInfo* output_info) {
   if (output_info->nmode <= 0)
     return None;
 
   return output_info->modes[0];
 }
 
 }  // namespace chromeos