Support failing modeset

chromeos/display/output_configurator_unittest.cc

Index: chromeos/display/output_configurator_unittest.cc
diff --git a/chromeos/display/output_configurator_unittest.cc b/chromeos/display/output_configurator_unittest.cc
index 571df38f6cd4b3906e95f62bcbc506ea001d615b..526bb981d38e9edf7a3d26c0b0aeaa69b99442fd 100644
--- a/chromeos/display/output_configurator_unittest.cc
+++ b/chromeos/display/output_configurator_unittest.cc
@@ -102,7 +102,7 @@ class TestDelegate : public OutputConfigurator::Delegate {
   static const int kXRandREventBase = 10;
 
   TestDelegate()
-      : configure_crtc_result_(true),
+      : max_configurable_pixels_(0),
         hdcp_state_(HDCP_STATE_UNDESIRED) {}
   virtual ~TestDelegate() {}
 
@@ -114,8 +114,14 @@ class TestDelegate : public OutputConfigurator::Delegate {
     outputs_ = outputs;
   }
 
-  void set_configure_crtc_result(bool result) {
-    configure_crtc_result_ = result;
+  // max_configurable_pixels_ represents the maximum number of pixels that
+  // ConfigureCrtc will support.   Tests can use this to force ConfigureCrtc
+  // to fail if attempting to set a resolution that is higher than what
+  // a device might support under a given circumstance.
+  // A value of 0 means that no limit is enforced and ConfigureCrtc will
+  // return success regardless of the resolution.

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

nit: merge all of this documentation with the other comment that appears at the
point where the variable is defined (i.e. getters and setters usually don't have
any comments of their own)

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

Done.

+  void set_max_configurable_pixels(int pixels) {
+    max_configurable_pixels_ = pixels;
   }
 
   void set_hdcp_state(HDCPState state) { hdcp_state_ = state; }
@@ -154,6 +160,12 @@ class TestDelegate : public OutputConfigurator::Delegate {
   }
   virtual void AddOutputMode(RROutput output, RRMode mode) OVERRIDE {
     AppendAction(GetAddOutputModeAction(output, mode));
+    OutputConfigurator::OutputSnapshot* snapshot =
+        GetOutputFromId(output);
+    if (snapshot != NULL) {

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

nit:

  if (!snapshot)
    return;

(or maybe just CHECK(snapshot) instead? if OutputConfigurator tries to add a
mode to a bogus output, that seems like a reasonable thing to crash the test
about in a controlled way)

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

I did this and also did the same with mode_info (used CHECK(mode_info) but that
caused PanelFitting test to fail.  So, I realized that this code added in
AddOutputMode should not have been added to begin with.

+      snapshot->mode_infos[mode] =
+          *OutputConfigurator::GetModeInfo(*snapshot, mode);

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

probably also want to check GetModeInfo()'s return value here so that an invalid
mode won't make the test segfault:

  OutputConfigurator::OutputSnapshot::ModeInfo* mode_info =
      OutputConfigurator::GetModeInfo(*snapshot, mode);
  if (mode_info)
    snapshot->mode_infos[mode] = mode_info;

+    }
   }
   virtual bool ConfigureCrtc(RRCrtc crtc,
                              RRMode mode,
@@ -161,7 +173,22 @@ class TestDelegate : public OutputConfigurator::Delegate {
                              int x,
                              int y) OVERRIDE {
     AppendAction(GetCrtcAction(crtc, x, y, mode, output));
-    return configure_crtc_result_;
+
+    // |max_configurable_pixels_| value of 0 means no limit on resolution

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

nit: don't need this comment

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

Done.

+    if (max_configurable_pixels_ == 0) {

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

nit: don't need curly brackets for an 'if' with a single-line statement

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

Done.

+      return true;
+    }
+
+    OutputConfigurator::OutputSnapshot* snapshot = GetOutputFromId(output);
+    if (snapshot) {

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

nit:

  if (!snapshot)
    return false;

  const OutputConfigurator...

(return early to avoid longer indented blocks and to keep the "return false"
error-handling close to the error condition)

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

Done.

+      const OutputConfigurator::ModeInfo* mode_info =
+          OutputConfigurator::GetModeInfo(*snapshot, mode);
+
+      if (mode_info)
+        return mode_info->width * mode_info->height <= max_configurable_pixels_;
+    }
+
+    return false;
   }
   virtual void CreateFrameBuffer(
       int width,
@@ -212,6 +239,14 @@ class TestDelegate : public OutputConfigurator::Delegate {
     actions_ += action;
   }
 
+  OutputConfigurator::OutputSnapshot* GetOutputFromId(RROutput output_id) {
+    for (unsigned int i = 0; i < outputs_.size(); i++) {
+      if (outputs_[i].output == output_id)
+        return &outputs_[i];
+    }
+    return NULL;
+  }
+
   std::map<RRMode, ModeDetails> modes_;
 
   // Most-recently-configured transformation matrices, keyed by touch device ID.
@@ -222,8 +257,9 @@ class TestDelegate : public OutputConfigurator::Delegate {
 
   std::string actions_;
 
-  // Return value returned by ConfigureCrtc().
-  bool configure_crtc_result_;
+  // Represents the maximum number of pixels that a modeset can handle.   Any
+  // attempt to set a resolution with more pixels will fail.
+  int  max_configurable_pixels_;
 
   // Result value of GetHDCPState().
   HDCPState hdcp_state_;
@@ -1019,9 +1055,9 @@ TEST_F(OutputConfiguratorTest, AvoidUnnecessaryProbes) {
   EXPECT_FALSE(test_api_.TriggerConfigureTimeout());
   EXPECT_EQ(kNoActions, delegate_->GetActionsAndClear());
 
-  // Tell the delegate to report failure, which should result in the
-  // second output sticking with its native mode.
-  delegate_->set_configure_crtc_result(false);
+  // Lower the limit for which the delegate will succeed, which should result
+  // in the second output sticking with its native mode.
+  delegate_->set_max_configurable_pixels(1);
   UpdateOutputs(2, true);
   EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
                         GetFramebufferAction(kSmallModeWidth, kSmallModeHeight,
@@ -1033,9 +1069,9 @@ TEST_F(OutputConfiguratorTest, AvoidUnnecessaryProbes) {
                         kUngrab, kProjectingOn, NULL),
             delegate_->GetActionsAndClear());
 
-  // An change event reporting a mode change on the second output should
+  // A change event reporting a mode change on the second output should
   // trigger another reconfigure.
-  delegate_->set_configure_crtc_result(true);
+  delegate_->set_max_configurable_pixels(0);
   test_api_.SendOutputChangeEvent(
       outputs_[1].output, outputs_[1].crtc, outputs_[1].mirror_mode, true);
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
@@ -1263,4 +1299,116 @@ TEST_F(OutputConfiguratorTest, CTMForMultiScreens) {
   EXPECT_EQ(0, round((kDualWidth - 1) * ctm2.x_offset));
 }
 
+TEST_F(OutputConfiguratorTest, HandleConfigureCrtcFailure) {
+  int current_mode;

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

nit: move this down to the place where it's first used
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...)

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

Done.

+  InitWithSingleOutput();
+  test_api_.SendScreenChangeEvent();

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

i don't think you need this or the kUpdateXRandR check (the initial config gets
loaded by the call to Start() in InitWithSingleOutput(), and nothing's changed
since then)

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

That's fine.  I had modeled this after the AvoidUnnessecaryProbes which does the
same thing.  I have removed.

+  EXPECT_EQ(kUpdateXRandR, delegate_->GetActionsAndClear());
+
+  // kFirstMode represents the first mode in the list and
+  // also the mode that we are requesting the output_configurator
+  // to choose.  The test will be setup so that this mode will fail
+  // and it will have to choose the next best option.
+  const int kFirstMode = 11;
+
+  // Give the mode_info lists a few reasonable modes.
+  for (int i = 0; i < 2; i++) {

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

nit: s/2/outputs_.size()/

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

Here outputs_ is an array not a vector:

 OutputConfigurator::OutputSnapshot outputs_[2];

Do you want me to change it?

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

ah, i think that arraysize(outputs_) should work, in that case.

+    outputs_[i].mode_infos.clear();
+
+    current_mode = kFirstMode;
+    outputs_[i].mode_infos[current_mode++] = OutputConfigurator::ModeInfo(
+        2560, 1600, false, 60.0);
+    outputs_[i].mode_infos[current_mode++] = OutputConfigurator::ModeInfo(
+        1024, 768, false, 60.0);
+    outputs_[i].mode_infos[current_mode++] = OutputConfigurator::ModeInfo(
+        1280, 720, false, 60.0);
+    outputs_[i].mode_infos[current_mode++] = OutputConfigurator::ModeInfo(
+        1920, 1080, false, 60.0);
+    outputs_[i].mode_infos[current_mode++] = OutputConfigurator::ModeInfo(
+        1920, 1080, false, 40.0);
+
+    outputs_[i].selected_mode = kFirstMode;

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

s/selected_mode/current_mode/ ?

(selected_mode gets set by OutputConfigurator -- OutputSnapshots usually start
out with only current_mode and native_mode set; see
RealOutputConfiguratorDelegate::InitOutputSnapshot() in
chromeos/display/real_output_configurator_delegate.cc)

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

Done.

+    outputs_[i].native_mode = kFirstMode;
+  }
+
+  current_mode = kFirstMode;

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

this doesn't seem to do anything (i don't see it used later)

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

Done.

+  configurator_.Init(false);
+
+  // First test simply fails in STATE_SINGLE mode.   This is probably
+  // unrealistic but the want to make sure any assumptions don't
+  // creep in.
+  delegate_->set_max_configurable_pixels(
+      outputs_[0].mode_infos[kFirstMode + 2].width *
+      outputs_[0].mode_infos[kFirstMode + 2].height);
+  state_controller_.set_state(STATE_SINGLE);
+  test_api_.SendOutputChangeEvent(

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

i don't think you need this call either; the next call to UpdateOutputs() should
send an event (since its |send_events| argument is set)

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

I removed.  I had modeled it after AvoidUnnecessaryProbes

+      outputs_[0].output, outputs_[0].crtc, outputs_[0].selected_mode, true);
+  UpdateOutputs(1, true);
+
+  EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
+                        GetFramebufferAction(2560, 1600,
+                            outputs_[0].crtc, 0).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode + 3,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode + 2,
+                            outputs_[0].output).c_str(),
+                        kUngrab, kProjectingOff, NULL),
+            delegate_->GetActionsAndClear());
+
+  // This test should attempt to configure a mirror mode that will not succeed
+  // and should end up in extended mode.
+  delegate_->set_max_configurable_pixels(
+      outputs_[0].mode_infos[kFirstMode + 3].width *
+      outputs_[0].mode_infos[kFirstMode + 3].height);
+  state_controller_.set_state(STATE_DUAL_MIRROR);
+  test_api_.SendOutputChangeEvent(

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

same here

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

Done.

+      outputs_[1].output, outputs_[1].crtc, outputs_[1].selected_mode, true);
+  UpdateOutputs(2, true);
+
+  EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
+                        GetFramebufferAction(
+                            outputs_[0].mode_infos[kFirstMode].width,
+                            outputs_[0].mode_infos[kFirstMode].height,
+                            outputs_[0].crtc,
+                            outputs_[1].crtc).c_str(),
+                        GetCrtcAction(outputs_[0].crtc,
+                            0, 0, kFirstMode, outputs_[0].output).c_str(),
+                        // First mode tried is expected to fail and it will
+                        // retry wil the 4th mode in the list.
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode + 3,
+                            outputs_[0].output).c_str(),
+                        // Then attempt to configure crtc1 with the first mode.
+                        GetCrtcAction(outputs_[1].crtc, 0, 0, kFirstMode,
+                            outputs_[1].output).c_str(),
+                        GetCrtcAction(outputs_[1].crtc, 0, 0, kFirstMode + 3,
+                            outputs_[1].output).c_str(),
+                        // Since it was requested to go into mirror mode
+                        // and the configured modes were different, it
+                        // should now try and setup a valid configurable
+                        // extended mode.
+                        GetFramebufferAction(
+                            outputs_[0].mode_infos[kFirstMode].width,
+                            outputs_[0].mode_infos[kFirstMode].height +
+                              outputs_[1].mode_infos[kFirstMode].height +
+                              OutputConfigurator::kVerticalGap,
+                            outputs_[0].crtc, outputs_[1].crtc).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, kFirstMode + 3,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[1].crtc, 0,
+                            outputs_[1].mode_infos[kFirstMode].height +
+                              OutputConfigurator::kVerticalGap, kFirstMode,
+                            outputs_[1].output).c_str(),
+                        GetCrtcAction(outputs_[1].crtc, 0,
+                            outputs_[1].mode_infos[kFirstMode].height +
+                              OutputConfigurator::kVerticalGap, kFirstMode + 3,
+                            outputs_[1].output).c_str(),
+                        kUngrab, kProjectingOn, NULL),
+            delegate_->GetActionsAndClear());
+
+}
+
 }  // namespace chromeos