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

1 files changed, 25 insertions, 6 deletions

diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index 5c6158c771..292f13de2d 100644
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -324,16 +324,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(
@@ -346,7 +355,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;
 }