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..bd2267d9d17afcb9b1c93a2f35111dafc339e8e6 100644
--- a/chromeos/display/output_configurator_unittest.cc
+++ b/chromeos/display/output_configurator_unittest.cc
@@ -100,9 +100,11 @@ std::string JoinActions(const char* action, ...) {
 class TestDelegate : public OutputConfigurator::Delegate {
  public:
   static const int kXRandREventBase = 10;
+  static const int kMaxTestWidth = 2560;
+  static const int kMaxTestHeight = 1600;
 
   TestDelegate()
-      : configure_crtc_result_(true),
+      : max_configurable_pixels_(kMaxTestWidth * kMaxTestHeight),

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

please make this default to 0 and document that 0 values will result in no limit
being enforced. get rid of the above kMaxTest* constants, and make tests that
want to trigger failures instead just pass their own reasonable values to
set_max_configurable_pixels().

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

Done.

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

Done.

         hdcp_state_(HDCP_STATE_UNDESIRED) {}
   virtual ~TestDelegate() {}
 
@@ -114,8 +116,8 @@ class TestDelegate : public OutputConfigurator::Delegate {
     outputs_ = outputs;
   }
 
-  void set_configure_crtc_result(bool result) {
-    configure_crtc_result_ = result;
+  void set_max_configurable_pixels(long pixels) {

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

why long? int seems like it should be big enough, but please use either int64 or
uint64 if you need a specific size.

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

Done.

+    max_configurable_pixels_ = pixels;
   }
 
   void set_hdcp_state(HDCPState state) { hdcp_state_ = state; }
@@ -154,6 +156,8 @@ class TestDelegate : public OutputConfigurator::Delegate {
   }
   virtual void AddOutputMode(RROutput output, RRMode mode) OVERRIDE {
     AppendAction(GetAddOutputModeAction(output, mode));
+    outputs_[output-1].mode_infos[mode] = OutputConfigurator::ModeInfo(

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

please don't add an undocumented assumption that output #n will be at index n-1.
those just happen to be defaults used by SetUp(). you need to iterate through
|outputs_| to find the correct output instead. there should probably be a
private method for this since ConfigureCrtc() needs it too:

  OutputSnapshot* GetOutput(RROutput output);

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

Done.

+        0, 0, false, 0.0);
   }
   virtual bool ConfigureCrtc(RRCrtc crtc,
                              RRMode mode,
@@ -161,7 +165,13 @@ class TestDelegate : public OutputConfigurator::Delegate {
                              int x,
                              int y) OVERRIDE {
     AppendAction(GetCrtcAction(crtc, x, y, mode, output));
-    return configure_crtc_result_;
+    if (mode == 0)
+      return true;
+
+    const OutputConfigurator::ModeInfo mode_info =
+        outputs_[output-1].mode_infos.find(mode)->second;
+
+    return mode_info.width * mode_info.height <= max_configurable_pixels_;
   }
   virtual void CreateFrameBuffer(
       int width,
@@ -222,8 +232,8 @@ class TestDelegate : public OutputConfigurator::Delegate {
 
   std::string actions_;
 
-  // Return value returned by ConfigureCrtc().
-  bool configure_crtc_result_;
+  // Simulate pixel-clk limitations

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

nit: avoid abbreviations ("pixel clock"?)

also describe what this actually does. is something like the following correct?

// Attempts to use a mode containing more pixels than this will fail.

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

Done.

+  long max_configurable_pixels_;
 
   // Result value of GetHDCPState().
   HDCPState hdcp_state_;
@@ -874,6 +884,9 @@ TEST_F(OutputConfiguratorTest, Headless) {
 
   // Connect an external display and check that it's configured correctly.
   outputs_[0] = outputs_[1];
+  // Some of the stub routines use the output field to know which output_
+  // it is dealing with
+  outputs_[0].output = 1;
   UpdateOutputs(1, true);
   EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
                         GetFramebufferAction(kBigModeWidth, kBigModeHeight,
@@ -1019,9 +1032,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(0);
   UpdateOutputs(2, true);
   EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
                         GetFramebufferAction(kSmallModeWidth, kSmallModeHeight,
@@ -1033,9 +1046,10 @@ 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(
+      TestDelegate::kMaxTestWidth * TestDelegate::kMaxTestHeight);
   test_api_.SendOutputChangeEvent(
       outputs_[1].output, outputs_[1].crtc, outputs_[1].mirror_mode, true);
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
@@ -1072,12 +1086,14 @@ TEST_F(OutputConfiguratorTest, PanelFitting) {
   // external display to support only the small mode.
   outputs_[0].current_mode = kBigModeId;
   outputs_[0].native_mode = kBigModeId;
+  outputs_[0].mirror_mode = kBigModeId;

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

OutputConfigurator is responsible for choosing the mirror mode and updating this
member accordingly -- if you need to manually assign it now to get an existing
test to pass, that seems like a sign that something is broken.

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

Thanks for the explanation.  This is indeed not necessary.  Might have been an
artifact of an early version where I did have something wrong.

   outputs_[0].mode_infos.clear();
   outputs_[0].mode_infos[kBigModeId] = OutputConfigurator::ModeInfo(
       kBigModeWidth, kBigModeHeight, false, 60.0);
 
   outputs_[1].current_mode = kSmallModeId;
   outputs_[1].native_mode = kSmallModeId;
+  outputs_[1].mirror_mode = kSmallModeId;
   outputs_[1].mode_infos.clear();
   outputs_[1].mode_infos[kSmallModeId] = OutputConfigurator::ModeInfo(
       kSmallModeWidth, kSmallModeHeight, false, 60.0);
@@ -1263,4 +1279,81 @@ TEST_F(OutputConfiguratorTest, CTMForMultiScreens) {
   EXPECT_EQ(0, round((kDualWidth - 1) * ctm2.x_offset));
 }
 
+TEST_F(OutputConfiguratorTest, HandleConfigureCrtcFailure) {
+  OutputConfigurator::OutputSnapshot *output;

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

nit: '*' goes on left side of space

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

Done.

+  OutputConfigurator::OutputSnapshot backup_output;
+  InitWithSingleOutput();
+  test_api_.SendScreenChangeEvent();
+  EXPECT_EQ(kUpdateXRandR, delegate_->GetActionsAndClear());
+

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

nit: delete extra blank line

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

Done.

+
+  // Save the state of output_[0] so that it can be restored
+  // when the test is done

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

nit: add trailing period

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

Done.

+  backup_output = outputs_[0];
+  outputs_[0].mode_infos.clear();
+
+  // Give the mode_info list a few reasonable modes
+  output = &outputs_[0];
+  output->mode_infos[11]=OutputConfigurator::ModeInfo(2560, 1600, false, 60.0);

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

there should be spaces on either side of the equals sign, even if it makes the
line wrap. it would probably be better to just add a helper method to the base
test class, though:

  RRMode CreateMode(OutputConfigurator::OutputSnapshot* output,
                    int width,
                    int height,
                    bool interlaced,
                    float refresh_rate);

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

Done.

+  output->mode_infos[12]=OutputConfigurator::ModeInfo(1920, 1080, false, 60.0);
+  output->mode_infos[13]=OutputConfigurator::ModeInfo(1920, 1080, false, 40.0);
+  output->mode_infos[14]=OutputConfigurator::ModeInfo(1024, 768, false, 60.0);
+
+  output = &outputs_[1];
+  output->mode_infos[11]=OutputConfigurator::ModeInfo(2560, 1600, false, 60.0);
+  output->mode_infos[12]=OutputConfigurator::ModeInfo(1920, 1080, false, 60.0);
+  output->mode_infos[13]=OutputConfigurator::ModeInfo(1920, 1080, false, 40.0);
+  output->mode_infos[14]=OutputConfigurator::ModeInfo(1024, 768, false, 60.0);
+
+  // This should cause ConfigureCrtc to fail once and get 1920x1080
+  outputs_[0].selected_mode = 11;
+  outputs_[0].mirror_mode = 11;
+  outputs_[1].selected_mode = 11;
+  delegate_->set_max_configurable_pixels(1920*1080);
+
+  configurator_.Init(false);

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

don't call Init() multiple times; the later call to UpdateOutputs() should be
sufficient. the multiple calls to UpdateOutputs() seem wrong too. the general
pattern should be along these lines:

  // update outputs_ as desired
  ...
  // copy the outputs to the delegator and notify the configurator
  UpdateOutputs(1, true);
  // now check that the configurator performed the correct actions
  EXPECT_EQ(..., delegate_->GetActionsAndClear());

doing it this way exercises the code in the same way that it's used in the real
world.

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

Thanks for the explanation.  I've tried to update it to model it how it should
be.  Let me know if there is still something wrong.

+  state_controller_.set_state(STATE_SINGLE);
+  UpdateOutputs(1, true);
+  EXPECT_NE(kNoActions, delegate_->GetActionsAndClear());
+  test_api_.SendOutputChangeEvent(
+      outputs_[0].output, outputs_[0].crtc, outputs_[0].selected_mode, true);

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

UpdateOutputs() should already do this

+  UpdateOutputs(1, true);
+
+  EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
+                        GetFramebufferAction(TestDelegate::kMaxTestWidth,
+                            TestDelegate::kMaxTestHeight,
+                            outputs_[0].crtc, 0).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, 11,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, 12,
+                            outputs_[0].output).c_str(),
+                        kUngrab, kProjectingOff, NULL),
+            delegate_->GetActionsAndClear());
+
+  configurator_.Init(false);

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

remove this too

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

Done.

+  state_controller_.set_state(STATE_DUAL_MIRROR);
+  UpdateOutputs(2, true);
+  EXPECT_NE(kNoActions, delegate_->GetActionsAndClear());
+  test_api_.SendOutputChangeEvent(
+      outputs_[1].output, outputs_[1].crtc, outputs_[1].selected_mode, true);
+  UpdateOutputs(2, true);
+
+  EXPECT_EQ(JoinActions(kUpdateXRandR, kGrab,
+                        GetFramebufferAction(TestDelegate::kMaxTestWidth,
+                            TestDelegate::kMaxTestHeight,
+                            outputs_[0].crtc, outputs_[1].crtc).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, 11,
+                            outputs_[0].output).c_str(),
+                        GetCrtcAction(outputs_[0].crtc, 0, 0, 12,
+                            outputs_[0].output).c_str(),
+                        kUngrab, kProjectingOn, NULL),
+            delegate_->GetActionsAndClear());
+
+
+  delegate_->set_max_configurable_pixels(
+      TestDelegate::kMaxTestWidth * TestDelegate::kMaxTestHeight);
+
+  outputs_[0] = backup_output;

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

you shouldn't need to restore anything at the end of a test; the test harness is
torn down and rebuilt for each test

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

Done.

+}
+
 }  // namespace chromeos