libgui: Change BufferQueue to use free lists

BufferQueue used to choose free buffers by scanning through its array
of slots and picking one based on timestamp. This changes that
mechanism to use a pair of free lists: one with buffers attached and
one without. This makes it easier to choose either type of free slot
depending on the needs of the current operation.

Fixes an issue with the first version of this change, found in bugs
20482952, 20443314, and 20464549.

Bug: 13175420
Change-Id: I9b6e83cfe8f9b4329a976025cb8e291d51fb6d4a

3 files changed, 116 insertions, 39 deletions

diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp
index 526c3b7..c7d5e00 100644
--- a/libs/gui/BufferQueueConsumer.cpp
+++ b/libs/gui/BufferQueueConsumer.cpp
@@ -120,6 +120,7 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer,
             if (mCore->stillTracking(front)) {
                 // Front buffer is still in mSlots, so mark the slot as free
                 mSlots[front->mSlot].mBufferState = BufferSlot::FREE;
+                mCore->mFreeBuffers.push_back(front->mSlot);
             }
             mCore->mQueue.erase(front);
             front = mCore->mQueue.begin();
@@ -173,6 +174,8 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer,
 
     ATRACE_INT(mCore->mConsumerName.string(), mCore->mQueue.size());
 
+    mCore->validateConsistencyLocked();
+
     return NO_ERROR;
 }
 
@@ -199,6 +202,7 @@ status_t BufferQueueConsumer::detachBuffer(int slot) {
 
     mCore->freeBufferLocked(slot);
     mCore->mDequeueCondition.broadcast();
+    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -217,18 +221,11 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot,
 
     Mutex::Autolock lock(mCore->mMutex);
 
-    // Make sure we don't have too many acquired buffers and find a free slot
-    // to put the buffer into (the oldest if there are multiple).
+    // Make sure we don't have too many acquired buffers
     int numAcquiredBuffers = 0;
-    int found = BufferQueueCore::INVALID_BUFFER_SLOT;
     for (int s = 0; s < BufferQueueDefs::NUM_BUFFER_SLOTS; ++s) {
         if (mSlots[s].mBufferState == BufferSlot::ACQUIRED) {
             ++numAcquiredBuffers;
-        } else if (mSlots[s].mBufferState == BufferSlot::FREE) {
-            if (found == BufferQueueCore::INVALID_BUFFER_SLOT ||
-                    mSlots[s].mFrameNumber < mSlots[found].mFrameNumber) {
-                found = s;
-            }
         }
     }
 
@@ -238,6 +235,17 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot,
                 mCore->mMaxAcquiredBufferCount);
         return INVALID_OPERATION;
     }
+
+    // Find a free slot to put the buffer into
+    int found = BufferQueueCore::INVALID_BUFFER_SLOT;
+    if (!mCore->mFreeSlots.empty()) {
+        auto slot = mCore->mFreeSlots.begin();
+        found = *slot;
+        mCore->mFreeSlots.erase(slot);
+    } else if (!mCore->mFreeBuffers.empty()) {
+        found = mCore->mFreeBuffers.front();
+        mCore->mFreeBuffers.remove(found);
+    }
     if (found == BufferQueueCore::INVALID_BUFFER_SLOT) {
         BQ_LOGE("attachBuffer(P): could not find free buffer slot");
         return NO_MEMORY;
@@ -271,6 +279,8 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot,
     // for attached buffers.
     mSlots[*outSlot].mAcquireCalled = false;
 
+    mCore->validateConsistencyLocked();
+
     return NO_ERROR;
 }
 
@@ -311,6 +321,7 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber,
             mSlots[slot].mEglFence = eglFence;
             mSlots[slot].mFence = releaseFence;
             mSlots[slot].mBufferState = BufferSlot::FREE;
+            mCore->mFreeBuffers.push_back(slot);
             listener = mCore->mConnectedProducerListener;
             BQ_LOGV("releaseBuffer: releasing slot %d", slot);
         } else if (mSlots[slot].mNeedsCleanupOnRelease) {
@@ -325,6 +336,7 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber,
         }
 
         mCore->mDequeueCondition.broadcast();
+        mCore->validateConsistencyLocked();
     } // Autolock scope
 
     // Call back without lock held
diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp
index edebc45..29415c9 100644
--- a/libs/gui/BufferQueueCore.cpp
+++ b/libs/gui/BufferQueueCore.cpp
@@ -53,6 +53,8 @@ BufferQueueCore::BufferQueueCore(const sp<IGraphicBufferAlloc>& allocator) :
     mConnectedProducerListener(),
     mSlots(),
     mQueue(),
+    mFreeSlots(),
+    mFreeBuffers(),
     mOverrideMaxBufferCount(0),
     mDequeueCondition(),
     mUseAsyncBuffer(true),
@@ -76,6 +78,9 @@ BufferQueueCore::BufferQueueCore(const sp<IGraphicBufferAlloc>& allocator) :
             BQ_LOGE("createGraphicBufferAlloc failed");
         }
     }
+    for (int slot = 0; slot < BufferQueueDefs::NUM_BUFFER_SLOTS; ++slot) {
+        mFreeSlots.insert(slot);
+    }
 }
 
 BufferQueueCore::~BufferQueueCore() {}
@@ -190,12 +195,20 @@ status_t BufferQueueCore::setDefaultMaxBufferCountLocked(int count) {
 
 void BufferQueueCore::freeBufferLocked(int slot) {
     BQ_LOGV("freeBufferLocked: slot %d", slot);
+    bool hadBuffer = mSlots[slot].mGraphicBuffer != NULL;
     mSlots[slot].mGraphicBuffer.clear();
     if (mSlots[slot].mBufferState == BufferSlot::ACQUIRED) {
         mSlots[slot].mNeedsCleanupOnRelease = true;
     }
+    if (mSlots[slot].mBufferState != BufferSlot::FREE) {
+        mFreeSlots.insert(slot);
+    } else if (hadBuffer) {
+        // If the slot was FREE, but we had a buffer, we need to move this slot
+        // from the free buffers list to the the free slots list
+        mFreeBuffers.remove(slot);
+        mFreeSlots.insert(slot);
+    }
     mSlots[slot].mBufferState = BufferSlot::FREE;
-    mSlots[slot].mFrameNumber = UINT32_MAX;
     mSlots[slot].mAcquireCalled = false;
 
     // Destroy fence as BufferQueue now takes ownership
@@ -204,6 +217,7 @@ void BufferQueueCore::freeBufferLocked(int slot) {
         mSlots[slot].mEglFence = EGL_NO_SYNC_KHR;
     }
     mSlots[slot].mFence = Fence::NO_FENCE;
+    validateConsistencyLocked();
 }
 
 void BufferQueueCore::freeAllBuffersLocked() {
@@ -236,4 +250,48 @@ void BufferQueueCore::waitWhileAllocatingLocked() const {
     }
 }
 
+void BufferQueueCore::validateConsistencyLocked() const {
+    static const useconds_t PAUSE_TIME = 0;
+    for (int slot = 0; slot < BufferQueueDefs::NUM_BUFFER_SLOTS; ++slot) {
+        bool isInFreeSlots = mFreeSlots.count(slot) != 0;
+        bool isInFreeBuffers =
+                std::find(mFreeBuffers.cbegin(), mFreeBuffers.cend(), slot) !=
+                mFreeBuffers.cend();
+        if (mSlots[slot].mBufferState == BufferSlot::FREE) {
+            if (mSlots[slot].mGraphicBuffer == NULL) {
+                if (!isInFreeSlots) {
+                    BQ_LOGE("Slot %d is FREE but is not in mFreeSlots", slot);
+                    usleep(PAUSE_TIME);
+                }
+                if (isInFreeBuffers) {
+                    BQ_LOGE("Slot %d is in mFreeSlots "
+                            "but is also in mFreeBuffers", slot);
+                    usleep(PAUSE_TIME);
+                }
+            } else {
+                if (!isInFreeBuffers) {
+                    BQ_LOGE("Slot %d is FREE but is not in mFreeBuffers", slot);
+                    usleep(PAUSE_TIME);
+                }
+                if (isInFreeSlots) {
+                    BQ_LOGE("Slot %d is in mFreeBuffers "
+                            "but is also in mFreeSlots", slot);
+                    usleep(PAUSE_TIME);
+                }
+            }
+        } else {
+            if (isInFreeSlots) {
+                BQ_LOGE("Slot %d is in mFreeSlots but is not FREE (%d)",
+                        slot, mSlots[slot].mBufferState);
+                usleep(PAUSE_TIME);
+            }
+            if (isInFreeBuffers) {
+                BQ_LOGE("Slot %d is in mFreeBuffers but is not FREE (%d)",
+                        slot, mSlots[slot].mBufferState);
+                usleep(PAUSE_TIME);
+            }
+        }
+    }
+}
+
 } // namespace android
diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp
index 6452cdd..a27d5f0 100644
--- a/libs/gui/BufferQueueProducer.cpp
+++ b/libs/gui/BufferQueueProducer.cpp
@@ -161,8 +161,6 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
             }
         }
 
-        // Look for a free buffer to give to the client
-        *found = BufferQueueCore::INVALID_BUFFER_SLOT;
         int dequeuedCount = 0;
         int acquiredCount = 0;
         for (int s = 0; s < maxBufferCount; ++s) {
@@ -173,15 +171,6 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
                 case BufferSlot::ACQUIRED:
                     ++acquiredCount;
                     break;
-                case BufferSlot::FREE:
-                    // We return the oldest of the free buffers to avoid
-                    // stalling the producer if possible, since the consumer
-                    // may still have pending reads of in-flight buffers
-                    if (*found == BufferQueueCore::INVALID_BUFFER_SLOT ||
-                            mSlots[s].mFrameNumber < mSlots[*found].mFrameNumber) {
-                        *found = s;
-                    }
-                    break;
                 default:
                     break;
             }
@@ -214,6 +203,8 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
             }
         }
 
+        *found = BufferQueueCore::INVALID_BUFFER_SLOT;
+
         // If we disconnect and reconnect quickly, we can be in a state where
         // our slots are empty but we have many buffers in the queue. This can
         // cause us to run out of memory if we outrun the consumer. Wait here if
@@ -223,6 +214,19 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
         if (tooManyBuffers) {
             BQ_LOGV("%s: queue size is %zu, waiting", caller,
                     mCore->mQueue.size());
+        } else {
+            if (!mCore->mFreeBuffers.empty()) {
+                auto slot = mCore->mFreeBuffers.begin();
+                *found = *slot;
+                mCore->mFreeBuffers.erase(slot);
+            } else if (!mCore->mFreeSlots.empty()) {
+                auto slot = mCore->mFreeSlots.begin();
+                // Only return free slots up to the max buffer count
+                if (*slot < maxBufferCount) {
+                    *found = *slot;
+                    mCore->mFreeSlots.erase(slot);
+                }
+            }
         }
 
         // If no buffer is found, or if the queue has too many buffers
@@ -335,6 +339,8 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot,
         *outFence = mSlots[found].mFence;
         mSlots[found].mEglFence = EGL_NO_SYNC_KHR;
         mSlots[found].mFence = Fence::NO_FENCE;
+
+        mCore->validateConsistencyLocked();
     } // Autolock scope
 
     if (returnFlags & BUFFER_NEEDS_REALLOCATION) {
@@ -355,7 +361,6 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot,
                 return NO_INIT;
             }
 
-            mSlots[*outSlot].mFrameNumber = UINT32_MAX;
             mSlots[*outSlot].mGraphicBuffer = graphicBuffer;
         } // Autolock scope
     }
@@ -414,6 +419,7 @@ status_t BufferQueueProducer::detachBuffer(int slot) {
 
     mCore->freeBufferLocked(slot);
     mCore->mDequeueCondition.broadcast();
+    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -438,27 +444,19 @@ status_t BufferQueueProducer::detachNextBuffer(sp<GraphicBuffer>* outBuffer,
         return NO_INIT;
     }
 
-    // Find the oldest valid slot
-    int found = BufferQueueCore::INVALID_BUFFER_SLOT;
-    for (int s = 0; s < BufferQueueDefs::NUM_BUFFER_SLOTS; ++s) {
-        if (mSlots[s].mBufferState == BufferSlot::FREE &&
-                mSlots[s].mGraphicBuffer != NULL) {
-            if (found == BufferQueueCore::INVALID_BUFFER_SLOT ||
-                    mSlots[s].mFrameNumber < mSlots[found].mFrameNumber) {
-                found = s;
-            }
-        }
-    }
-
-    if (found == BufferQueueCore::INVALID_BUFFER_SLOT) {
+    if (mCore->mFreeBuffers.empty()) {
         return NO_MEMORY;
     }
 
+    int found = mCore->mFreeBuffers.front();
+    mCore->mFreeBuffers.remove(found);
+
     BQ_LOGV("detachNextBuffer detached slot %d", found);
 
     *outBuffer = mSlots[found].mGraphicBuffer;
     *outFence = mSlots[found].mFence;
     mCore->freeBufferLocked(found);
+    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -506,6 +504,8 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot,
     mSlots[*outSlot].mFence = Fence::NO_FENCE;
     mSlots[*outSlot].mRequestBufferCalled = true;
 
+    mCore->validateConsistencyLocked();
+
     return returnFlags;
 }
 
@@ -640,9 +640,7 @@ status_t BufferQueueProducer::queueBuffer(int slot,
                 // mark it as freed
                 if (mCore->stillTracking(front)) {
                     mSlots[front->mSlot].mBufferState = BufferSlot::FREE;
-                    // Reset the frame number of the freed buffer so that it is
-                    // the first in line to be dequeued again
-                    mSlots[front->mSlot].mFrameNumber = 0;
+                    mCore->mFreeBuffers.push_front(front->mSlot);
                 }
                 // Overwrite the droppable buffer with the incoming one
                 *front = item;
@@ -664,6 +662,8 @@ status_t BufferQueueProducer::queueBuffer(int slot,
 
         // Take a ticket for the callback functions
         callbackTicket = mNextCallbackTicket++;
+
+        mCore->validateConsistencyLocked();
     } // Autolock scope
 
     // Wait without lock held
@@ -724,10 +724,11 @@ void BufferQueueProducer::cancelBuffer(int slot, const sp<Fence>& fence) {
         return;
     }
 
+    mCore->mFreeBuffers.push_front(slot);
     mSlots[slot].mBufferState = BufferSlot::FREE;
-    mSlots[slot].mFrameNumber = 0;
     mSlots[slot].mFence = fence;
     mCore->mDequeueCondition.broadcast();
+    mCore->validateConsistencyLocked();
 }
 
 int BufferQueueProducer::query(int what, int *outValue) {
@@ -1009,13 +1010,19 @@ void BufferQueueProducer::allocateBuffers(bool async, uint32_t width,
                 }
                 mCore->freeBufferLocked(slot); // Clean up the slot first
                 mSlots[slot].mGraphicBuffer = buffers[i];
-                mSlots[slot].mFrameNumber = 0;
                 mSlots[slot].mFence = Fence::NO_FENCE;
+
+                // freeBufferLocked puts this slot on the free slots list. Since
+                // we then attached a buffer, move the slot to free buffer list.
+                mCore->mFreeSlots.erase(slot);
+                mCore->mFreeBuffers.push_front(slot);
+
                 BQ_LOGV("allocateBuffers: allocated a new buffer in slot %d", slot);
             }
 
             mCore->mIsAllocating = false;
             mCore->mIsAllocatingCondition.broadcast();
+            mCore->validateConsistencyLocked();
         } // Autolock scope
     }
 }