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

[Daniel van Vugt]

Fix deadlocked (frozen) clients when nbuffers==2 (LP: #1319765)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1635
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1646/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/217
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/218
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/218
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/165
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/165/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/164
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/164/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/556
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/556/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1470
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7260

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1640
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1658/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/176
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/176/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/596
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/596/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1483
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7302

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1641
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1659/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/232
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/232
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/178
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/178/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/599
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/599/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1484
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7309

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Indeed.

review: Approve

[Kevin DuBois (kdub)]

looks good, and what the code is accomplishing makes sense to me. Suggestion and a question:

composite_on_demand_never_deadlocks_with_2_buffers is a short test to run, but wouldn't the same lines be hit if we just ran once or twice? Its hard as someone looking at the code for the first time to tease out what the expectations are.

Could having a smarter callback system also avert the problem? (I think the composition scheduling system has some improvements that are needed, curious if the root cause is related)

review: Approve

[Daniel van Vugt (vanvugt)]

kdub:

Yeah it's a complicated (and redundant) test that is mostly just a repeatable copy of the simple test before it. I wanted to iterate it many times to verify the fix didn't just work on the first iteration, and stress it a bit.

Not sure what you mean by "smarter callback system". This proposal ensures all callbacks are made as soon as logically possible (catching the one(?) case where that wasn't already guaranteed). Although I suspect you might be referring to an automatic callback system where callbacks happen implicitly from queue changes...? That would be interesting if it simplified the algorithm (not interesting otherwise :). But that's also not a task for right now.

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-05-15 03:53:45 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-05-19 07:45:18 +0000
@@ -239,13 +239,29 @@
     if (contains(buffer.get(), buffers_sent_to_compositor))
         return;
 
+    if (nbuffers <= 1)
+        return;
+
     /*
-     * This buffer is not owned by compositor anymore. If the queue is supposed
-     * to only own a single buffer (nbuffers == 1), then this is a buffer that
-     * is no longer used (i.e. replaced by a new resized buffer), so just
-     * discard it.
+     * We can't release the current_compositor_buffer because we need to keep
+     * a compositor buffer always-available. But there might be a new
+     * compositor buffer available to take its place immediately. Moving to
+     * that one immediately will free up the old compositor buffer, allowing
+     * us to call back the client with a buffer where otherwise we couldn't.
      */
-    if (current_compositor_buffer != buffer.get() && nbuffers > 1)
+    if (current_compositor_buffer == buffer.get() &&
+        !ready_to_composite_queue.empty())
+    {
+        current_compositor_buffer = pop(ready_to_composite_queue);
+
+        // Ensure current_compositor_buffer gets reused by the next
+        // compositor_acquire:
+        current_buffer_users.clear();
+        void const* const impossible_user_id = this;
+        current_buffer_users.push_back(impossible_user_id);
+    }
+
+    if (current_compositor_buffer != buffer.get())
         release(buffer.get(), std::move(lock));
 }
 

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-15 03:20:25 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-19 07:45:18 +0000
@@ -1068,6 +1068,68 @@
     q.compositor_release(buf);
 }
 
+TEST_F(BufferQueueTest, double_buffered_client_is_not_blocked_prematurely)
+{  // Regression test for LP: #1319765
+    using namespace testing;
+
+    mc::BufferQueue q{2, allocator, basic_properties};
+
+    q.client_release(client_acquire_sync(q));
+    auto a = q.compositor_acquire(this);
+    q.client_release(client_acquire_sync(q));
+    auto b = q.compositor_acquire(this);
+
+    ASSERT_NE(a.get(), b.get());
+
+    q.compositor_release(a);
+    q.client_release(client_acquire_sync(q));
+
+    q.compositor_release(b);
+    auto handle = client_acquire_async(q);
+    // With the fix, a buffer will be available instantaneously:
+    ASSERT_TRUE(handle->has_acquired_buffer());
+    handle->release_buffer();
+}
+
+TEST_F(BufferQueueTest, composite_on_demand_never_deadlocks_with_2_buffers)
+{  // Extended regression test for LP: #1319765
+    using namespace testing;
+
+    mc::BufferQueue q{2, allocator, basic_properties};
+
+    for (int i = 0; i < 100; ++i)
+    {
+        auto x = client_acquire_async(q);
+        ASSERT_TRUE(x->has_acquired_buffer());
+        x->release_buffer();
+
+        auto a = q.compositor_acquire(this);
+
+        auto y = client_acquire_async(q);
+        ASSERT_TRUE(y->has_acquired_buffer());
+        y->release_buffer();
+
+        auto b = q.compositor_acquire(this);
+    
+        ASSERT_NE(a.get(), b.get());
+    
+        q.compositor_release(a);
+
+        auto w = client_acquire_async(q);
+        ASSERT_TRUE(w->has_acquired_buffer());
+        w->release_buffer();
+    
+        q.compositor_release(b);
+
+        auto z = client_acquire_async(q);
+        ASSERT_TRUE(z->has_acquired_buffer());
+        z->release_buffer();
+
+        q.compositor_release(q.compositor_acquire(this));
+        q.compositor_release(q.compositor_acquire(this));
+    }
+}
+
 /* Regression test for LP: #1306464 */
 TEST_F(BufferQueueTest, framedropping_client_acquire_does_not_block_when_no_available_buffers)
 {