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

[Daniel van Vugt]

Proof of concept fix. This might be enough, or maybe not.

[Daniel van Vugt (vanvugt)]

I'm hesitant to endorse this still, because it reduces our ability to catch up in almost-full-framerate scenarios across the board (instead opting to sleep and hope that's enough to give the server more GPU time). I feel this is something we shouldn't be "fixing" because it's not a Mir bug and more important devices (like everything other than Freedreno) could suffer slightly if we land this.

I'll look at other workarounds still that don't make the same sacrifice (although may be much less clean in code than this).

review: Disapprove

Unmerged revisions

4068. By Daniel van Vugt on 2017-03-01

Tidy up the test

4067. By Daniel van Vugt on 2017-03-01

Correct and clean up the fix. Tests all pass

4066. By Daniel van Vugt on 2017-03-01

Expand the test more to fail due to the latching

4065. By Daniel van Vugt on 2017-03-01

Enable the fix (and debug output). Tests pass but the flag is latching.
Needs a better test

4064. By Daniel van Vugt on 2017-03-01

Disable the fix and rewrite the regression test

4063. By Daniel van Vugt on 2017-03-01

Merge latest trunk

4062. By Daniel van Vugt on 2017-02-27

Expand the test and leave in debug output

4061. By Daniel van Vugt on 2017-02-27

Fix the fix. Now all tests pass.

4060. By Daniel van Vugt on 2017-02-27

Try to fix the bug, and bad expectations in older tests

4059. By Daniel van Vugt on 2017-02-27

Write a detailed regression test (presently failing)

=== modified file 'src/client/frame_clock.cpp'
--- src/client/frame_clock.cpp	2016-11-29 08:00:45 +0000
+++ src/client/frame_clock.cpp	2017-03-01 04:44:28 +0000
@@ -103,7 +103,7 @@
      * Crucially this is not required on most frames, so that even if it is
      * implemented as a round trip to the server, that won't happen often.
      */
-    if (missed_frames > 1 || config_changed)
+    if (missed_frames > 1 || config_changed || !throttled)
     {
         lock.unlock();
         auto const server_frame = resync_callback();
@@ -152,5 +152,14 @@
         target = target + missed_frames * period;
     }
 
+    /*
+     * Often we'll be targeting the past in order to catch up after a previous
+     * slow frame. That's correct, but try to not do it multiple frames in a
+     * row, since that might saturate a low-power GPU, which might then starve
+     * the server of GPU time if the driver lacks adequate fair scheduling
+     * (e.g. Freedreno/Gallium LP: #1665802)
+     */
+    throttled = target > now;
+
     return target;
 }

=== modified file 'src/client/frame_clock.h'
--- src/client/frame_clock.h	2016-12-15 08:15:54 +0000
+++ src/client/frame_clock.h	2017-03-01 04:44:28 +0000
@@ -66,6 +66,7 @@
     mutable bool config_changed;
     std::chrono::nanoseconds period;
     ResyncCallback resync_callback;
+    mutable bool throttled = true;
 };
 
 }} // namespace mir::client

=== modified file 'tests/unit-tests/client/test_frame_clock.cpp'
--- tests/unit-tests/client/test_frame_clock.cpp	2017-02-28 13:25:18 +0000
+++ tests/unit-tests/client/test_frame_clock.cpp	2017-03-01 04:44:28 +0000
@@ -135,9 +135,9 @@
     fake_sleep_for(one_frame * 7 / 6);  // long render time; over a frame
 
     auto d = clock.next_frame_after(c);
-    EXPECT_EQ(one_frame, d - c);
+    EXPECT_EQ(2*one_frame, d - c);
 
-    EXPECT_LT(d, now);
+    EXPECT_GT(d, now);
 
     fake_sleep_until(d);
     fake_sleep_for(one_frame/4);  // short render time
@@ -392,8 +392,9 @@
     fake_sleep_for(one_frame * 8 / 5);  // Render time: 1.6 frames
 
     auto d = clock.next_frame_after(c);
-    EXPECT_EQ(2*one_frame, d - c);      // One frame skipped
-    EXPECT_LT(d, now);                  // Targets the past, catching up again
+    EXPECT_EQ(3*one_frame, d - c);      // Two frames skipped (since c targeted
+                                        //   the past but d the future)
+    EXPECT_GT(d, now);                  // Targets the future
 
     fake_sleep_until(d);
     fake_sleep_for(one_frame/4);        // Short render time, immediate recovery
@@ -403,6 +404,95 @@
     EXPECT_GT(e, now);                  // And targeting the future
 }
 
+TEST_F(FrameClockTest, does_not_continuously_target_the_past)
+{   // Regression test for LP: #1665802
+    FrameClock clock(with_fake_time);
+    auto& now = fake_time[CLOCK_MONOTONIC];
+    clock.set_period(one_frame);
+
+    auto a = clock.next_frame_after(PosixTimestamp());
+    fake_sleep_until(a);
+    fake_sleep_for(one_frame / 9);      // Start with a fast frame
+
+    auto b = clock.next_frame_after(a);
+    ASSERT_GT(b, now);
+
+    fake_sleep_until(b);
+    fake_sleep_for(one_frame * 3 / 2);  // Render time: 1.5 frames
+
+    auto c = clock.next_frame_after(b);
+    ASSERT_LT(c, now);                  // Targets the past, catching up
+    ASSERT_EQ(one_frame, c - b);        // No frame skipped
+
+    fake_sleep_until(c);
+
+    bool targeted_past = false;
+
+    auto d = c;
+    for (int slow_frame = 0; slow_frame < 10; ++slow_frame)
+    {
+        fake_sleep_for(one_frame * 8 / 5);  // Render time: 1.6 frames
+        d = clock.next_frame_after(c);
+
+        bool targets_past = d < now;
+        if (targeted_past)
+            ASSERT_FALSE(targets_past);
+        targeted_past = targets_past;
+
+        // May target the future, but not too far in the future:
+        ASSERT_LE(d, now+one_frame);
+        c = d;
+        fake_sleep_until(d);
+    }
+
+    fake_sleep_for(one_frame/4);        // Short render time, immediate recovery
+
+    auto e = clock.next_frame_after(d);
+    ASSERT_EQ(one_frame, e - d);        // No frame skipped. We have recovered.
+    ASSERT_GT(e, now);                  // And targeting the future
+    targeted_past = false;
+
+    /*
+     * In the original bug LP: #1665802 it appeared that the GPU could only
+     * sustain quarter frame rate. So test that scenario too...
+     */
+    auto f = e;
+    for (int slower_frame = 0; slower_frame < 10; ++slower_frame)
+    {
+        fake_sleep_for(one_frame * 17 / 5);  // Render time: 3.4 frames
+        f = clock.next_frame_after(e);
+
+        bool targets_past = f < now;
+        if (targeted_past)
+            ASSERT_FALSE(targets_past);
+        targeted_past = targets_past;
+
+        // May target the future, but not too far in the future:
+        ASSERT_LE(f, now+one_frame);        // But not too far in the future
+        e = f;
+        fake_sleep_until(f);
+    }
+
+    auto g = f;
+
+    for (int recovered_frame = 0; recovered_frame < 10; ++recovered_frame)
+    {
+        fake_sleep_for(one_frame / 10);
+        g = clock.next_frame_after(f);
+        ASSERT_GT(g, now);
+        ASSERT_LE(g, now+one_frame);
+        ASSERT_EQ(one_frame, g - f);
+        f = g;
+        fake_sleep_until(g);
+    }
+
+    fake_sleep_for(one_frame * 3 / 2);  // Render time: 1.5 frames
+
+    auto h = clock.next_frame_after(g);
+    ASSERT_LT(h, now);                  // Targets the past, catching up
+    ASSERT_EQ(one_frame, h - g);        // No frame skipped
+}
+
 TEST_F(FrameClockTest, nesting_adds_zero_lag)
 {
     FrameClock inner(with_fake_time);