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
diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index 5c6158c..292f13d 100644
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -324,16 +324,25 @@
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 @@
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;
}