From bc52c7039cc89e5cb14a8ee48fabfc70a96c0c89 Mon Sep 17 00:00:00 2001
From: Brian Anderson <brianderson@google.com>
Date: Tue, 8 Aug 2017 16:31:37 -0700
Subject: [PATCH] Merge fences when needed for accurate timestamps.

There's an optimization in ConsumerBase that checks the status
of the current fence before merging it with a new fence. If
the current fence has already signaled, then it just picks up
the new fence without merging.

Unfortunately, if the new fence is already signaled too, then
it's possible that it signaled long before the current fence,
which can result in an inaccurate timestamp with the current
logic.

The new logic merges the fences when the statuses of the current
and new fences are the same. If they differ, then it takes the
unsignaled fence.

This fixes the reads done timestamps in the GetFrameTimestamps
dEQP tests so that they are always monotonic and always arrive
after rendering completes.

Test: --deqp-case=dEQP-EGL*get_frame_timestamps*
Bug: 37513882

Change-Id: I345e48aae0fbb3c28c2f2c0dc035e6b0fa70df43
(cherry picked from commit 7b097e2e3d9dd9444916ddf77d75ca394e6b753e)
---
 libs/gui/ConsumerBase.cpp | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index 3d3637648c..7aa7872513 100644
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -335,16 +335,25 @@ status_t ConsumerBase::addReleaseFenceLocked(int slot,
         return OK;
     }
 
-    auto status = mSlots[slot].mFence->getStatus();
-
-    if (status == Fence::Status::Invalid) {
-        CB_LOGE("fence has invalid state");
+    // Check status of fences first because merging is expensive.
+    // Merging an invalid fence with any other fence results in an
+    // invalid fence.
+    auto currentStatus = mSlots[slot].mFence->getStatus();
+    if (currentStatus == Fence::Status::Invalid) {
+        CB_LOGE("Existing fence has invalid state");
         return BAD_VALUE;
     }
 
-    if (status == Fence::Status::Signaled) {
+    auto incomingStatus = fence->getStatus();
+    if (incomingStatus == Fence::Status::Invalid) {
+        CB_LOGE("New fence has invalid state");
         mSlots[slot].mFence = fence;
-    } else {  // status == Fence::Status::Unsignaled
+        return BAD_VALUE;
+    }
+
+    // If both fences are signaled or both are unsignaled, we need to merge
+    // them to get an accurate timestamp.
+    if (currentStatus == incomingStatus) {
         char fenceName[32] = {};
         snprintf(fenceName, 32, "%.28s:%d", mName.string(), slot);
         sp<Fence> mergedFence = Fence::merge(
@@ -357,7 +366,17 @@ status_t ConsumerBase::addReleaseFenceLocked(int slot,
             return BAD_VALUE;
         }
         mSlots[slot].mFence = mergedFence;
+    } else if (incomingStatus == Fence::Status::Unsignaled) {
+        // If one fence has signaled and the other hasn't, the unsignaled
+        // fence will approximately correspond with the correct timestamp.
+        // There's a small race if both fences signal at about the same time
+        // and their statuses are retrieved with unfortunate timing. However,
+        // by this point, they will have both signaled and only the timestamp
+        // will be slightly off; any dependencies after this point will
+        // already have been met.
+        mSlots[slot].mFence = fence;
     }
+    // else if (currentStatus == Fence::Status::Unsignaled) is a no-op.
 
     return OK;
 }
-- 
2.11.0