Mir

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

Proposed by Daniel van Vugt on 2016-11-02
Status: Merged
Approved by: Daniel van Vugt on 2016-11-07
Approved revision: 3801
Merged at revision: 3804
Proposed branch: lp:~vanvugt/mir/fix-1584894
Merge into: lp:mir
Diff against target: 202 lines (+66/-30)
5 files modified
src/platforms/mesa/server/kms/display_buffer.cpp (+20/-12)
src/platforms/mesa/server/kms/kms_page_flipper.cpp (+6/-0)
src/platforms/mesa/server/kms/real_kms_output.cpp (+9/-6)
tests/unit-tests/platforms/mesa/kms/test_display.cpp (+27/-8)
tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp (+4/-4)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1584894
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve on 2016-11-04
Andreas Pokorny (community) 2016-11-02 Approve on 2016-11-03
Mir CI Bot continuous-integration Approve on 2016-11-02
Review via email: mp+309819@code.launchpad.net

Commit message

If page flipping fails then fall back to blitting the screen
instead of crashing. This allows Mir servers to run under VirtualBox
(LP: #1584894) and also on ASpeed graphics chips.

Server-side GL rendering works fine, as do software clients. Hardware
(GL) clients presently just hang and never render, but I'm happy to
declare that outside the scope of LP: #1584894.

Description of the change

Surprisingly resizing the screen (the VirtualBox window) also works. VirtualBox simply 'unplugs' the old virtual display and plugs in a new one with the new dimensions. And Mir already supports such hotplugging.

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

PASSED: Continuous integration, rev:3801
https://mir-jenkins.ubuntu.com/job/mir-ci/2097/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2697
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2760
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2752
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2752
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2752
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2726/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2726/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2726/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/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2726/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/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2726/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2726
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2726/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

The lack of hardware client support is probably because DRM_CAP_PRIME is missing (returned as zero) under VirtualBox. We may need to fall back to flink to resolve that on systems that lack PRIME. I'm hoping that's all it is...

Andreas Pokorny (andreas-pokorny) wrote :

I tried it on a ast-drm system. But the system behaves strange compared to x11. I.e. in fingerpaint one mouse down the system freezes for a short time - or the frame rate drops to below one frame per second. I havent tried virtual box - but the vbox drm driver code claims to be a fork of ast.

Approve because it is an improvement over crashing on startup..

review: Approve
Daniel van Vugt (vanvugt) wrote :

Fingerpaint might be driving the display too hard (since it uses swap interval 0). Maybe try the -w option to force swap interval 1. That said, there is no throttling in this code path so my VM for example renders >80FPS (according to compositor report) despite the virtual output reporting 59.8Hz.

If your ast system supports wait for vblank then we should do that (after set_crtc) to slow it down, perhaps. Or just sleep, in case the driver doesn't support wait for vblank.

Andreas Pokorny (andreas-pokorny) wrote :

It also happens with flicker.. I will add a few reports to learn more. It would be easier if we could restrict mir to only open specific dri nodes.. I currently have to remove the amd card, because it would otherwise try to poke on the amd card.. especially then for rendering.. and the aspeed then stops working.

Kevin DuBois (kdub) wrote :

alright by me

review: Approve

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_buffer.cpp'
2--- src/platforms/mesa/server/kms/display_buffer.cpp 2016-10-12 13:31:04 +0000
3+++ src/platforms/mesa/server/kms/display_buffer.cpp 2016-11-02 08:19:38 +0000
4@@ -22,6 +22,7 @@
5 #include "bypass.h"
6 #include "gbm_buffer.h"
7 #include "mir/fatal.h"
8+#include "mir/log.h"
9 #include "native_buffer.h"
10
11 #include <boost/throw_exception.hpp>
12@@ -247,8 +248,17 @@
13 {
14 for (auto& output : outputs)
15 {
16+ /*
17+ * Note that failure to set the CRTC is not a fatal error. This can
18+ * happen under normal conditions when resizing VirtualBox (which
19+ * actually removes and replaces the virtual output each time so
20+ * sometimes it's really not there). Xorg often reports similar
21+ * errors, and it's not fatal.
22+ */
23 if (!output->set_crtc(forced_frame->get_drm_fb_id()))
24- fatal_error("Failed to set DRM CRTC");
25+ mir::log_error("Failed to set DRM CRTC. "
26+ "Screen contents may be incomplete. "
27+ "Try plugging the monitor in again.");
28 }
29 }
30
31@@ -275,19 +285,17 @@
32 }
33
34 /*
35- * Schedule the current front buffer object for display, and wait
36- * for it to be actually displayed (flipped).
37- *
38- * If the flip fails, release the buffer object to make it available
39- * for future rendering.
40+ * Try to schedule a page flip as first preference to avoid tearing.
41+ * [will complete in a background thread]
42 */
43 if (!needs_set_crtc && !schedule_page_flip(bufobj))
44- {
45- if (!bypass_buf)
46- bufobj->release();
47- fatal_error("Failed to schedule page flip");
48- }
49- else if (needs_set_crtc)
50+ needs_set_crtc = true;
51+
52+ /*
53+ * Fallback blitting: Not pretty, since it may tear. VirtualBox seems
54+ * to need to do this on every frame. [will complete in this thread]
55+ */
56+ if (needs_set_crtc)
57 {
58 set_crtc(bufobj);
59 needs_set_crtc = false;
60
61=== modified file 'src/platforms/mesa/server/kms/kms_page_flipper.cpp'
62--- src/platforms/mesa/server/kms/kms_page_flipper.cpp 2016-10-12 06:03:15 +0000
63+++ src/platforms/mesa/server/kms/kms_page_flipper.cpp 2016-11-02 08:19:38 +0000
64@@ -71,6 +71,12 @@
65
66 pending_page_flips[crtc_id] = PageFlipEventData{crtc_id, connector_id, this};
67
68+ /*
69+ * It appears we can't tell the difference between flipping being
70+ * unsupported or failing for other reasons. On VirtualBox this always
71+ * fails with -22 (Invalid argument) despite the arguments being
72+ * apparently valid.
73+ */
74 auto ret = drmModePageFlip(drm_fd, crtc_id, fb_id,
75 DRM_MODE_PAGE_FLIP_EVENT,
76 &pending_page_flips[crtc_id]);
77
78=== modified file 'src/platforms/mesa/server/kms/real_kms_output.cpp'
79--- src/platforms/mesa/server/kms/real_kms_output.cpp 2016-11-01 09:34:45 +0000
80+++ src/platforms/mesa/server/kms/real_kms_output.cpp 2016-11-02 08:19:38 +0000
81@@ -112,8 +112,9 @@
82 {
83 if (!ensure_crtc())
84 {
85- fatal_error("Output %s has no associated CRTC to set a framebuffer on",
86- mgk::connector_name(connector).c_str());
87+ mir::log_error("Output %s has no associated CRTC to set a framebuffer on",
88+ mgk::connector_name(connector).c_str());
89+ return false;
90 }
91
92 auto ret = drmModeSetCrtc(drm_fd, current_crtc->crtc_id,
93@@ -165,8 +166,9 @@
94 return true;
95 if (!current_crtc)
96 {
97- fatal_error("Output %s has no associated CRTC to schedule page flips on",
98- mgk::connector_name(connector).c_str());
99+ mir::log_error("Output %s has no associated CRTC to schedule page flips on",
100+ mgk::connector_name(connector).c_str());
101+ return false;
102 }
103 return page_flipper->schedule_flip(current_crtc->crtc_id, fb_id, connector_id);
104 }
105@@ -285,8 +287,9 @@
106 {
107 if (!ensure_crtc())
108 {
109- fatal_error("Output %s has no associated CRTC to set gamma on",
110- mgk::connector_name(connector).c_str());
111+ mir::log_warning("Output %s has no associated CRTC to set gamma on",
112+ mgk::connector_name(connector).c_str());
113+ return;
114 }
115
116 if (gamma.red.size() != gamma.green.size() ||
117
118=== modified file 'tests/unit-tests/platforms/mesa/kms/test_display.cpp'
119--- tests/unit-tests/platforms/mesa/kms/test_display.cpp 2016-10-12 06:03:15 +0000
120+++ tests/unit-tests/platforms/mesa/kms/test_display.cpp 2016-11-02 08:19:38 +0000
121@@ -458,10 +458,23 @@
122
123 setup_post_update_expectations();
124
125+ // clear_crtc happens at some stage. Not interesting.
126+ EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
127+ crtc_id, 0,
128+ _, _, _, _, _))
129+ .WillOnce(Return(0));
130+
131 {
132 InSequence s;
133
134- /* New FB flip failure */
135+ // DisplayBuffer construction paints an empty screen.
136+ // That's probably less than ideal but we've always had it that way.
137+ EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
138+ crtc_id, fake.fb_id1,
139+ _, _, _, _, _))
140+ .WillOnce(Return(0));
141+
142+ // New FB flip failure
143 EXPECT_CALL(mock_drm, drmModePageFlip(mock_drm.fake_drm.fd(),
144 crtc_id,
145 fake.fb_id2,
146@@ -469,16 +482,22 @@
147 .Times(Exactly(1))
148 .WillOnce(Return(-1));
149
150- /* Release failed bufobj */
151+ // Expect fallback to blitting
152+ EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
153+ crtc_id, fake.fb_id2,
154+ _, _, _, _, _))
155+ .WillOnce(Return(0));
156+
157+ // Release all buffer objects
158+ EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
159+ .Times(Exactly(1));
160+
161 EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo2))
162 .Times(Exactly(1));
163-
164- /* Release scheduled_bufobj (at destruction time) */
165- EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
166- .Times(Exactly(1));
167 }
168
169- EXPECT_THROW(
170+ // drmModePageFlip is allowed to fail (e.g. on VirtualBox)
171+ EXPECT_NO_THROW(
172 {
173 auto display = create_display(create_platform());
174 display->for_each_display_sync_group([](mg::DisplaySyncGroup& group) {
175@@ -487,7 +506,7 @@
176 });
177 group.post();
178 });
179- }, std::runtime_error);
180+ });
181 }
182
183 TEST_F(MesaDisplayTest, successful_creation_of_display_reports_successful_setup_of_native_resources)
184
185=== modified file 'tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp'
186--- tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp 2016-11-01 09:30:32 +0000
187+++ tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp 2016-11-02 08:19:38 +0000
188@@ -237,10 +237,10 @@
189 mt::fake_shared(mock_page_flipper)};
190
191 EXPECT_FALSE(output.set_crtc(fb_id));
192- EXPECT_THROW({
193- output.schedule_page_flip(fb_id);
194- }, std::runtime_error);
195- EXPECT_THROW({
196+ EXPECT_NO_THROW({
197+ EXPECT_FALSE(output.schedule_page_flip(fb_id));
198+ });
199+ EXPECT_THROW({ // schedule failed. It's programmer error if you then wait.
200 output.wait_for_page_flip();
201 }, std::runtime_error);
202 }

Subscribers

People subscribed via source and target branches