diff --git a/include/gui/BufferQueueCore.h b/include/gui/BufferQueueCore.h index 40026bda3..a6065a9e9 100644 --- a/include/gui/BufferQueueCore.h +++ b/include/gui/BufferQueueCore.h @@ -30,6 +30,9 @@ #include #include +#include +#include + #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 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 mFreeSlots; + + // mFreeBuffers contains all of the slots which are FREE and currently have + // a buffer attached + std::list 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 526c3b729..c7d5e0032 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 edebc4553..29415c94c 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -53,6 +53,8 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : mConnectedProducerListener(), mSlots(), mQueue(), + mFreeSlots(), + mFreeBuffers(), mOverrideMaxBufferCount(0), mDequeueCondition(), mUseAsyncBuffer(true), @@ -76,6 +78,9 @@ BufferQueueCore::BufferQueueCore(const sp& 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 6452cddcf..b54641e98 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,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* 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 +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 - // 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 +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) { 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 } }