Merge "libgui: Change BufferQueue to use free lists"

include/gui/BufferQueueCore.h

@@ -30,6 +30,9 @@
 #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__)
@@ -123,6 +126,10 @@ 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;
@@ -177,6 +184,14 @@ 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

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
+    // Make sure we don't have too many acquired buffers
-    // 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;
-            }
         }
     }
 
@@ -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

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

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,16 @@ 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
@@ -335,6 +336,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 +358,6 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot,
                 return NO_INIT;
             }
 
-            mSlots[*outSlot].mFrameNumber = UINT32_MAX;
             mSlots[*outSlot].mGraphicBuffer = graphicBuffer;
         } // Autolock scope
     }
@@ -414,6 +416,7 @@ status_t BufferQueueProducer::detachBuffer(int slot) {
 
     mCore->freeBufferLocked(slot);
     mCore->mDequeueCondition.broadcast();
+    mCore->validateConsistencyLocked();
 
     return NO_ERROR;
 }
@@ -438,27 +441,19 @@ status_t BufferQueueProducer::detachNextBuffer(sp<GraphicBuffer>* outBuffer,
         return NO_INIT;
     }
 
-    // Find the oldest valid slot
+    if (mCore->mFreeBuffers.empty()) {
-    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) {
         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 +501,8 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot,
     mSlots[*outSlot].mFence = Fence::NO_FENCE;
     mSlots[*outSlot].mRequestBufferCalled = true;
 
+    mCore->validateConsistencyLocked();
+
     return returnFlags;
 }
 
@@ -640,9 +637,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
+                    mCore->mFreeBuffers.push_front(front->mSlot);
-                    // the first in line to be dequeued again
-                    mSlots[front->mSlot].mFrameNumber = 0;
                 }
                 // Overwrite the droppable buffer with the incoming one
                 *front = item;
@@ -664,6 +659,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 +721,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 +1007,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
     }
 }