Merge lp:~afrantzis/mir/expose-display-buffer-only-for-power-mode-on into lp:mir

[Alexandros Frantzis]

mesa,android: Expose display buffers only for outputs with mir_power_mode_on

mesa,android: Expose display buffers only for outputs with mir_power_mode_on

Exposing display buffers for inactive outputs is not only wasteful but can cause other problems/crashes depending on the platform.

This is part of the series to avoid blocking swap buffers for surfaces that are not rendered by the compositor.

Other MPs in the non-blocking eglSwapBuffers() series:
https://code.launchpad.net/~afrantzis/mir/non-blocking-swap-buffers/+merge/214755/+merge/214758
https://code.launchpad.net/~afrantzis/unity-system-compositor/non-blocking-swap-buffers/+merge/214759

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1542
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1275/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1530
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1528
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1007
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1007/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1012
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1012/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1105
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1105/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1022
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5753

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

[Kevin DuBois (kdub)]

might be pre-existing, but I think android needs some locking around the power_mode configuring and skipping the DisplayBuffer.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> might be pre-existing, but I think android needs some locking around the power_mode configuring and skipping the DisplayBuffer.

The problem is that we will be calling a callback with a lock held. We are doing the same in mesa/display.cpp though, so we might as well make the two backends equally susceptible to deadlock. Of course we could try to fix them both somehow, but I don't see any reasonable solution other than creating copies of the DisplayBuffers, which has its own set of problems.

[Kevin DuBois (kdub)]

> > might be pre-existing, but I think android needs some locking around the
> power_mode configuring and skipping the DisplayBuffer.
>
> The problem is that we will be calling a callback with a lock held. We are
> doing the same in mesa/display.cpp though, so we might as well make the two
> backends equally susceptible to deadlock. Of course we could try to fix them
> both somehow, but I don't see any reasonable solution other than creating
> copies of the DisplayBuffers, which has its own set of problems.

okay, looks good to me

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1543
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1292/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1551
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1549
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1024
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1024/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1029
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1029/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1125
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1125/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1042
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5865

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/552/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1561
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1559
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1134/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/457
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/457/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/458
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/458/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1135
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1135/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1051/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5920

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

Install these packages without verification? [y/N] Build timed out (after 60 minutes). Marking the build as failed.

seems safe to retrigger

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/554/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1565
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1563
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1138/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/459
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/459/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/460
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/460/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1139
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1139/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1055/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5934

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/platform/graphics/android/android_display.cpp'
--- src/platform/graphics/android/android_display.cpp	2014-03-27 13:38:00 +0000
+++ src/platform/graphics/android/android_display.cpp	2014-04-09 14:53:50 +0000
@@ -49,11 +49,16 @@
 
 void mga::AndroidDisplay::for_each_display_buffer(std::function<void(mg::DisplayBuffer&)> const& f)
 {
-    f(*display_buffer);
+    std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
+
+    if (display_buffer->configuration().power_mode == mir_power_mode_on)
+        f(*display_buffer);
 }
 
 std::unique_ptr<mg::DisplayConfiguration> mga::AndroidDisplay::configuration() const
 {
+    std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
+
     return std::unique_ptr<mg::DisplayConfiguration>(
         new mga::AndroidDisplayConfiguration(display_buffer->configuration()));
 }
@@ -66,6 +71,8 @@
             std::logic_error("Invalid or inconsistent display configuration"));
     }
 
+    std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
+
     configuration.for_each_output([&](mg::DisplayConfigurationOutput const& output)
     {
         display_buffer->configure(output);

=== modified file 'src/platform/graphics/android/android_display.h'
--- src/platform/graphics/android/android_display.h	2014-03-27 13:38:00 +0000
+++ src/platform/graphics/android/android_display.h	2014-04-09 14:53:50 +0000
@@ -23,6 +23,7 @@
 #include "gl_context.h"
 
 #include <memory>
+#include <mutex>
 
 namespace mir
 {
@@ -68,6 +69,7 @@
 private:
     std::shared_ptr<DisplayBuilder> const display_builder;
     GLContext gl_context;
+    mutable std::mutex configuration_mutex;
 
     //we only have a primary display at the moment
     std::unique_ptr<ConfigurableDisplayBuffer> const display_buffer;

=== modified file 'src/platform/graphics/mesa/display.cpp'
--- src/platform/graphics/mesa/display.cpp	2014-03-27 09:52:04 +0000
+++ src/platform/graphics/mesa/display.cpp	2014-04-09 14:53:50 +0000
@@ -337,11 +337,19 @@
 {
     current_display_configuration.for_each_output([&](DisplayConfigurationOutput const& conf_output)
     {
-        if (conf_output.connected && !conf_output.used)
+        /*
+         * An output may be unused either because it's explicitly not used
+         * (DisplayConfigurationOutput::used) or because its power mode is
+         * not mir_power_mode_on.
+         */
+        if (conf_output.connected &&
+            (!conf_output.used || (conf_output.power_mode != mir_power_mode_on)))
         {
             uint32_t const connector_id = current_display_configuration.get_kms_connector_id(conf_output.id);
             auto kms_output = output_container.get_kms_output_for(connector_id);
+
             kms_output->clear_crtc();
+            kms_output->set_power_mode(conf_output.power_mode);
         }
     });
 }

=== modified file 'src/platform/graphics/overlapping_output_grouping.cpp'
--- src/platform/graphics/overlapping_output_grouping.cpp	2014-03-10 03:02:32 +0000
+++ src/platform/graphics/overlapping_output_grouping.cpp	2014-04-09 14:53:50 +0000
@@ -76,6 +76,7 @@
     conf.for_each_output([&](DisplayConfigurationOutput const& conf_output)
     {
         if (conf_output.connected && conf_output.used &&
+            conf_output.power_mode == mir_power_mode_on &&
             conf_output.current_mode_index < conf_output.modes.size())
         {
             add_output(conf_output);

=== modified file 'tests/unit-tests/graphics/CMakeLists.txt'
--- tests/unit-tests/graphics/CMakeLists.txt	2014-03-12 02:46:58 +0000
+++ tests/unit-tests/graphics/CMakeLists.txt	2014-04-09 14:53:50 +0000
@@ -8,6 +8,7 @@
     ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test_pixel_format_utils.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/test_overlapping_output_grouping.cpp
 )
 
 add_subdirectory(nested/)

=== modified file 'tests/unit-tests/graphics/mesa/CMakeLists.txt'
--- tests/unit-tests/graphics/mesa/CMakeLists.txt	2014-03-06 06:05:17 +0000
+++ tests/unit-tests/graphics/mesa/CMakeLists.txt	2014-04-09 14:53:50 +0000
@@ -12,7 +12,6 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_linux_virtual_terminal.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_internal_native_display.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_internal_native_surface.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_overlapping_output_grouping.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_cursor.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_native_platform.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_anonymous_shm_file.cpp

=== modified file 'tests/unit-tests/graphics/test_display.cpp'
--- tests/unit-tests/graphics/test_display.cpp	2014-03-27 09:52:04 +0000
+++ tests/unit-tests/graphics/test_display.cpp	2014-04-09 14:53:50 +0000
@@ -199,3 +199,26 @@
     EXPECT_CALL(mock_egl, eglMakeCurrent(_,EGL_NO_SURFACE,EGL_NO_SURFACE,EGL_NO_CONTEXT))
         .Times(AtLeast(0));
 }
+
+TEST_F(DisplayTest, does_not_expose_display_buffer_for_output_with_power_mode_off)
+{
+    using namespace testing;
+    auto display = create_display();
+    int db_count{0};
+
+    display->for_each_display_buffer([&] (mg::DisplayBuffer&) { ++db_count; });
+    EXPECT_THAT(db_count, Eq(1));
+
+    auto conf = display->configuration();
+    conf->for_each_output(
+        [] (mg::UserDisplayConfigurationOutput& output)
+        {
+            output.power_mode = mir_power_mode_off;
+        });
+
+    display->configure(*conf);
+
+    db_count = 0;
+    display->for_each_display_buffer([&] (mg::DisplayBuffer&) { ++db_count; });
+    EXPECT_THAT(db_count, Eq(0));
+}

=== renamed file 'tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp' => 'tests/unit-tests/graphics/test_overlapping_output_grouping.cpp'
--- tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp	2014-03-26 05:48:59 +0000
+++ tests/unit-tests/graphics/test_overlapping_output_grouping.cpp	2014-04-09 14:53:50 +0000
@@ -37,10 +37,22 @@
 public:
     struct OutputInfo
     {
+        OutputInfo(
+            geom::Rectangle const& rect,
+            bool connected,
+            bool used,
+            MirOrientation orientation,
+            MirPowerMode power_mode = mir_power_mode_on)
+            : rect(rect), connected{connected}, used{used},
+              orientation{orientation}, power_mode{power_mode}
+        {
+        }
+
         geom::Rectangle rect;
         bool connected;
         bool used;
         MirOrientation orientation;
+        MirPowerMode power_mode;
     };
 
     StubDisplayConfiguration(std::vector<OutputInfo> const& info)
@@ -81,7 +93,7 @@
                 info.rect.top_left,
                 i - 1,
                 mir_pixel_format_invalid,
-                mir_power_mode_on,
+                info.power_mode,
                 info.orientation
             };
 
@@ -291,3 +303,17 @@
 
     check_groupings(info, expected_groups);
 }
+
+TEST_F(OverlappingOutputGroupingTest, ignores_outputs_with_power_mode_not_on)
+{
+    std::vector<StubDisplayConfiguration::OutputInfo> info
+    {
+        {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_off},
+        {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_standby},
+        {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_suspend}
+    };
+
+    std::vector<geom::Rectangles> expected_groups;
+
+    check_groupings(info, expected_groups);
+}