Merge lp:~vanvugt/mir/fix-1308941 into lp:mir

[Daniel van Vugt]

Ensure Mir remembers the current display mode even when the display is off.
So it's easy to toggle a monitor's power mode and it will stay in the
same mode. (LP: #1308941)

The one exception where current_mode_index is not restored is when the
definition of that display mode index has changed (e.g. new monitor plugged
in). When that happens, fall back to the preferred mode.

[Alexandros Frantzis (afrantzis)]

32 + output.current_mode_index = preferred_mode_index;

We are essentially misinforming the users about the current state of their displays. That being said, we are only doing so when a display is not used or is not in the mir_power_mode_on state (the only cases were there is no current mode set), so I don't think this will cause problems. It will a bit strange when we print out display information, though (e.g. with mirout).

I can't think of a another way to provide easy power on/off, or a particular failure scenario for this approach, so OK, but we should keep an eye out for unintended consequences.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1574
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1396/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1697
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1695
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1270
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1128
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1128/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1133
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1133/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1273
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1273/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1172
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6358

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1396/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

[Robert Carr (robertcarr)]

Hai! So desu. Arigato!

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1577
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1398/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1700
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1698
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1273
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1130
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1130/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1135
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1135/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1276
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1276/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1175
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6376

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1398/rebuild

review: Approve (continuous-integration)

=== modified file 'src/platform/graphics/mesa/real_kms_display_configuration.cpp'
--- src/platform/graphics/mesa/real_kms_display_configuration.cpp	2014-03-26 05:48:59 +0000
+++ src/platform/graphics/mesa/real_kms_display_configuration.cpp	2014-04-24 10:44:16 +0000
@@ -168,8 +168,9 @@
         kms_connector_type_to_output_type(connector.connector_type)};
     geom::Size physical_size{connector.mmWidth, connector.mmHeight};
     bool connected{connector.connection == DRM_MODE_CONNECTED};
-    size_t current_mode_index{std::numeric_limits<size_t>::max()};
-    size_t preferred_mode_index{std::numeric_limits<size_t>::max()};
+    size_t const invalid_mode_index = std::numeric_limits<size_t>::max();
+    size_t current_mode_index{invalid_mode_index};
+    size_t preferred_mode_index{invalid_mode_index};
     std::vector<DisplayConfigurationMode> modes;
     std::vector<MirPixelFormat> formats {mir_pixel_format_argb_8888,
                                          mir_pixel_format_xrgb_8888};
@@ -217,11 +218,26 @@
     {
         auto& output = *iter;
 
+        if (current_mode_index != invalid_mode_index)
+        {
+            output.current_mode_index = current_mode_index;
+        }
+        else if (!modes.empty() &&  // If empty retain old current_mode_index!
+                 ( output.current_mode_index >= modes.size() ||
+                   output.modes[output.current_mode_index] !=
+                          modes[output.current_mode_index]))
+        {
+            // current_mode_index is invalid and the definition of the old
+            // current mode has also changed (different display plugged in)
+            // so fall back to the preferred mode...
+            output.current_mode_index = preferred_mode_index;
+        }
+        // else output.current_mode_index is correct and unchanged.
+
         output.modes = modes;
         output.preferred_mode_index = preferred_mode_index;
         output.physical_size_mm = physical_size;
         output.connected = connected;
-        output.current_mode_index = current_mode_index;
         output.current_format = mir_pixel_format_xrgb_8888;
     }
 }

=== modified file 'tests/unit-tests/graphics/mesa/test_display_configuration.cpp'
--- tests/unit-tests/graphics/mesa/test_display_configuration.cpp	2014-03-27 09:46:09 +0000
+++ tests/unit-tests/graphics/mesa/test_display_configuration.cpp	2014-04-24 10:44:16 +0000
@@ -118,6 +118,13 @@
         /* Add the DisplayConfiguration modes corresponding to the DRM modes */
         for (auto const& mode : modes0)
             conf_modes0.push_back(conf_mode_from_drm_mode(mode));
+
+        modes1.push_back(fake::create_mode(123, 456, 138500, 2080, 1111, fake::NormalMode));
+        modes1.push_back(fake::create_mode(789, 1011, 148500, 2200, 1125, fake::NormalMode));
+        modes1.push_back(fake::create_mode(1213, 1415, 119000, 1840, 1080, fake::PreferredMode));
+
+        for (auto const& mode : modes1)
+            conf_modes1.push_back(conf_mode_from_drm_mode(mode));
     }
 
     ::testing::NiceMock<mtd::MockEGL> mock_egl;
@@ -127,6 +134,8 @@
 
     std::vector<drmModeModeInfo> modes0;
     std::vector<mg::DisplayConfigurationMode> conf_modes0;
+    std::vector<drmModeModeInfo> modes1;
+    std::vector<mg::DisplayConfigurationMode> conf_modes1;
     std::vector<drmModeModeInfo> modes_empty;
 
     mtf::UdevEnvironment fake_devices;
@@ -422,7 +431,7 @@
             false,
             true,
             geom::Point(),
-            std::numeric_limits<size_t>::max(),
+            1,  // Ensure current_mode_index is remembered even still
             mir_pixel_format_invalid,
             mir_power_mode_on,
             mir_orientation_normal
@@ -531,3 +540,101 @@
 
     EXPECT_EQ(expected_outputs_after.size(), output_count);
 }
+
+TEST_F(MesaDisplayConfigurationTest, new_monitor_defaults_to_preferred_mode)
+{
+    using namespace ::testing;
+
+    uint32_t const crtc_ids[1] = {10};
+    uint32_t const encoder_ids[2] = {20, 21};
+    uint32_t const connector_ids[1] = {30};
+    geom::Size const connector_physical_sizes_mm_before{480, 270};
+    geom::Size const connector_physical_sizes_mm_after{512, 642};
+    std::vector<uint32_t> possible_encoder_ids_empty;
+    uint32_t const possible_crtcs_mask_empty{0};
+    int const noutputs = 1;
+
+    mg::DisplayConfigurationOutput const expected_outputs_before[noutputs] =
+    {
+        {
+            mg::DisplayConfigurationOutputId(connector_ids[0]),
+            mg::DisplayConfigurationCardId{0},
+            mg::DisplayConfigurationOutputType::composite,
+            {},
+            conf_modes0,
+            1,
+            connector_physical_sizes_mm_before,
+            true,
+            true,
+            geom::Point(),
+            1,
+            mir_pixel_format_invalid,
+            mir_power_mode_on,
+            mir_orientation_normal
+        },
+    };
+
+    mg::DisplayConfigurationOutput const expected_outputs_after[noutputs] =
+    {
+        {
+            mg::DisplayConfigurationOutputId(connector_ids[0]),
+            mg::DisplayConfigurationCardId{0},
+            mg::DisplayConfigurationOutputType::composite,
+            {},
+            conf_modes1,
+            2,
+            connector_physical_sizes_mm_after,
+            true,
+            true,
+            geom::Point(),
+            2,  // current_mode_index changed to preferred as the list of
+                // available modes has also changed
+            mir_pixel_format_invalid,
+            mir_power_mode_on,
+            mir_orientation_normal
+        },
+    };
+
+    mtd::FakeDRMResources& resources(mock_drm.fake_drm);
+
+    resources.reset();
+    resources.add_crtc(crtc_ids[0], modes0[1]);
+    resources.add_encoder(encoder_ids[0], crtc_ids[0], possible_crtcs_mask_empty);
+    resources.add_connector(connector_ids[0], DRM_MODE_CONNECTOR_Composite,
+                            DRM_MODE_CONNECTED, encoder_ids[0],
+                            modes0, possible_encoder_ids_empty,
+                            connector_physical_sizes_mm_before);
+    resources.prepare();
+
+    auto display = create_display(create_platform());
+    auto conf = display->configuration();
+    int output_count = 0;
+    conf->for_each_output([&](mg::DisplayConfigurationOutput const& output)
+    {
+        ASSERT_LT(output_count, noutputs);
+        EXPECT_EQ(expected_outputs_before[output_count], output) << "output_count: " << output_count;
+        ++output_count;
+    });
+    EXPECT_EQ(noutputs, output_count);
+
+    // Now simulate a change of monitor with different capabilities where the
+    // old current_mode does not exist. Mir should choose the preferred mode.
+    resources.reset();
+    resources.add_crtc(crtc_ids[0], modes1[1]);
+    resources.add_encoder(encoder_ids[0], crtc_ids[0], possible_crtcs_mask_empty);
+    resources.add_connector(connector_ids[0], DRM_MODE_CONNECTOR_Composite,
+                            DRM_MODE_CONNECTED, encoder_ids[1],
+                            modes1, possible_encoder_ids_empty,
+                            connector_physical_sizes_mm_after);
+    resources.prepare();
+
+    conf = display->configuration();
+    output_count = 0;
+    conf->for_each_output([&](mg::DisplayConfigurationOutput const& output)
+    {
+        ASSERT_LT(output_count, noutputs);
+        EXPECT_EQ(expected_outputs_after[output_count], output) << "output_count: " << output_count;
+        ++output_count;
+    });
+    EXPECT_EQ(noutputs, output_count);
+}