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 84 matching lines @@
   }
   va_end(arg_list);
   return actions;
 }
 
 class TestDelegate : public OutputConfigurator::Delegate {
  public:
   static const int kXRandREventBase = 10;
 
   TestDelegate()
-      : configure_crtc_result_(true),
+      : max_configurable_pixels_(0),
         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;
+  // 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; }
 
   // 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));
+    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,
                              RROutput output,
                              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,
       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 30 matching lines @@
     int height;
     bool interlaced;
   };
 
   void AppendAction(const std::string& action) {
     if (!actions_.empty())
       actions_ += ",";
     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.
   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_;
+  // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
 class TestObserver : public OutputConfigurator::Observer {
  public:
   explicit TestObserver(OutputConfigurator* configurator)
@@ skipping 775 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(1);
   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(0);
   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 207 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) {
+  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