Mir

Merge lp:~albaguirre/mir/fix-1549359 into lp:mir

  1. fix-1549359
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 3335
Proposed branch: lp:~albaguirre/mir/fix-1549359
Merge into: lp:mir
Diff against target: 233 lines (+97/-9)
3 files modified
src/platforms/mesa/server/kms/display.cpp (+9/-3)
src/platforms/mesa/server/kms/display.h (+2/-1)
tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp (+86/-5)
To merge this branch: bzr merge lp:~albaguirre/mir/fix-1549359
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve
Cemil Azizoglu (community) Approve
Alan Griffiths Approve
Review via email: mp+287057@code.launchpad.net

Commit message

mesa-kms: display configuration - only query drm when a change is notified

Querying drm display configuration can be expensive - only query when necessary

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3333
https://mir-jenkins.ubuntu.com/job/mir-ci/389/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/182/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/196
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/192
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/192
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/187/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/187/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/187/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/187/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/389/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

^-- Failure looks like lp:1523621

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Looks like a sensible way to reduce the impact of the underlying problem. But we really ought to avoid any chance of doing significant work (like probing display hardware) on the IPC thread.

review: Approve
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

LGTM

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3333
https://mir-jenkins.ubuntu.com/job/mir-ci/390/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/183
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/199
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/188/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/188/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/188/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/188/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/390/rebuild

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/platforms/mesa/server/kms/display.cpp'
2--- src/platforms/mesa/server/kms/display.cpp 2016-02-01 22:53:06 +0000
3+++ src/platforms/mesa/server/kms/display.cpp 2016-02-24 16:29:41 +0000
4@@ -96,6 +96,7 @@
5 output_container{drm->fd,
6 std::make_shared<KMSPageFlipper>(drm->fd, listener)},
7 current_display_configuration{drm->fd},
8+ dirty_configuration{false},
9 bypass_option(bypass_option),
10 gl_config{gl_config}
11 {
12@@ -133,8 +134,12 @@
13 {
14 std::lock_guard<std::mutex> lg{configuration_mutex};
15
16- /* Give back a copy of the latest configuration information */
17- current_display_configuration.update();
18+ if (dirty_configuration)
19+ {
20+ /* Give back a copy of the latest configuration information */
21+ current_display_configuration.update();
22+ dirty_configuration = false;
23+ }
24 return std::unique_ptr<mg::DisplayConfiguration>(
25 new mgm::RealKMSDisplayConfiguration(current_display_configuration)
26 );
27@@ -268,9 +273,10 @@
28 make_module_ptr<std::function<void(int)>>(
29 [conf_change_handler, this](int)
30 {
31- monitor.process_events([conf_change_handler]
32+ monitor.process_events([conf_change_handler, this]
33 (mir::udev::Monitor::EventType, mir::udev::Device const&)
34 {
35+ dirty_configuration = true;
36 conf_change_handler();
37 });
38 }));
39
40=== modified file 'src/platforms/mesa/server/kms/display.h'
41--- src/platforms/mesa/server/kms/display.h 2016-02-01 22:53:06 +0000
42+++ src/platforms/mesa/server/kms/display.h 2016-02-24 16:29:41 +0000
43@@ -25,8 +25,8 @@
44 #include "display_helpers.h"
45 #include "platform_common.h"
46
47+#include <atomic>
48 #include <mutex>
49-
50 #include <vector>
51
52 namespace mir
53@@ -106,6 +106,7 @@
54 std::vector<std::unique_ptr<DisplayBuffer>> display_buffers;
55 RealKMSOutputContainer output_container;
56 mutable RealKMSDisplayConfiguration current_display_configuration;
57+ mutable std::atomic<bool> dirty_configuration;
58
59 BypassOption bypass_option;
60 std::weak_ptr<Cursor> cursor;
61
62=== modified file 'tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp'
63--- tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp 2016-01-29 08:18:22 +0000
64+++ tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp 2016-02-24 16:29:41 +0000
65@@ -20,22 +20,26 @@
66 #include "mir/graphics/display_configuration.h"
67 #include "mir/graphics/display.h"
68 #include "mir/graphics//default_display_configuration_policy.h"
69+#include "mir/time/steady_clock.h"
70+#include "mir/glib_main_loop.h"
71 #include "src/platforms/mesa/server/kms/platform.h"
72 #include "src/platforms/mesa/server/kms/kms_display_configuration.h"
73-#include "mir/test/doubles/null_emergency_cleanup.h"
74 #include "src/server/report/null_report_factory.h"
75-#include "mir/test/doubles/null_virtual_terminal.h"
76+
77+#include "mir/test/signal.h"
78+#include "mir/test/auto_unblock_thread.h"
79
80 #include "mir/test/doubles/mock_egl.h"
81 #include "mir/test/doubles/mock_gl.h"
82+#include "mir/test/doubles/mock_drm.h"
83+#include "mir/test/doubles/mock_gbm.h"
84+#include "mir/test/doubles/null_emergency_cleanup.h"
85+#include "mir/test/doubles/null_virtual_terminal.h"
86 #include "mir/test/doubles/stub_gl_config.h"
87 #include "mir/test/doubles/stub_gl_program_factory.h"
88
89 #include "mir_test_framework/udev_environment.h"
90
91-#include "mir/test/doubles/mock_drm.h"
92-#include "mir/test/doubles/mock_gbm.h"
93-
94 #include <gmock/gmock.h>
95 #include <gtest/gtest.h>
96
97@@ -44,6 +48,7 @@
98 namespace mg = mir::graphics;
99 namespace mgm = mir::graphics::mesa;
100 namespace geom = mir::geometry;
101+namespace mt = mir::test;
102 namespace mtd = mir::test::doubles;
103 namespace mtf = mir_test_framework;
104
105@@ -65,6 +70,24 @@
106 return mg::DisplayConfigurationMode{size, vrefresh_hz};
107 }
108
109+struct MainLoop
110+{
111+ MainLoop()
112+ {
113+ int const owner{0};
114+ mt::Signal mainloop_started;
115+ ml.enqueue(&owner, [&] { mainloop_started.raise(); });
116+ mt::AutoUnblockThread t([this]{ml.stop();}, [this]{ml.run();});
117+ bool const started = mainloop_started.wait_for(std::chrono::seconds(10));
118+ if (!started)
119+ throw std::runtime_error("Failed to start main loop");
120+
121+ ml_thread = std::move(t);
122+ }
123+ mir::GLibMainLoop ml{std::make_shared<mir::time::SteadyClock>()};
124+ mt::AutoUnblockThread ml_thread;
125+};
126+
127 class MesaDisplayConfigurationTest : public ::testing::Test
128 {
129 public:
130@@ -362,6 +385,7 @@
131 TEST_F(MesaDisplayConfigurationTest, returns_updated_configuration)
132 {
133 using namespace ::testing;
134+ using namespace std::chrono_literals;
135
136 uint32_t const invalid_id{0};
137 std::vector<uint32_t> const crtc_ids{10, 11};
138@@ -468,6 +492,7 @@
139
140 /* Set up DRM resources and check */
141 mtd::FakeDRMResources& resources(mock_drm.fake_drm);
142+ auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
143
144 resources.reset();
145
146@@ -530,6 +555,13 @@
147
148 resources.prepare();
149
150+ /* Fake a device change so display can fetch updated configuration*/
151+ MainLoop ml;
152+ mt::Signal handler_signal;
153+ display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
154+ fake_devices.emit_device_changed(syspath);
155+ ASSERT_TRUE(handler_signal.wait_for(10s));
156+
157 conf = display->configuration();
158
159 card_count = 0;
160@@ -556,6 +588,7 @@
161 TEST_F(MesaDisplayConfigurationTest, new_monitor_defaults_to_preferred_mode)
162 {
163 using namespace ::testing;
164+ using namespace std::chrono_literals;
165
166 uint32_t const crtc_ids[1] = {10};
167 uint32_t const encoder_ids[2] = {20, 21};
168@@ -612,6 +645,7 @@
169 };
170
171 mtd::FakeDRMResources& resources(mock_drm.fake_drm);
172+ auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
173
174 resources.reset();
175 resources.add_crtc(crtc_ids[0], modes0[1]);
176@@ -644,6 +678,12 @@
177 connector_physical_sizes_mm_after);
178 resources.prepare();
179
180+ MainLoop ml;
181+ mt::Signal handler_signal;
182+ display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
183+ fake_devices.emit_device_changed(syspath);
184+ ASSERT_TRUE(handler_signal.wait_for(10s));
185+
186 conf = display->configuration();
187 output_count = 0;
188 conf->for_each_output([&](mg::DisplayConfigurationOutput const& output)
189@@ -654,3 +694,44 @@
190 });
191 EXPECT_EQ(noutputs, output_count);
192 }
193+
194+TEST_F(MesaDisplayConfigurationTest, does_not_query_drm_unnecesarily)
195+{
196+ using namespace ::testing;
197+ using namespace std::chrono_literals;
198+
199+ uint32_t const crtc_id{10};
200+ uint32_t const encoder_id{20};
201+ uint32_t const connector_id{30};
202+ geom::Size const connector_physical_sizes_mm{480, 270};
203+ std::vector<uint32_t> possible_encoder_ids_empty;
204+
205+ uint32_t const possible_crtcs_mask_empty{0};
206+ mtd::FakeDRMResources& resources(mock_drm.fake_drm);
207+ resources.reset();
208+ resources.add_crtc(crtc_id, modes0[1]);
209+ resources.add_encoder(encoder_id, crtc_id, possible_crtcs_mask_empty);
210+ resources.add_connector(connector_id, DRM_MODE_CONNECTOR_Composite,
211+ DRM_MODE_CONNECTED, encoder_id,
212+ modes0, possible_encoder_ids_empty,
213+ connector_physical_sizes_mm);
214+ resources.prepare();
215+
216+ auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
217+
218+ auto display = create_display(create_platform());
219+
220+ // No display change reported yet so display should not query drm
221+ EXPECT_CALL(mock_drm, drmModeGetConnector(_,_)).Times(0);
222+ display->configuration();
223+
224+ // Now signal a device change
225+ MainLoop ml;
226+ mt::Signal handler_signal;
227+ display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
228+ fake_devices.emit_device_changed(syspath);
229+ ASSERT_TRUE(handler_signal.wait_for(10s));
230+
231+ EXPECT_CALL(mock_drm, drmModeGetConnector(_,_)).Times(1);
232+ display->configuration();
233+}

Subscribers

People subscribed via source and target branches