Merge lp:~vanvugt/mir/async-page-flips into lp:mir

[Daniel van Vugt]

Parallelize page flipping with rendering (of the next frame).

Blocking rendering of the next frame until your page flip has finished is
pretty harmless on a single-headed system. However, in the case of cloned
outputs, waiting for multiple flips can take up to the whole frame time
(as monitors could easily be 16ms out of phase). This left little or no time
for rendering, resulting in dropped frames (stuttering) with cloned outputs
(LP: #1213801).

The solution is to wait for the page flips in parallel to rendering the next
frame. So it doesn't matter if your cloned outputs are many milliseconds
out of phase. That will no longer take away from your rendering time.

[Alexandros Frantzis (afrantzis)]

> The solution is to wait for the page flips in parallel to rendering the next frame

Unless I am missing something, this is not safe. After we schedule a page flip we can't start rendering to the buffer that is currently visible before the page flip is complete. It's important to note that when we flip a buffer, the buffer itself becomes the display framebuffer (i.e. where the display hardware reads from for the whole duration of the frame), its contents are not copied to some other internal framebuffer. Here is a sample run with the proposed changes:

(Front,Back): which buffer is front/back from gbm's point of view

== Initial setup ==

make_current: (A,B) current rendering target is B
draw/clear: (A,B) clearing B
swap: (B,A)
set_crtc: (B,A) B is immediately visible on screen

== First iter ==

make_current: (B,A) current rendering target is A
draw: (B,A) drawing to A
wait_for_page_flip: (B,A) nothing to wait for
swap: (A,B)
schedule_page_flip: (A,B) scheduling for A to become visible on screen

== Second iter ==

make_current: (A,B) current rendering target is B
*** draw: (A,B) drawing to B => there is no guarantee that the page flip has happened, so B may still be visible on screen!

Disapproving, although I do hope I am missing something and thus proved wrong.

review: Disapprove

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1381
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/752/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/771
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/768
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/372
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/482
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/482/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/486
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/486/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/373
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/373/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/375
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3477

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Alexandros,

You are mistaken and I am indeed aware of the perils of touching the currently-scanning-out-buffer. Learned that the hard way when I did bypass... Fortunately when you do make that mistake it's usually visibly obvious.

What makes this proposal safe is that we hold both the buffer being scheduled and the buffer currently visible ("last_flipped_"), to ensure neither of them ever get modified until they have been retired by a later page flip.

Buffers are only released when they are neither scheduled, nor being scanned out:

// Presently visible is either last_flipped_bufobj (composited), last_flipped_bypass_buf (bypassed) or
// scheduled_bufobj (recently flipped which replaced last_flipped_bufobj).
wait_for_page_flip();

// Now presently visible is either scheduled_bufobj (composited) or last_flipped_bypass_buf (bypassed), so
// those are the ones we need to continue holding references to.

if (scheduled_bufobj) // switched from bypass to now compositing
    last_flipped_bypass_buf = nullptr; // release formerly bypassed buffer

// last_flipped_bufobj is now guaranteed no longer on screen, since wait_for_page_flip
if (last_flipped_bufobj)
    last_flipped_bufobj->release();

// Hold the reference so we don't release/reuse the on-screen buffer until something replaces it
last_flipped_bufobj = scheduled_bufobj;

It's confusing I know. But I believe it is correct.

[Daniel van Vugt (vanvugt)]

Alexandros,

I think I see where you're getting confused. You're assuming that GBM only has two buffers (A and B). Therefore you're also assuming that GBM can willingly violate its own lock semantics [1] and reuse a buffer as the back while Mir still has it locked as a front buffer. Either that or you're assuming that I'm violating GBM lock semantics. None of those are true.

GBM will not allow a currently locked front buffer to be reused as a back buffer. You have to release it first, which we only do after a replacement has been flipped. In fact, if you look at the Mesa source, it allocates a fixed limit of 3-4 color buffers [2] depending on which version of Mesa you have (trusty has 3, upstream Mesa has 4).

The 3 buffers we use are:
  A: last_flipped_bufobj
  B: scheduled_bufobj
  C: The EGL back buffer currently being rendered to (there is no GBM function to access this until you swap and it becomes front).

Thus, yes, this proposal will trigger the internal use of 3 color buffers (as opposed to 2 at present) in Mesa. But it does so safely and guarantees that the visible buffer can not get reused as a back buffer while it is visible.

[1] http://cgit.freedesktop.org/mesa/mesa/tree/src/gbm/main/gbm.c#n406
[2] http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_dri2.h#n197

[Alexandros Frantzis (afrantzis)]

> I think I see where you're getting confused. You're assuming that GBM only has two buffers (A and B).
...

Excellent, that was indeed my incorrect assumption. With more than 2 buffers this optimization is safe.

I haven't yet processed the interaction with bypass, but for normal rendering the optimization seems ok, apart from a small glitch in the set up: I think that in the constructor we should be storing the locked front buffer in scheduled_bufobj instead of last_flipped_bufobj. Using last_flipped_bufobj will lead to the release of the front buffer in the first post_update(), after the null wait_for_page_flip, while that front buffer is still visible.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alexandros:
Thanks. Constructor issue fixed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1384
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/761/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/783
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/780
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/491
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/491/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/495
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/495/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/384
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/384/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/386
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3557

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

This looks like "no obvious problems" - I can't help wondering if there's a way to achieve "obviously no problems".

If I get any good ideas I'll be back.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> This looks like "no obvious problems" - I can't help wondering if there's a
> way to achieve "obviously no problems".
>
> If I get any good ideas I'll be back.

The more I think about this the clearer the code looks. That's probably an illusion, but I'll go along with it.

review: Approve

[Daniel van Vugt (vanvugt)]

No new issues raised since the last one was fixed 7 days ago... Top approving.

[Alexandros Frantzis (afrantzis)]

review: Approve

=== modified file 'src/platform/graphics/mesa/display.cpp'
--- src/platform/graphics/mesa/display.cpp	2014-01-22 08:32:55 +0000
+++ src/platform/graphics/mesa/display.cpp	2014-02-03 10:43:40 +0000
@@ -132,6 +132,17 @@
         auto const& kms_conf = dynamic_cast<RealKMSDisplayConfiguration const&>(conf);
         std::vector<std::unique_ptr<DisplayBuffer>> display_buffers_new;
 
+        /*
+         * Notice for a little while here we will have duplicate
+         * DisplayBuffers attached to each output, and the display_buffers_new
+         * will take over the outputs before the old display_buffers are
+         * destroyed. So to avoid page flipping confusion in-between, make
+         * sure we wait for all pending page flips to finish before the
+         * display_buffers_new are created and take control of the outputs.
+         */
+        for (auto& db : display_buffers)
+            db->wait_for_page_flip();
+
         /* Reset the state of all outputs */
         kms_conf.for_each_output([&](DisplayConfigurationOutput const& conf_output)
         {

=== modified file 'src/platform/graphics/mesa/display_buffer.cpp'
--- src/platform/graphics/mesa/display_buffer.cpp	2014-01-23 17:26:51 +0000
+++ src/platform/graphics/mesa/display_buffer.cpp	2014-02-03 10:43:40 +0000
@@ -103,6 +103,7 @@
     MirOrientation rot,
     EGLContext shared_context)
     : last_flipped_bufobj{nullptr},
+      scheduled_bufobj{nullptr},
       platform(platform),
       listener(listener),
       drm(platform->drm),
@@ -110,7 +111,8 @@
       surface_gbm{std::move(surface_gbm_param)},
       area(area),
       rotation(rot),
-      needs_set_crtc{false}
+      needs_set_crtc{false},
+      page_flips_pending{false}
 {
     uint32_t area_width = area.size.width.as_uint32_t();
     uint32_t area_height = area.size.height.as_uint32_t();
@@ -142,13 +144,13 @@
 
     listener->report_successful_egl_buffer_swap_on_construction();
 
-    last_flipped_bufobj = get_front_buffer_object();
-    if (!last_flipped_bufobj)
+    scheduled_bufobj = get_front_buffer_object();
+    if (!scheduled_bufobj)
         BOOST_THROW_EXCEPTION(std::runtime_error("Failed to get frontbuffer"));
 
     for (auto& output : outputs)
     {
-        if (!output->set_crtc(last_flipped_bufobj->get_drm_fb_id()))
+        if (!output->set_crtc(scheduled_bufobj->get_drm_fb_id()))
             BOOST_THROW_EXCEPTION(std::runtime_error("Failed to set DRM crtc"));
     }
 
@@ -171,6 +173,9 @@
      */
     if (last_flipped_bufobj)
         last_flipped_bufobj->release();
+
+    if (scheduled_bufobj)
+        scheduled_bufobj->release();
 }
 
 geom::Rectangle mgm::DisplayBuffer::view_area() const
@@ -206,6 +211,31 @@
     std::shared_ptr<graphics::Buffer> bypass_buf)
 {
     /*
+     * If the last frame was composited then we haven't waited for the
+     * page flips yet. This is good because it maximizes the time available
+     * to spend rendering each frame. However we have to wait here, because
+     * it will be unsafe to swap_buffers before guaranteeing the previous
+     * page flip finished.
+     */
+    wait_for_page_flip();
+
+    /*
+     * Switching from bypass to compositing? Now is the earliest safe time
+     * we can unreference the bypass buffer...
+     */
+    if (scheduled_bufobj)
+        last_flipped_bypass_buf = nullptr;
+    /*
+     * Release the last flipped buffer object (which is not displayed anymore)
+     * to make it available for future rendering.
+     */
+    if (last_flipped_bufobj)
+        last_flipped_bufobj->release();
+
+    last_flipped_bufobj = scheduled_bufobj;
+    scheduled_bufobj = nullptr;
+
+    /*
      * Bring the back buffer to the front and get the buffer object
      * corresponding to the front buffer.
      */
@@ -234,7 +264,7 @@
      * If the flip fails, release the buffer object to make it available
      * for future rendering.
      */
-    if (!needs_set_crtc && !schedule_and_wait_for_page_flip(bufobj))
+    if (!needs_set_crtc && !schedule_page_flip(bufobj))
     {
         if (!bypass_buf)
             bufobj->release();
@@ -250,22 +280,32 @@
         needs_set_crtc = false;
     }
 
-    /*
-     * Release the last flipped buffer object (which is not displayed anymore)
-     * to make it available for future rendering.
-     */
-    if (last_flipped_bufobj)
-        last_flipped_bufobj->release();
-
-    last_flipped_bufobj = bypass_buf ? nullptr : bufobj;
-
-    /*
-     * Keep a reference to the buffer being bypassed for the entire duration
-     * of the frame. This ensures the buffer doesn't get reused by the client
-     * prematurely, which would be seen as tearing.
-     * If not bypassing, then bypass_buf will be nullptr.
-     */
-    last_flipped_bypass_buf = bypass_buf;
+    if (bypass_buf)
+    {
+        /*
+         * For composited frames we defer wait_for_page_flip till just before
+         * the next frame, but not for bypass frames. Deferring the flip of
+         * bypass frames would increase the time we held
+         * last_flipped_bypass_buf unacceptably, resulting in client stuttering
+         * unless we allocate quad-buffers (which I'm trying to avoid).
+         * Also, bypass does not need the deferred page flip because it has
+         * no compositing/rendering step for which to save time for.
+         */
+        wait_for_page_flip();
+        scheduled_bufobj = nullptr;
+
+        /*
+         * Keep a reference to the buffer being bypassed for the entire
+         * duration of the frame. This ensures the buffer doesn't get reused by
+         * the client while its on-screen, which would be seen as tearing or
+         * worse.
+         */
+        last_flipped_bypass_buf = bypass_buf;
+    }
+    else
+    {
+        scheduled_bufobj = bufobj;
+    }
 }
 
 mgm::BufferObject* mgm::DisplayBuffer::get_front_buffer_object()
@@ -311,10 +351,8 @@
 }
 
 
-bool mgm::DisplayBuffer::schedule_and_wait_for_page_flip(BufferObject* bufobj)
+bool mgm::DisplayBuffer::schedule_page_flip(BufferObject* bufobj)
 {
-    int page_flips_pending{0};
-
     /*
      * Schedule the current front buffer object for display. Note that
      * the page flip is asynchronous and synchronized with vertical refresh.
@@ -322,18 +360,21 @@
     for (auto& output : outputs)
     {
         if (output->schedule_page_flip(bufobj->get_drm_fb_id()))
-            ++page_flips_pending;
+            page_flips_pending = true;
     }
 
-    if (page_flips_pending == 0)
-        return false;
+    return page_flips_pending;
+}
 
-    for (auto& output : outputs)
+void mgm::DisplayBuffer::wait_for_page_flip()
+{
+    if (page_flips_pending)
     {
-        output->wait_for_page_flip();
+        for (auto& output : outputs)
+            output->wait_for_page_flip();
+
+        page_flips_pending = false;
     }
-
-    return true;
 }
 
 void mgm::DisplayBuffer::make_current()

=== modified file 'src/platform/graphics/mesa/display_buffer.h'
--- src/platform/graphics/mesa/display_buffer.h	2014-01-23 17:26:51 +0000
+++ src/platform/graphics/mesa/display_buffer.h	2014-02-03 10:43:40 +0000
@@ -63,13 +63,15 @@
                                 std::function<void(Renderable const&)> const& render_fn);
     MirOrientation orientation() const override;
     void schedule_set_crtc();
+    void wait_for_page_flip();
 
 private:
     BufferObject* get_front_buffer_object();
     BufferObject* get_buffer_object(struct gbm_bo *bo);
-    bool schedule_and_wait_for_page_flip(BufferObject* bufobj);
+    bool schedule_page_flip(BufferObject* bufobj);
 
     BufferObject* last_flipped_bufobj;
+    BufferObject* scheduled_bufobj;
     std::shared_ptr<graphics::Buffer> last_flipped_bypass_buf;
     std::shared_ptr<Platform> const platform;
     std::shared_ptr<DisplayReport> const listener;
@@ -82,6 +84,7 @@
     uint32_t fb_width, fb_height;
     MirOrientation rotation;
     std::atomic<bool> needs_set_crtc;
+    bool page_flips_pending;
 };
 
 }

=== added file 'tests/unit-tests/graphics/mesa/mock_kms_output.h'
--- tests/unit-tests/graphics/mesa/mock_kms_output.h	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/graphics/mesa/mock_kms_output.h	2014-02-03 10:43:40 +0000
@@ -0,0 +1,52 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#ifndef MOCK_KMS_OUTPUT_H_
+#define MOCK_KMS_OUTPUT_H_
+
+#include "src/platform/graphics/mesa/kms_output.h"
+#include <gmock/gmock.h>
+
+namespace mir
+{
+namespace test
+{
+
+struct MockKMSOutput : public graphics::mesa::KMSOutput
+{
+    MOCK_METHOD0(reset, void());
+    MOCK_METHOD2(configure, void(geometry::Displacement, size_t));
+    MOCK_CONST_METHOD0(size, geometry::Size());
+
+    MOCK_METHOD1(set_crtc, bool(uint32_t));
+    MOCK_METHOD0(clear_crtc, void());
+    MOCK_METHOD1(schedule_page_flip, bool(uint32_t));
+    MOCK_METHOD0(wait_for_page_flip, void());
+
+    MOCK_METHOD1(set_cursor, void(gbm_bo*));
+    MOCK_METHOD1(move_cursor, void(geometry::Point));
+    MOCK_METHOD0(clear_cursor, void());
+    MOCK_CONST_METHOD0(has_cursor, bool());
+
+    MOCK_METHOD1(set_power_mode, void(MirPowerMode));
+};
+
+} // namespace test
+} // namespace mir
+
+#endif // MOCK_KMS_OUTPUT_H_

=== modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
--- tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-01-13 06:12:33 +0000
+++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-02-03 10:43:40 +0000
@@ -22,6 +22,7 @@
 #include "src/platform/graphics/mesa/kms_display_configuration.h"
 
 #include "mir_test_doubles/mock_gbm.h"
+#include "mock_kms_output.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -32,29 +33,11 @@
 namespace mgm = mir::graphics::mesa;
 namespace geom = mir::geometry;
 namespace mtd = mir::test::doubles;
+using mir::test::MockKMSOutput;
 
 namespace
 {
 
-struct MockKMSOutput : public mgm::KMSOutput
-{
-    MOCK_METHOD0(reset, void());
-    MOCK_METHOD2(configure, void(geom::Displacement, size_t));
-    MOCK_CONST_METHOD0(size, geom::Size());
-
-    MOCK_METHOD1(set_crtc, bool(uint32_t));
-    MOCK_METHOD0(clear_crtc, void());
-    MOCK_METHOD1(schedule_page_flip, bool(uint32_t));
-    MOCK_METHOD0(wait_for_page_flip, void());
-
-    MOCK_METHOD1(set_cursor, void(gbm_bo*));
-    MOCK_METHOD1(move_cursor, void(geom::Point));
-    MOCK_METHOD0(clear_cursor, void());
-    MOCK_CONST_METHOD0(has_cursor, bool());
-
-    MOCK_METHOD1(set_power_mode, void(MirPowerMode));
-};
-
 struct StubKMSOutputContainer : public mgm::KMSOutputContainer
 {
     StubKMSOutputContainer()

=== modified file 'tests/unit-tests/graphics/mesa/test_display.cpp'
--- tests/unit-tests/graphics/mesa/test_display.cpp	2014-01-13 06:12:33 +0000
+++ tests/unit-tests/graphics/mesa/test_display.cpp	2014-02-03 10:43:40 +0000
@@ -428,14 +428,12 @@
 
         /* Handle the flip event */
         EXPECT_CALL(mock_drm, drmHandleEvent(mock_drm.fake_drm.fd(), _))
-            .Times(1)
-            .WillOnce(DoAll(InvokePageFlipHandler(&user_data), Return(0)));
+            .Times(0);
 
-        /* Release the current FB */
+        /* Release last_flipped_bufobj (at destruction time) */
         EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
             .Times(Exactly(1));
-
-        /* Release the new FB (at destruction time) */
+        /* Release scheduled_bufobj (at destruction time) */
         EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo2))
             .Times(Exactly(1));
     }
@@ -468,11 +466,11 @@
             .Times(Exactly(1))
             .WillOnce(Return(-1));
 
-        /* Release the new (not flipped) BO */
+        /* Release failed bufobj */
         EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo2))
             .Times(Exactly(1));
 
-        /* Release the current FB (at destruction time) */
+        /* Release scheduled_bufobj (at destruction time) */
         EXPECT_CALL(mock_gbm, gbm_surface_release_buffer(mock_gbm.fake_gbm.surface, fake.bo1))
             .Times(Exactly(1));
     }

=== modified file 'tests/unit-tests/graphics/mesa/test_display_buffer.cpp'
--- tests/unit-tests/graphics/mesa/test_display_buffer.cpp	2014-01-21 06:27:40 +0000
+++ tests/unit-tests/graphics/mesa/test_display_buffer.cpp	2014-02-03 10:43:40 +0000
@@ -25,6 +25,7 @@
 #include "mir_test_doubles/mock_drm.h"
 #include "mir_test_doubles/mock_gbm.h"
 #include "mir_test_framework/udev_environment.h"
+#include "mock_kms_output.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -33,8 +34,10 @@
 using namespace testing;
 using namespace mir;
 using namespace std;
+using namespace mir::test;
 using namespace mir::test::doubles;
 using namespace mir::mir_test_framework;
+using namespace mir::graphics;
 
 class MesaDisplayBufferTest : public Test
 {
@@ -66,6 +69,12 @@
             .WillByDefault(Return(456));
 
         fake_devices.add_standard_drm_devices();
+
+        mock_kms_output = std::make_shared<NiceMock<MockKMSOutput>>();
+        ON_CALL(*mock_kms_output, set_crtc(_))
+            .WillByDefault(Return(true));
+        ON_CALL(*mock_kms_output, schedule_page_flip(_))
+            .WillByDefault(Return(true));
     }
 
     // The platform has an implicit dependency on mock_gbm etc so must be
@@ -85,6 +94,7 @@
     gbm_bo*           fake_bo;
     gbm_bo_handle     fake_handle;
     UdevEnvironment   fake_devices;
+    std::shared_ptr<MockKMSOutput> mock_kms_output;
 };
 
 TEST_F(MesaDisplayBufferTest, unrotated_view_area_is_untouched)
@@ -234,3 +244,55 @@
         mir_orientation_right,
         mock_egl.fake_egl_context);
 }
+
+TEST_F(MesaDisplayBufferTest, first_post_flips_but_no_wait)
+{
+    geometry::Rectangle const area{{12,34}, {56,78}};
+
+    EXPECT_CALL(*mock_kms_output, schedule_page_flip(_))
+        .Times(1);
+    EXPECT_CALL(*mock_kms_output, wait_for_page_flip())
+        .Times(0);
+
+    graphics::mesa::DisplayBuffer db(
+        create_platform(),
+        make_shared<graphics::NullDisplayReport>(),
+        {mock_kms_output},
+        nullptr,
+        area,
+        mir_orientation_normal,
+        mock_egl.fake_egl_context);
+
+    db.post_update();
+}
+
+TEST_F(MesaDisplayBufferTest, waits_for_page_flip_on_second_post)
+{
+    geometry::Rectangle const area{{12,34}, {56,78}};
+
+    InSequence seq;
+
+    EXPECT_CALL(*mock_kms_output, wait_for_page_flip())
+        .Times(0);
+    EXPECT_CALL(*mock_kms_output, schedule_page_flip(_))
+        .Times(1);
+    EXPECT_CALL(*mock_kms_output, wait_for_page_flip())
+        .Times(1);
+    EXPECT_CALL(*mock_kms_output, schedule_page_flip(_))
+        .Times(1);
+    EXPECT_CALL(*mock_kms_output, wait_for_page_flip())
+        .Times(0);
+
+    graphics::mesa::DisplayBuffer db(
+        create_platform(),
+        make_shared<graphics::NullDisplayReport>(),
+        {mock_kms_output},
+        nullptr,
+        area,
+        mir_orientation_normal,
+        mock_egl.fake_egl_context);
+
+    db.post_update();
+    db.post_update();
+}
+

=== modified file 'tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp'
--- tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp	2014-01-13 06:12:33 +0000
+++ tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp	2014-02-03 10:43:40 +0000
@@ -349,8 +349,8 @@
         EXPECT_CALL(mock_drm, drmModePageFlip(mock_drm.fake_drm.fd(),
                                               crtc_ids[i], fb_id,
                                               _, _))
-            .Times(1)
-            .WillOnce(DoAll(SaveArg<4>(&user_data[i]), Return(0)));
+            .Times(2)
+            .WillRepeatedly(DoAll(SaveArg<4>(&user_data[i]), Return(0)));
 
         /* Emit fake DRM page-flip events */
         EXPECT_EQ(1, write(mock_drm.fake_drm.write_fd(), "a", 1));
@@ -365,6 +365,14 @@
 
     auto display = create_display_cloned(create_platform());
 
+    /* First frame: Page flips are scheduled, but not waited for */
+    display->for_each_display_buffer([](mg::DisplayBuffer& buffer)
+    {
+        buffer.post_update();
+    });
+
+    /* Second frame: Previous page flips finish (drmHandleEvent) and new ones
+       are scheduled */
     display->for_each_display_buffer([](mg::DisplayBuffer& buffer)
     {
         buffer.post_update();