From 6c6dd3b2ae7ae677f0448624cf8f6aeafdaffbf5 Mon Sep 17 00:00:00 2001
From: Patrick Williams <pdwilliams@google.com>
Date: Mon, 13 Feb 2023 22:53:06 +0000
Subject: Cache and uncache buffers in the same transaction

This removes a race condition where clients may attempt to cache a
buffer before an associated uncache buffer request has completed,
leading to full buffer cache errors in SurfaceFlinger.

GLSurfaceViewTest#testPauseResumeWithoutDelay is failing on barbet and
test logs show frequent `ClientCache::add - cache is full` messages.
This CL fixes the "cache is full" error but does not fix the broken
test.

This reverts commit 1088bbf9882d778079eaa7465c1dec2b08848f1a.

Reason for revert: b/269141832

Change-Id: I4ba8cf18a367ae6ca94992aabd695d8984fea76f
---
 services/surfaceflinger/SurfaceFlinger.cpp | 49 +++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 17 deletions(-)

(limited to 'services/surfaceflinger/SurfaceFlinger.cpp')

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 40de4d675d..d63e2812e2 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3973,7 +3973,7 @@ bool SurfaceFlinger::applyTransactions(std::vector<TransactionState>& transactio
                                       transaction.displays, transaction.flags,
                                       transaction.inputWindowCommands,
                                       transaction.desiredPresentTime, transaction.isAutoTimestamp,
-                                      transaction.buffer, transaction.postTime,
+                                      std::move(transaction.uncacheBufferIds), transaction.postTime,
                                       transaction.permissions, transaction.hasListenerCallbacks,
                                       transaction.listenerCallbacks, transaction.originPid,
                                       transaction.originUid, transaction.id);
@@ -4061,8 +4061,9 @@ status_t SurfaceFlinger::setTransactionState(
         const FrameTimelineInfo& frameTimelineInfo, Vector<ComposerState>& states,
         const Vector<DisplayState>& displays, uint32_t flags, const sp<IBinder>& applyToken,
         const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime,
-        bool isAutoTimestamp, const client_cache_t& uncacheBuffer, bool hasListenerCallbacks,
-        const std::vector<ListenerCallbacks>& listenerCallbacks, uint64_t transactionId) {
+        bool isAutoTimestamp, const std::vector<client_cache_t>& uncacheBuffers,
+        bool hasListenerCallbacks, const std::vector<ListenerCallbacks>& listenerCallbacks,
+        uint64_t transactionId) {
     ATRACE_CALL();
 
     uint32_t permissions =
@@ -4096,6 +4097,15 @@ status_t SurfaceFlinger::setTransactionState(
     const int originPid = ipc->getCallingPid();
     const int originUid = ipc->getCallingUid();
 
+    std::vector<uint64_t> uncacheBufferIds;
+    uncacheBufferIds.reserve(uncacheBuffers.size());
+    for (const auto& uncacheBuffer : uncacheBuffers) {
+        sp<GraphicBuffer> buffer = ClientCache::getInstance().erase(uncacheBuffer);
+        if (buffer != nullptr) {
+            uncacheBufferIds.push_back(buffer->getId());
+        }
+    }
+
     std::vector<ResolvedComposerState> resolvedStates;
     resolvedStates.reserve(states.size());
     for (auto& state : states) {
@@ -4113,14 +4123,22 @@ status_t SurfaceFlinger::setTransactionState(
         }
     }
 
-    TransactionState state{frameTimelineInfo,  resolvedStates,
-                           displays,           flags,
-                           applyToken,         inputWindowCommands,
-                           desiredPresentTime, isAutoTimestamp,
-                           uncacheBuffer,      postTime,
-                           permissions,        hasListenerCallbacks,
-                           listenerCallbacks,  originPid,
-                           originUid,          transactionId};
+    TransactionState state{frameTimelineInfo,
+                           resolvedStates,
+                           displays,
+                           flags,
+                           applyToken,
+                           inputWindowCommands,
+                           desiredPresentTime,
+                           isAutoTimestamp,
+                           std::move(uncacheBufferIds),
+                           postTime,
+                           permissions,
+                           hasListenerCallbacks,
+                           listenerCallbacks,
+                           originPid,
+                           originUid,
+                           transactionId};
 
     if (mTransactionTracing) {
         mTransactionTracing->addQueuedTransaction(state);
@@ -4144,7 +4162,7 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
                                            Vector<DisplayState>& displays, uint32_t flags,
                                            const InputWindowCommands& inputWindowCommands,
                                            const int64_t desiredPresentTime, bool isAutoTimestamp,
-                                           const client_cache_t& uncacheBuffer,
+                                           const std::vector<uint64_t>& uncacheBufferIds,
                                            const int64_t postTime, uint32_t permissions,
                                            bool hasListenerCallbacks,
                                            const std::vector<ListenerCallbacks>& listenerCallbacks,
@@ -4185,11 +4203,8 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
         ALOGE("Only privileged callers are allowed to send input commands.");
     }
 
-    if (uncacheBuffer.isValid()) {
-        sp<GraphicBuffer> buffer = ClientCache::getInstance().erase(uncacheBuffer);
-        if (buffer != nullptr) {
-            mBufferIdsToUncache.push_back(buffer->getId());
-        }
+    for (uint64_t uncacheBufferId : uncacheBufferIds) {
+        mBufferIdsToUncache.push_back(uncacheBufferId);
     }
 
     // If a synchronous transaction is explicitly requested without any changes, force a transaction
-- 
cgit v1.2.3-59-g8ed1b