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

[Daniel van Vugt]

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.

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.

[Mir CI Bot (mir-ci-bot)]

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)]

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)]

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)]

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)]

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)]

alright by me

review: Approve

=== modified file 'src/platforms/mesa/server/kms/display_buffer.cpp'
--- src/platforms/mesa/server/kms/display_buffer.cpp	2016-10-12 13:31:04 +0000
+++ src/platforms/mesa/server/kms/display_buffer.cpp	2016-11-02 08:19:38 +0000
@@ -22,6 +22,7 @@
 #include "bypass.h"
 #include "gbm_buffer.h"
 #include "mir/fatal.h"
+#include "mir/log.h"
 #include "native_buffer.h"
 
 #include <boost/throw_exception.hpp>
@@ -247,8 +248,17 @@
 {
     for (auto& output : outputs)
     {
+        /*
+         * Note that failure to set the CRTC is not a fatal error. This can
+         * happen under normal conditions when resizing VirtualBox (which
+         * actually removes and replaces the virtual output each time so
+         * sometimes it's really not there). Xorg often reports similar
+         * errors, and it's not fatal.
+         */
         if (!output->set_crtc(forced_frame->get_drm_fb_id()))
-            fatal_error("Failed to set DRM CRTC");
+            mir::log_error("Failed to set DRM CRTC. "
+                "Screen contents may be incomplete. "
+                "Try plugging the monitor in again.");
     }
 }
 
@@ -275,19 +285,17 @@
     }
 
     /*
-     * Schedule the current front buffer object for display, and wait
-     * for it to be actually displayed (flipped).
-     *
-     * If the flip fails, release the buffer object to make it available
-     * for future rendering.
+     * Try to schedule a page flip as first preference to avoid tearing.
+     * [will complete in a background thread]
      */
     if (!needs_set_crtc && !schedule_page_flip(bufobj))
-    {
-        if (!bypass_buf)
-            bufobj->release();
-        fatal_error("Failed to schedule page flip");
-    }
-    else if (needs_set_crtc)
+        needs_set_crtc = true;
+
+    /*
+     * Fallback blitting: Not pretty, since it may tear. VirtualBox seems
+     * to need to do this on every frame. [will complete in this thread]
+     */
+    if (needs_set_crtc)
     {
         set_crtc(bufobj);
         needs_set_crtc = false;
 
=== modified file 'src/platforms/mesa/server/kms/kms_page_flipper.cpp'
--- src/platforms/mesa/server/kms/kms_page_flipper.cpp	2016-10-12 06:03:15 +0000
+++ src/platforms/mesa/server/kms/kms_page_flipper.cpp	2016-11-02 08:19:38 +0000
@@ -71,6 +71,12 @@
 
     pending_page_flips[crtc_id] = PageFlipEventData{crtc_id, connector_id, this};
 
+    /*
+     * It appears we can't tell the difference between flipping being
+     * unsupported or failing for other reasons. On VirtualBox this always
+     * fails with -22 (Invalid argument) despite the arguments being
+     * apparently valid.
+     */
     auto ret = drmModePageFlip(drm_fd, crtc_id, fb_id,
                                DRM_MODE_PAGE_FLIP_EVENT,
                                &pending_page_flips[crtc_id]);
 
=== modified file 'src/platforms/mesa/server/kms/real_kms_output.cpp'
--- src/platforms/mesa/server/kms/real_kms_output.cpp	2016-11-01 09:34:45 +0000
+++ src/platforms/mesa/server/kms/real_kms_output.cpp	2016-11-02 08:19:38 +0000
@@ -112,8 +112,9 @@
 {
     if (!ensure_crtc())
     {
-        fatal_error("Output %s has no associated CRTC to set a framebuffer on",
-                    mgk::connector_name(connector).c_str());
+        mir::log_error("Output %s has no associated CRTC to set a framebuffer on",
+                       mgk::connector_name(connector).c_str());
+        return false;
     }
 
     auto ret = drmModeSetCrtc(drm_fd, current_crtc->crtc_id,
@@ -165,8 +166,9 @@
         return true;
     if (!current_crtc)
     {
-        fatal_error("Output %s has no associated CRTC to schedule page flips on",
-                   mgk::connector_name(connector).c_str());
+        mir::log_error("Output %s has no associated CRTC to schedule page flips on",
+                       mgk::connector_name(connector).c_str());
+        return false;
     }
     return page_flipper->schedule_flip(current_crtc->crtc_id, fb_id, connector_id);
 }
@@ -285,8 +287,9 @@
 {
     if (!ensure_crtc())
     {
-        fatal_error("Output %s has no associated CRTC to set gamma on",
-                    mgk::connector_name(connector).c_str());
+        mir::log_warning("Output %s has no associated CRTC to set gamma on",
+                         mgk::connector_name(connector).c_str());
+        return;
     }
 
     if (gamma.red.size() != gamma.green.size() ||
 
=== modified file 'tests/unit-tests/platforms/mesa/kms/test_display.cpp'
--- tests/unit-tests/platforms/mesa/kms/test_display.cpp	2016-10-12 06:03:15 +0000
+++ tests/unit-tests/platforms/mesa/kms/test_display.cpp	2016-11-02 08:19:38 +0000
@@ -458,10 +458,23 @@
 
     setup_post_update_expectations();
 
+    // clear_crtc happens at some stage. Not interesting.
+    EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
+                                         crtc_id, 0,
+                                         _, _, _, _, _))
+        .WillOnce(Return(0));
+
     {
         InSequence s;
 
-        /* New FB flip failure */
+        // DisplayBuffer construction paints an empty screen.
+        // That's probably less than ideal but we've always had it that way.
+        EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
+                                             crtc_id, fake.fb_id1,
+                                             _, _, _, _, _))
+            .WillOnce(Return(0));
+
+        // New FB flip failure
         EXPECT_CALL(mock_drm, drmModePageFlip(mock_drm.fake_drm.fd(),
                                               crtc_id,
                                               fake.fb_id2,
@@ -469,16 +482,22 @@
             .Times(Exactly(1))
             .WillOnce(Return(-1));
 
-        /* Release failed bufobj */
+        // Expect fallback to blitting
+        EXPECT_CALL(mock_drm, drmModeSetCrtc(mock_drm.fake_drm.fd(),
+                                             crtc_id, fake.fb_id2,
+                                             _, _, _, _, _))
+            .WillOnce(Return(0));
+
+        // Release all buffer objects
+        EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
+            .Times(Exactly(1));
+
         EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo2))
             .Times(Exactly(1));
-
-        /* Release scheduled_bufobj (at destruction time) */
-        EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
-            .Times(Exactly(1));
     }
 
-    EXPECT_THROW(
+    // drmModePageFlip is allowed to fail (e.g. on VirtualBox)
+    EXPECT_NO_THROW(
     {
         auto display = create_display(create_platform());
         display->for_each_display_sync_group([](mg::DisplaySyncGroup& group) {
@@ -487,7 +506,7 @@
             });
             group.post();
         });
-    }, std::runtime_error);
+    });
 }
 
 TEST_F(MesaDisplayTest, successful_creation_of_display_reports_successful_setup_of_native_resources)
 
=== modified file 'tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp'
--- tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp	2016-11-01 09:30:32 +0000
+++ tests/unit-tests/platforms/mesa/kms/test_real_kms_output.cpp	2016-11-02 08:19:38 +0000
@@ -237,10 +237,10 @@
                               mt::fake_shared(mock_page_flipper)};
 
     EXPECT_FALSE(output.set_crtc(fb_id));
-    EXPECT_THROW({
-        output.schedule_page_flip(fb_id);
-    }, std::runtime_error);
-    EXPECT_THROW({
+    EXPECT_NO_THROW({
+        EXPECT_FALSE(output.schedule_page_flip(fb_id));
+    });
+    EXPECT_THROW({  // schedule failed. It's programmer error if you then wait.
         output.wait_for_page_flip();
     }, std::runtime_error);
 }