am 41422980: am d2c12e4c: am 7637e35d: Merge "Revert "libgui: Change BufferQueue to use free lists""

* commit '41422980fabb087390791d951c28fef072d93be1':
  Revert "libgui: Change BufferQueue to use free lists"

4 files changed, 39 insertions, 128 deletions

diff --git a/include/gui/BufferQueueCore.h b/include/gui/BufferQueueCore.h
index a6065a9..40026bd 100644
--- a/include/gui/BufferQueueCore.h
+++ b/include/gui/BufferQueueCore.h
@@ -30,9 +30,6 @@
 #include <utils/Trace.h>
 #include <utils/Vector.h>
 
-#include <list>
-#include <set>
-
 #define BQ_LOGV(x, ...) ALOGV("[%s] " x, mConsumerName.string(), ##__VA_ARGS__)
 #define BQ_LOGD(x, ...) ALOGD("[%s] " x, mConsumerName.string(), ##__VA_ARGS__)
 #define BQ_LOGI(x, ...) ALOGI("[%s] " x, mConsumerName.string(), ##__VA_ARGS__)
@@ -126,10 +123,6 @@ private:
     // waitWhileAllocatingLocked blocks until mIsAllocating is false.
     void waitWhileAllocatingLocked() const;
 
-    // validateConsistencyLocked ensures that the free lists are in sync with
-    // the information stored in mSlots
-    void validateConsistencyLocked() const;
-
     // mAllocator is the connection to SurfaceFlinger that is used to allocate
     // new GraphicBuffer objects.
     sp<IGraphicBufferAlloc> mAllocator;
@@ -184,14 +177,6 @@ private:
     // mQueue is a FIFO of queued buffers used in synchronous mode.
     Fifo mQueue;
 
-    // mFreeSlots contains all of the slots which are FREE and do not currently
-    // have a buffer attached
-    std::set<int> mFreeSlots;
-
-    // mFreeBuffers contains all of the slots which are FREE and currently have
-    // a buffer attached
-    std::list<int> mFreeBuffers;
-
     // mOverrideMaxBufferCount is the limit on the number of buffers that will
     // be allocated at one time. This value is set by the producer by calling
     // setBufferCount. The default is 0, which means that the producer doesn't
diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp
index c7d5e00..526c3b7 100644
--- a/libs/gui/BufferQueueConsumer.cpp
+++ b/libs/gui/BufferQueueConsumer.cpp
@@ -120,7 +120,6 @@ 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();
@@ -174,8 +173,6 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer,
 
     ATRACE_INT(mCore->mConsumerName.string(), mCore->mQueue.size());
 
-    mCore->validateConsistencyLocked();
-
     return NO_ERROR;
 }
 
@@ -202,7 +199,6 @@ status_t BufferQueueConsumer::detachBuffer(int slot) {
 
     mCore->freeBufferLocked(slot);
     mCore->mDequeueCondition.broadcast();
-    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -221,11 +217,18 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot,
 
     Mutex::Autolock lock(mCore->mMutex);
 
-    // Make sure we don't have too many acquired buffers
+    // 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).
     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;
+            }
         }
     }
 
@@ -235,17 +238,6 @@ 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;
@@ -279,8 +271,6 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot,
     // for attached buffers.
     mSlots[*outSlot].mAcquireCalled = false;
 
-    mCore->validateConsistencyLocked();
-
     return NO_ERROR;
 }
 
@@ -321,7 +311,6 @@ 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) {
@@ -336,7 +325,6 @@ 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 29415c9..edebc45 100644
--- a/libs/gui/BufferQueueCore.cpp
+++ b/libs/gui/BufferQueueCore.cpp
@@ -53,8 +53,6 @@ BufferQueueCore::BufferQueueCore(const sp<IGraphicBufferAlloc>& allocator) :
     mConnectedProducerListener(),
     mSlots(),
     mQueue(),
-    mFreeSlots(),
-    mFreeBuffers(),
     mOverrideMaxBufferCount(0),
     mDequeueCondition(),
     mUseAsyncBuffer(true),
@@ -78,9 +76,6 @@ 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() {}
@@ -195,20 +190,12 @@ 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
@@ -217,7 +204,6 @@ void BufferQueueCore::freeBufferLocked(int slot) {
         mSlots[slot].mEglFence = EGL_NO_SYNC_KHR;
     }
     mSlots[slot].mFence = Fence::NO_FENCE;
-    validateConsistencyLocked();
 }
 
 void BufferQueueCore::freeAllBuffersLocked() {
@@ -250,48 +236,4 @@ 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 b54641e..6452cdd 100644
--- a/libs/gui/BufferQueueProducer.cpp
+++ b/libs/gui/BufferQueueProducer.cpp
@@ -161,6 +161,8 @@ 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) {
@@ -171,6 +173,15 @@ 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;
             }
@@ -203,8 +214,6 @@ 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
@@ -214,16 +223,6 @@ 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();
-                *found = *slot;
-                mCore->mFreeSlots.erase(slot);
-            }
         }
 
         // If no buffer is found, or if the queue has too many buffers
@@ -336,8 +335,6 @@ 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) {
@@ -358,6 +355,7 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot,
                 return NO_INIT;
             }
 
+            mSlots[*outSlot].mFrameNumber = UINT32_MAX;
             mSlots[*outSlot].mGraphicBuffer = graphicBuffer;
         } // Autolock scope
     }
@@ -416,7 +414,6 @@ status_t BufferQueueProducer::detachBuffer(int slot) {
 
     mCore->freeBufferLocked(slot);
     mCore->mDequeueCondition.broadcast();
-    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -441,19 +438,27 @@ status_t BufferQueueProducer::detachNextBuffer(sp<GraphicBuffer>* outBuffer,
         return NO_INIT;
     }
 
-    if (mCore->mFreeBuffers.empty()) {
-        return NO_MEMORY;
+    // 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;
+            }
+        }
     }
 
-    int found = mCore->mFreeBuffers.front();
-    mCore->mFreeBuffers.remove(found);
+    if (found == BufferQueueCore::INVALID_BUFFER_SLOT) {
+        return NO_MEMORY;
+    }
 
     BQ_LOGV("detachNextBuffer detached slot %d", found);
 
     *outBuffer = mSlots[found].mGraphicBuffer;
     *outFence = mSlots[found].mFence;
     mCore->freeBufferLocked(found);
-    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -501,8 +506,6 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot,
     mSlots[*outSlot].mFence = Fence::NO_FENCE;
     mSlots[*outSlot].mRequestBufferCalled = true;
 
-    mCore->validateConsistencyLocked();
-
     return returnFlags;
 }
 
@@ -637,7 +640,9 @@ status_t BufferQueueProducer::queueBuffer(int slot,
                 // mark it as freed
                 if (mCore->stillTracking(front)) {
                     mSlots[front->mSlot].mBufferState = BufferSlot::FREE;
-                    mCore->mFreeBuffers.push_front(front->mSlot);
+                    // 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;
                 }
                 // Overwrite the droppable buffer with the incoming one
                 *front = item;
@@ -659,8 +664,6 @@ status_t BufferQueueProducer::queueBuffer(int slot,
 
         // Take a ticket for the callback functions
         callbackTicket = mNextCallbackTicket++;
-
-        mCore->validateConsistencyLocked();
     } // Autolock scope
 
     // Wait without lock held
@@ -721,11 +724,10 @@ 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) {
@@ -1007,19 +1009,13 @@ 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
     }
 }