Support failing modeset

chromeos/display/output_configurator.cc

Index: chromeos/display/output_configurator.cc
diff --git a/chromeos/display/output_configurator.cc b/chromeos/display/output_configurator.cc
index 67da273b64965469c0b9d252e2e64e3c8ab16158..97577ad14f2a4af862a89fdfd9353d291ae52d64 100644
--- a/chromeos/display/output_configurator.cc
+++ b/chromeos/display/output_configurator.cc
@@ -812,6 +812,7 @@ bool OutputConfigurator::EnterState(
     OutputState output_state,
     DisplayPowerState power_state) {
   std::vector<bool> output_power;
+  bool status;

[Daniel Erat, 2014/01/08 21:40:28]

move this down to the point where you need it, and maybe rename it to be more
descriptive (i.e. do "bool all_succeeded = true" on line 945)

[dsodman, 2014/01/09 00:26:28]

Done.

   int num_on_outputs = GetOutputPower(
       cached_outputs_, power_state, &output_power);
   VLOG(1) << "EnterState: output=" << OutputStateToString(output_state)
@@ -941,28 +942,59 @@ bool OutputConfigurator::EnterState(
 
   // Finally, apply the desired changes.
   DCHECK_EQ(cached_outputs_.size(), updated_outputs.size());
+  status = true;
   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;
+
+        // Find the mode with the next-best resolution and see if that can
+        // be set.
+        int best_mode_pixels = 0;
+        const ModeInfo* mode_info = GetModeInfo(output, output.current_mode);
+        if (!mode_info)
+          break;
+
+        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++) {

[Daniel Erat, 2014/01/08 21:40:28]

nit: indent one more space

[dsodman, 2014/01/09 00:26:28]

Done.

+          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 = pixel_count;
+          }
+        }
+
+        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/08 21:40:28]

nit: s/modeset's/modesets/

[dsodman, 2014/01/09 00:26:28]

Done.

+      // then the two monitors will be mis-matched.  In this case, return
+      // false to let the observers be aware.
+      if (output_state == STATE_DUAL_MIRROR &&
+          output_power[i] &&
+          output.current_mode != output.mirror_mode) {
+        status = false;
       }
+
+      if (output.touch_device_id)
+        delegate_->ConfigureCTM(output.touch_device_id, output.transform);
+      cached_outputs_[i] = updated_outputs[i];

[Daniel Erat, 2014/01/08 21:40:28]

ConfigureCTM() should only be run and cached_outputs_[i] should only be updated
if the call to ConfigureCrtc() succeeded, right?

i'm wondering if you need something structured more like this (switching to
"while (true)" since it arguably simplifies the code a bit):

  bool configure_succeeded = false;
  while (true) {
    if (delegate_->ConfigureCrtc(...)) {
      configure_suceeded = true;
      break;
    }
    ...
    if (!mode_info)
      break;
    ...
    if (best_mode_pixels == 0)
      break;
  }

  if (configure_succeeded) {
    if (output.touch_device_id)
      delegate_->ConfigureCTM(...);
    cached_outputs_[i] = updated_outputs[i];
  } else {
    all_succeeded = false;
  }

[dsodman, 2014/01/09 00:26:28]

To be honest, I am not entirely sure what is the impact of calling
ConfigureCTM(...) when ConfigureCrtc has failed.  I will take your word for it
that it would lead to an inconsistency.  I modified as you suggested, but, it
causes a new failure in AvoidUnnecessaryProbes.   The reason is that prior to
this revision (and prior to my change altogether), EnterState returns true when
ConfigureCrtc fails.  With this change, it is returning false and
EnterStateOrFallbackToSoftwareMirroring is calling EnterState again with
EXTENDED mode.   It seems to me that indicates that the expected output of the
AvoidUnnessecaryProbes needs to change, so, I updated it, but, you should tell
me if that is correct or if this has introduced a bug.

[Daniel Erat, 2014/01/09 00:40:34]

i'm not an expert on the touch code, but i believe that this matrix transforms
touch events ("Coordinate Transformation Matrix"; see
RealOutputConfiguratorDelegate::ConfigureCTM()), so it ought to match whatever
mode is being used for the output. maybe sadrul@ can confirm...

     }
   }
 
   output_state_ = output_state;
   power_state_ = power_state;
-  return true;
+  return status;

[Daniel Erat, 2014/01/08 21:40:28]

i'm also not sure whether output_state_ and power_state_ should be updated
unconditionally here, or only if all of the configures were successful. whether
they're left unchanged or updated determines whether a later call setting the
same output state or power state will run or be ignored, i believe, so it might
be better to leave them unchanged if that means that we'll at least try to
reconfigure everything again later.

[dsodman, 2014/01/09 00:26:28]

Done.

 }
 
 OutputState OutputConfigurator::ChooseOutputState(