Support failing modeset

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 <X11/Xlib.h>
 #include <X11/extensions/Xrandr.h>
 #include <X11/extensions/XInput2.h>
 
@@ skipping 927 matching lines @@
       break;
     }
   }
 
   // Finally, apply the desired changes.
   DCHECK_EQ(cached_outputs_.size(), updated_outputs.size());
   if (!updated_outputs.empty()) {
     delegate_->CreateFrameBuffer(width, height, updated_outputs);
     for (size_t i = 0; i < updated_outputs.size(); ++i) {
       const OutputSnapshot& output = updated_outputs[i];
-      if (delegate_->ConfigureCrtc(output.crtc, output.current_mode,
-                                   output.output, output.x, output.y)) {
-        if (output.touch_device_id)
-          delegate_->ConfigureCTM(output.touch_device_id, output.transform);
-        cached_outputs_[i] = updated_outputs[i];
-      } else {
+      while (!delegate_->ConfigureCrtc(output.crtc, output.current_mode,
+                                       output.output, output.x, output.y)) {
         LOG(WARNING) << "Unable to configure CRTC " << output.crtc << ":"
                      << " mode=" << output.current_mode
                      << " output=" << output.output
                      << " x=" << output.x
                      << " y=" << output.y;
+
+        int best_mode_pixels = 0;

[Daniel Erat, 2014/01/06 17:15:29]

nit: add a comment before this line summarizing what the loop does, like:

  // Find the mode with the next-highest resolution.

[dsodman, 2014/01/08 20:55:36]

Done.

+        const ModeInfo mode_info =
+            output.mode_infos.find(output.current_mode)->second;

[Daniel Erat, 2014/01/06 17:15:29]

nit: do this instead?

  const ModeInfo* mode_info = GetModeInfo(output.current_mode);
  if (!mode_info)
    break;

[dsodman, 2014/01/08 20:55:36]

Done.

+        int current_mode_pixels = mode_info.width * mode_info.height;
+        for (ModeInfoMap::const_iterator it = output.mode_infos.begin();
+            it != output.mode_infos.end(); it++) {
+          int pixel_count = it->second.width * it->second.height;
+          if ((pixel_count < current_mode_pixels) &&
+              (pixel_count > best_mode_pixels)) {
+            updated_outputs[i].current_mode = it->first;
+            best_mode_pixels = current_mode_pixels;

[Daniel Erat, 2014/01/06 17:15:29]

shouldn't this be:

  best_mode_pixels = pixel_count;

? if that's correct, please make sure you have a test that would've caught this.

[dsodman, 2014/01/08 20:55:36]

Thanks for catching this.  You are right.  I also updated tests to make sure
that the test would have caught it.

+          }
+        }
+
+        if (best_mode_pixels == 0)
+          break;
       }
+
+      // if we are trying to set mirror mode and one of the modeset's fails,

[Daniel Erat, 2014/01/06 17:15:29]

nit: s/if/If/

[dsodman, 2014/01/08 20:55:36]

Done.

+      // then the two monitors will be mis-matched.  In this case, return
+      // false to let the observer's be aware

[Daniel Erat, 2014/01/06 17:15:29]

nit: s/observer's/observers/, also add trailing period

[dsodman, 2014/01/08 20:55:36]

Done.

+      if (output_state == STATE_DUAL_MIRROR)

[Daniel Erat, 2014/01/06 17:15:29]

style guide requires curly brackets on the outermost if statement here (since
its statement is more than a single line), but it would be better to just do:

  if (output_state == STATE_DUAL_MIRROR &&
      output_power[i] &&
      output.current_mode != output.mirror_mode)
    return false;

[dsodman, 2014/01/08 20:55:36]

Done.

+        if (output_power[i] && output.current_mode != output.mirror_mode)
+          return false;

[Daniel Erat, 2014/01/06 17:15:29]

shouldn't you only return after you've finished iterating through all of the
outputs? please make sure you have a test for the case where configuring the
first output fails to make sure that the second output still gets configured.

[dsodman, 2014/01/08 20:55:36]

Done.

+
+      if (output.touch_device_id)
+        delegate_->ConfigureCTM(output.touch_device_id, output.transform);

[Daniel Erat, 2014/01/06 17:15:29]

shouldn't this come before the previous return statement? otherwise it seems
like you could end up with touch device settings that don't match the current
mode. please add a test for this too.

(this comment is perhaps irrelevant if the previous return statement gets moved
outside of the loop as suggested above.)

+      cached_outputs_[i] = updated_outputs[i];

[Daniel Erat, 2014/01/06 17:15:29]

i think that this also needs to happen before the previous return statement;
otherwise the cached state won't match reality.

[dsodman, 2014/01/08 20:55:36]

Done.

     }
   }
 
   output_state_ = output_state;
   power_state_ = power_state;
   return true;
 }
 
 OutputState OutputConfigurator::ChooseOutputState(
     DisplayPowerState power_state) const {
@@ skipping 115 matching lines @@
   float width_ratio = static_cast<float>(mirror_mode_info->width) /
       static_cast<float>(native_mode_info->width);
   float height_ratio = static_cast<float>(mirror_mode_info->height) /
       static_cast<float>(native_mode_info->height);
 
   area_ratio = width_ratio * height_ratio;
   return area_ratio;
 }
 
 }  // namespace chromeos