Mir

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

Proposed by Alexandros Frantzis
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: 1543
Merged at revision: 1546
Proposed branch: lp:~afrantzis/mir/expose-display-buffer-only-for-power-mode-on
Merge into: lp:mir
Diff against target: 196 lines (+71/-4)
8 files modified
src/platform/graphics/android/android_display.cpp (+8/-1)
src/platform/graphics/android/android_display.h (+2/-0)
src/platform/graphics/mesa/display.cpp (+9/-1)
src/platform/graphics/overlapping_output_grouping.cpp (+1/-0)
tests/unit-tests/graphics/CMakeLists.txt (+1/-0)
tests/unit-tests/graphics/mesa/CMakeLists.txt (+0/-1)
tests/unit-tests/graphics/test_display.cpp (+23/-0)
tests/unit-tests/graphics/test_overlapping_output_grouping.cpp (+27/-1)
To merge this branch: bzr merge lp:~afrantzis/mir/expose-display-buffer-only-for-power-mode-on
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Kevin DuBois (community) Approve
Alberto Aguirre (community) Approve
Review via email: mp+214758@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

LGTM

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

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

review: Needs Fixing
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> 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.

Revision history for this message
Kevin DuBois (kdub) wrote :

> > 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
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

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

seems safe to retrigger

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platform/graphics/android/android_display.cpp'
2--- src/platform/graphics/android/android_display.cpp 2014-03-27 13:38:00 +0000
3+++ src/platform/graphics/android/android_display.cpp 2014-04-09 14:53:50 +0000
4@@ -49,11 +49,16 @@
5
6 void mga::AndroidDisplay::for_each_display_buffer(std::function<void(mg::DisplayBuffer&)> const& f)
7 {
8- f(*display_buffer);
9+ std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
10+
11+ if (display_buffer->configuration().power_mode == mir_power_mode_on)
12+ f(*display_buffer);
13 }
14
15 std::unique_ptr<mg::DisplayConfiguration> mga::AndroidDisplay::configuration() const
16 {
17+ std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
18+
19 return std::unique_ptr<mg::DisplayConfiguration>(
20 new mga::AndroidDisplayConfiguration(display_buffer->configuration()));
21 }
22@@ -66,6 +71,8 @@
23 std::logic_error("Invalid or inconsistent display configuration"));
24 }
25
26+ std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
27+
28 configuration.for_each_output([&](mg::DisplayConfigurationOutput const& output)
29 {
30 display_buffer->configure(output);
31
32=== modified file 'src/platform/graphics/android/android_display.h'
33--- src/platform/graphics/android/android_display.h 2014-03-27 13:38:00 +0000
34+++ src/platform/graphics/android/android_display.h 2014-04-09 14:53:50 +0000
35@@ -23,6 +23,7 @@
36 #include "gl_context.h"
37
38 #include <memory>
39+#include <mutex>
40
41 namespace mir
42 {
43@@ -68,6 +69,7 @@
44 private:
45 std::shared_ptr<DisplayBuilder> const display_builder;
46 GLContext gl_context;
47+ mutable std::mutex configuration_mutex;
48
49 //we only have a primary display at the moment
50 std::unique_ptr<ConfigurableDisplayBuffer> const display_buffer;
51
52=== modified file 'src/platform/graphics/mesa/display.cpp'
53--- src/platform/graphics/mesa/display.cpp 2014-03-27 09:52:04 +0000
54+++ src/platform/graphics/mesa/display.cpp 2014-04-09 14:53:50 +0000
55@@ -337,11 +337,19 @@
56 {
57 current_display_configuration.for_each_output([&](DisplayConfigurationOutput const& conf_output)
58 {
59- if (conf_output.connected && !conf_output.used)
60+ /*
61+ * An output may be unused either because it's explicitly not used
62+ * (DisplayConfigurationOutput::used) or because its power mode is
63+ * not mir_power_mode_on.
64+ */
65+ if (conf_output.connected &&
66+ (!conf_output.used || (conf_output.power_mode != mir_power_mode_on)))
67 {
68 uint32_t const connector_id = current_display_configuration.get_kms_connector_id(conf_output.id);
69 auto kms_output = output_container.get_kms_output_for(connector_id);
70+
71 kms_output->clear_crtc();
72+ kms_output->set_power_mode(conf_output.power_mode);
73 }
74 });
75 }
76
77=== modified file 'src/platform/graphics/overlapping_output_grouping.cpp'
78--- src/platform/graphics/overlapping_output_grouping.cpp 2014-03-10 03:02:32 +0000
79+++ src/platform/graphics/overlapping_output_grouping.cpp 2014-04-09 14:53:50 +0000
80@@ -76,6 +76,7 @@
81 conf.for_each_output([&](DisplayConfigurationOutput const& conf_output)
82 {
83 if (conf_output.connected && conf_output.used &&
84+ conf_output.power_mode == mir_power_mode_on &&
85 conf_output.current_mode_index < conf_output.modes.size())
86 {
87 add_output(conf_output);
88
89=== modified file 'tests/unit-tests/graphics/CMakeLists.txt'
90--- tests/unit-tests/graphics/CMakeLists.txt 2014-03-12 02:46:58 +0000
91+++ tests/unit-tests/graphics/CMakeLists.txt 2014-04-09 14:53:50 +0000
92@@ -8,6 +8,7 @@
93 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp
94 ${CMAKE_CURRENT_SOURCE_DIR}/test_pixel_format_utils.cpp
95 ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp
96+ ${CMAKE_CURRENT_SOURCE_DIR}/test_overlapping_output_grouping.cpp
97 )
98
99 add_subdirectory(nested/)
100
101=== modified file 'tests/unit-tests/graphics/mesa/CMakeLists.txt'
102--- tests/unit-tests/graphics/mesa/CMakeLists.txt 2014-03-06 06:05:17 +0000
103+++ tests/unit-tests/graphics/mesa/CMakeLists.txt 2014-04-09 14:53:50 +0000
104@@ -12,7 +12,6 @@
105 ${CMAKE_CURRENT_SOURCE_DIR}/test_linux_virtual_terminal.cpp
106 ${CMAKE_CURRENT_SOURCE_DIR}/test_internal_native_display.cpp
107 ${CMAKE_CURRENT_SOURCE_DIR}/test_internal_native_surface.cpp
108- ${CMAKE_CURRENT_SOURCE_DIR}/test_overlapping_output_grouping.cpp
109 ${CMAKE_CURRENT_SOURCE_DIR}/test_cursor.cpp
110 ${CMAKE_CURRENT_SOURCE_DIR}/test_native_platform.cpp
111 ${CMAKE_CURRENT_SOURCE_DIR}/test_anonymous_shm_file.cpp
112
113=== modified file 'tests/unit-tests/graphics/test_display.cpp'
114--- tests/unit-tests/graphics/test_display.cpp 2014-03-27 09:52:04 +0000
115+++ tests/unit-tests/graphics/test_display.cpp 2014-04-09 14:53:50 +0000
116@@ -199,3 +199,26 @@
117 EXPECT_CALL(mock_egl, eglMakeCurrent(_,EGL_NO_SURFACE,EGL_NO_SURFACE,EGL_NO_CONTEXT))
118 .Times(AtLeast(0));
119 }
120+
121+TEST_F(DisplayTest, does_not_expose_display_buffer_for_output_with_power_mode_off)
122+{
123+ using namespace testing;
124+ auto display = create_display();
125+ int db_count{0};
126+
127+ display->for_each_display_buffer([&] (mg::DisplayBuffer&) { ++db_count; });
128+ EXPECT_THAT(db_count, Eq(1));
129+
130+ auto conf = display->configuration();
131+ conf->for_each_output(
132+ [] (mg::UserDisplayConfigurationOutput& output)
133+ {
134+ output.power_mode = mir_power_mode_off;
135+ });
136+
137+ display->configure(*conf);
138+
139+ db_count = 0;
140+ display->for_each_display_buffer([&] (mg::DisplayBuffer&) { ++db_count; });
141+ EXPECT_THAT(db_count, Eq(0));
142+}
143
144=== renamed file 'tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp' => 'tests/unit-tests/graphics/test_overlapping_output_grouping.cpp'
145--- tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp 2014-03-26 05:48:59 +0000
146+++ tests/unit-tests/graphics/test_overlapping_output_grouping.cpp 2014-04-09 14:53:50 +0000
147@@ -37,10 +37,22 @@
148 public:
149 struct OutputInfo
150 {
151+ OutputInfo(
152+ geom::Rectangle const& rect,
153+ bool connected,
154+ bool used,
155+ MirOrientation orientation,
156+ MirPowerMode power_mode = mir_power_mode_on)
157+ : rect(rect), connected{connected}, used{used},
158+ orientation{orientation}, power_mode{power_mode}
159+ {
160+ }
161+
162 geom::Rectangle rect;
163 bool connected;
164 bool used;
165 MirOrientation orientation;
166+ MirPowerMode power_mode;
167 };
168
169 StubDisplayConfiguration(std::vector<OutputInfo> const& info)
170@@ -81,7 +93,7 @@
171 info.rect.top_left,
172 i - 1,
173 mir_pixel_format_invalid,
174- mir_power_mode_on,
175+ info.power_mode,
176 info.orientation
177 };
178
179@@ -291,3 +303,17 @@
180
181 check_groupings(info, expected_groups);
182 }
183+
184+TEST_F(OverlappingOutputGroupingTest, ignores_outputs_with_power_mode_not_on)
185+{
186+ std::vector<StubDisplayConfiguration::OutputInfo> info
187+ {
188+ {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_off},
189+ {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_standby},
190+ {{{0,0}, {100, 100}}, true, true, mir_orientation_normal, mir_power_mode_suspend}
191+ };
192+
193+ std::vector<geom::Rectangles> expected_groups;
194+
195+ check_groupings(info, expected_groups);
196+}

Subscribers

People subscribed via source and target branches