Support failing modeset

chromeos/display/output_configurator_unittest.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 <cstdarg>
 #include <map>
 #include <string>
@@ skipping 82 matching lines @@
     actions += action;
     action = va_arg(arg_list, const char*);
   }
   va_end(arg_list);
   return actions;
 }
 
 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() {}
 
   const std::vector<OutputConfigurator::OutputSnapshot>& outputs() const {
     return outputs_;
   }
   void set_outputs(
       const std::vector<OutputConfigurator::OutputSnapshot>& outputs) {
     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; }
 
   // Returns a comma-separated string describing the actions that were
   // requested since the previous call to GetActionsAndClear() (i.e.
   // results are non-repeatable).
   std::string GetActionsAndClear() {
     std::string actions = actions_;
     actions_.clear();
@@ skipping 18 matching lines @@
   virtual void SetBackgroundColor(uint32 color_argb) OVERRIDE {
     AppendAction(GetBackgroundAction(color_argb));
   }
   virtual void ForceDPMSOn() OVERRIDE { AppendAction(kForceDPMS); }
   virtual std::vector<OutputConfigurator::OutputSnapshot> GetOutputs()
       OVERRIDE {
     return outputs_;
   }
   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,
                              RROutput output,
                              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,
       int height,
       const std::vector<OutputConfigurator::OutputSnapshot>& outputs) OVERRIDE {
     AppendAction(
         GetFramebufferAction(width,
                              height,
                              outputs.size() >= 1 ? outputs[0].crtc : 0,
                              outputs.size() >= 2 ? outputs[1].crtc : 0));
@@ skipping 40 matching lines @@
   std::map<RRMode, ModeDetails> modes_;
 
   // Most-recently-configured transformation matrices, keyed by touch device ID.
   std::map<int, OutputConfigurator::CoordinateTransformation> ctms_;
 
   // Outputs to be returned by GetOutputs().
   std::vector<OutputConfigurator::OutputSnapshot> outputs_;
 
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
 class TestObserver : public OutputConfigurator::Observer {
  public:
   explicit TestObserver(OutputConfigurator* configurator)
@@ skipping 630 matching lines @@
   configurator_.SetDisplayPower(DISPLAY_POWER_ALL_OFF,
                                 OutputConfigurator::kSetDisplayPowerNoFlags);
   EXPECT_EQ(JoinActions(kGrab, kUngrab, NULL), delegate_->GetActionsAndClear());
   configurator_.SetDisplayPower(DISPLAY_POWER_ALL_ON,
                                 OutputConfigurator::kSetDisplayPowerNoFlags);
   EXPECT_EQ(JoinActions(kGrab, kForceDPMS, kUngrab, NULL),
             delegate_->GetActionsAndClear());
 
   // 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,
                             outputs_[0].crtc, 0).c_str(),
                         GetCrtcAction(outputs_[0].crtc, 0, 0, kBigModeId,
                             outputs_[0].output).c_str(),
                         kUngrab, kProjectingOff, NULL),
             delegate_->GetActionsAndClear());
 }
 
@@ skipping 125 matching lines @@
                         kUngrab, kProjectingOff, NULL),
             delegate_->GetActionsAndClear());
 
   // An additional event about the second output being disconnected should
   // be ignored.
   test_api_.SendOutputChangeEvent(
       outputs_[1].output, outputs_[1].crtc, outputs_[1].current_mode, false);
   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,
                             outputs_[0].crtc, outputs_[1].crtc).c_str(),
                         GetCrtcAction(outputs_[0].crtc, 0, 0, kSmallModeId,
                             outputs_[0].output).c_str(),
                         GetCrtcAction(outputs_[1].crtc, 0, 0, kSmallModeId,
                             outputs_[1].output).c_str(),
                         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());
   EXPECT_EQ(JoinActions(kGrab,
                         GetFramebufferAction(kSmallModeWidth, kSmallModeHeight,
                             outputs_[0].crtc, outputs_[1].crtc).c_str(),
                         GetCrtcAction(outputs_[0].crtc, 0, 0, kSmallModeId,
                             outputs_[0].output).c_str(),
                         GetCrtcAction(outputs_[1].crtc, 0, 0, kSmallModeId,
                             outputs_[1].output).c_str(),
@@ skipping 16 matching lines @@
   ASSERT_EQ(static_cast<size_t>(2), cached->size());
   EXPECT_EQ(outputs_[0].current_mode, (*cached)[0].current_mode);
   EXPECT_EQ(outputs_[1].current_mode, (*cached)[1].current_mode);
 }
 
 TEST_F(OutputConfiguratorTest, PanelFitting) {
   // Configure the internal display to support only the big mode and the
   // 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);
 
   // The small mode should be added to the internal output when requesting
   // mirrored mode.
   UpdateOutputs(2, false);
   state_controller_.set_state(STATE_DUAL_MIRROR);
   configurator_.Init(true /* is_panel_fitting_enabled */);
   configurator_.Start(0);
@@ skipping 165 matching lines @@
   EXPECT_EQ(kSmallModeHeight + OutputConfigurator::kVerticalGap,
             round((kDualHeight - 1) * ctm2.y_offset));
 
   EXPECT_EQ(kSmallModeWidth - 1, round((kDualWidth - 1) * ctm1.x_scale));
   EXPECT_EQ(0, round((kDualWidth - 1) * ctm1.x_offset));
 
   EXPECT_EQ(kBigModeWidth - 1, round((kDualWidth - 1) * ctm2.x_scale));
   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