From 1df8c345854155cbbcb9f80de9d12d66ea70ac08 Mon Sep 17 00:00:00 2001 From: Jamie Gennis Date: Thu, 20 Dec 2012 14:05:45 -0800 Subject: [PATCH 1/2] libgui: disallow NULL Fence pointers This change eliminates the uses of a NULL sp indicating that no waiting is required. Instead we use a non-NULL but invalid Fence object for which the wait methods will return immediately. Bug: 7892871 Change-Id: I5360aebe3090422ef6920d56c99fc4eedc642e48 --- libs/gui/BufferItemConsumer.cpp | 2 +- libs/gui/BufferQueue.cpp | 23 +++++---- libs/gui/ConsumerBase.cpp | 4 +- libs/gui/GLConsumer.cpp | 3 +- libs/gui/IGraphicBufferProducer.cpp | 50 ++++++------------- libs/gui/SurfaceTextureClient.cpp | 6 +-- libs/gui/tests/BufferQueue_test.cpp | 2 +- libs/gui/tests/SurfaceTexture_test.cpp | 2 - libs/ui/Fence.cpp | 34 +++++++++---- .../DisplayHardware/HWComposer.cpp | 8 ++- services/surfaceflinger/Layer.cpp | 2 +- .../surfaceflinger/SurfaceFlingerConsumer.cpp | 1 + 12 files changed, 67 insertions(+), 70 deletions(-) diff --git a/libs/gui/BufferItemConsumer.cpp b/libs/gui/BufferItemConsumer.cpp index 50798836c..885b4e497 100644 --- a/libs/gui/BufferItemConsumer.cpp +++ b/libs/gui/BufferItemConsumer.cpp @@ -62,7 +62,7 @@ status_t BufferItemConsumer::acquireBuffer(BufferItem *item, bool waitForFence) return err; } - if (waitForFence && item->mFence.get()) { + if (waitForFence) { err = item->mFence->waitForever(1000, "BufferItemConsumer::acquireBuffer"); if (err != OK) { BI_LOGE("Failed to wait for fence of acquired buffer: %s (%d)", diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp index 4be655b3f..d93c06717 100644 --- a/libs/gui/BufferQueue.cpp +++ b/libs/gui/BufferQueue.cpp @@ -372,8 +372,6 @@ status_t BufferQueue::dequeueBuffer(int *outBuf, sp& outFence, h = mDefaultHeight; } - // buffer is now in DEQUEUED (but can also be current at the same time, - // if we're in synchronous mode) mSlots[buf].mBufferState = BufferSlot::DEQUEUED; const sp& buffer(mSlots[buf].mGraphicBuffer); @@ -387,7 +385,7 @@ status_t BufferQueue::dequeueBuffer(int *outBuf, sp& outFence, mSlots[buf].mGraphicBuffer = NULL; mSlots[buf].mRequestBufferCalled = false; mSlots[buf].mEglFence = EGL_NO_SYNC_KHR; - mSlots[buf].mFence.clear(); + mSlots[buf].mFence = Fence::NO_FENCE; mSlots[buf].mEglDisplay = EGL_NO_DISPLAY; returnFlags |= IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION; @@ -397,7 +395,7 @@ status_t BufferQueue::dequeueBuffer(int *outBuf, sp& outFence, eglFence = mSlots[buf].mEglFence; outFence = mSlots[buf].mFence; mSlots[buf].mEglFence = EGL_NO_SYNC_KHR; - mSlots[buf].mFence.clear(); + mSlots[buf].mFence = Fence::NO_FENCE; } // end lock scope if (returnFlags & IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) { @@ -423,7 +421,6 @@ status_t BufferQueue::dequeueBuffer(int *outBuf, sp& outFence, } } - if (eglFence != EGL_NO_SYNC_KHR) { EGLint result = eglClientWaitSyncKHR(dpy, eglFence, 0, 1000000000); // If something goes wrong, log the error, but return the buffer without @@ -488,6 +485,11 @@ status_t BufferQueue::queueBuffer(int buf, input.deflate(×tamp, &crop, &scalingMode, &transform, &fence); + if (fence == NULL) { + ST_LOGE("queueBuffer: fence is NULL"); + return BAD_VALUE; + } + ST_LOGV("queueBuffer: slot=%d time=%#llx crop=[%d,%d,%d,%d] tr=%#x " "scale=%s", buf, timestamp, crop.left, crop.top, crop.right, crop.bottom, @@ -607,6 +609,9 @@ void BufferQueue::cancelBuffer(int buf, sp fence) { ST_LOGE("cancelBuffer: slot %d is not owned by the client (state=%d)", buf, mSlots[buf].mBufferState); return; + } else if (fence == NULL) { + ST_LOGE("cancelBuffer: fence is NULL"); + return; } mSlots[buf].mBufferState = BufferSlot::FREE; mSlots[buf].mFrameNumber = 0; @@ -785,7 +790,7 @@ void BufferQueue::freeBufferLocked(int slot) { eglDestroySyncKHR(mSlots[slot].mEglDisplay, mSlots[slot].mEglFence); mSlots[slot].mEglFence = EGL_NO_SYNC_KHR; } - mSlots[slot].mFence.clear(); + mSlots[slot].mFence = Fence::NO_FENCE; } void BufferQueue::freeAllBuffersLocked() { @@ -843,7 +848,7 @@ status_t BufferQueue::acquireBuffer(BufferItem *buffer) { mSlots[buf].mAcquireCalled = true; mSlots[buf].mNeedsCleanupOnRelease = false; mSlots[buf].mBufferState = BufferSlot::ACQUIRED; - mSlots[buf].mFence.clear(); + mSlots[buf].mFence = Fence::NO_FENCE; mQueue.erase(front); mDequeueCondition.broadcast(); @@ -863,8 +868,8 @@ status_t BufferQueue::releaseBuffer(int buf, EGLDisplay display, Mutex::Autolock _l(mMutex); - if (buf == INVALID_BUFFER_SLOT) { - return -EINVAL; + if (buf == INVALID_BUFFER_SLOT || fence == NULL) { + return BAD_VALUE; } mSlots[buf].mEglDisplay = display; diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp index 8f391aa49..fb6ba7dbf 100644 --- a/libs/gui/ConsumerBase.cpp +++ b/libs/gui/ConsumerBase.cpp @@ -84,7 +84,7 @@ ConsumerBase::~ConsumerBase() { void ConsumerBase::freeBufferLocked(int slotIndex) { CB_LOGV("freeBufferLocked: slotIndex=%d", slotIndex); mSlots[slotIndex].mGraphicBuffer = 0; - mSlots[slotIndex].mFence = 0; + mSlots[slotIndex].mFence = Fence::NO_FENCE; } // Used for refactoring, should not be in final interface @@ -228,7 +228,7 @@ status_t ConsumerBase::releaseBufferLocked(int slot, EGLDisplay display, freeBufferLocked(slot); } - mSlots[slot].mFence.clear(); + mSlots[slot].mFence = Fence::NO_FENCE; return err; } diff --git a/libs/gui/GLConsumer.cpp b/libs/gui/GLConsumer.cpp index ba23f9edf..09831fb8b 100644 --- a/libs/gui/GLConsumer.cpp +++ b/libs/gui/GLConsumer.cpp @@ -117,6 +117,7 @@ GLConsumer::GLConsumer(GLuint tex, bool allowSynchronousMode, GLenum texTarget, bool useFenceSync, const sp &bufferQueue) : ConsumerBase(bufferQueue == 0 ? new BufferQueue(allowSynchronousMode) : bufferQueue), mCurrentTransform(0), + mCurrentFence(Fence::NO_FENCE), mCurrentTimestamp(0), mFilteringEnabled(true), mTexName(tex), @@ -823,7 +824,7 @@ status_t GLConsumer::doGLFenceWaitLocked() const { return INVALID_OPERATION; } - if (mCurrentFence != NULL) { + if (mCurrentFence->isValid()) { if (useWaitSync) { // Create an EGLSyncKHR from the current fence. int fenceFd = mCurrentFence->dup(); diff --git a/libs/gui/IGraphicBufferProducer.cpp b/libs/gui/IGraphicBufferProducer.cpp index c94981703..54860d794 100644 --- a/libs/gui/IGraphicBufferProducer.cpp +++ b/libs/gui/IGraphicBufferProducer.cpp @@ -94,9 +94,11 @@ public: return result; } *buf = reply.readInt32(); - fence.clear(); - bool hasFence = reply.readInt32(); - if (hasFence) { + bool fenceWasWritten = reply.readInt32(); + if (fenceWasWritten) { + // If the fence was written by the callee, then overwrite the + // caller's fence here. If it wasn't written then don't touch the + // caller's fence. fence = new Fence(); reply.read(*fence.get()); } @@ -121,13 +123,9 @@ public: virtual void cancelBuffer(int buf, sp fence) { Parcel data, reply; - bool hasFence = fence.get() && fence->isValid(); data.writeInterfaceToken(IGraphicBufferProducer::getInterfaceDescriptor()); data.writeInt32(buf); - data.writeInt32(hasFence); - if (hasFence) { - data.write(*fence.get()); - } + data.write(*fence.get()); remote()->transact(CANCEL_BUFFER, data, &reply); } @@ -218,10 +216,9 @@ status_t BnGraphicBufferProducer::onTransact( int buf; sp fence; int result = dequeueBuffer(&buf, fence, w, h, format, usage); - bool hasFence = fence.get() && fence->isValid(); reply->writeInt32(buf); - reply->writeInt32(hasFence); - if (hasFence) { + reply->writeInt32(fence != NULL); + if (fence != NULL) { reply->write(*fence.get()); } reply->writeInt32(result); @@ -241,12 +238,8 @@ status_t BnGraphicBufferProducer::onTransact( case CANCEL_BUFFER: { CHECK_INTERFACE(IGraphicBufferProducer, data, reply); int buf = data.readInt32(); - sp fence; - bool hasFence = data.readInt32(); - if (hasFence) { - fence = new Fence(); - data.read(*fence.get()); - } + sp fence = new Fence(); + data.read(*fence.get()); cancelBuffer(buf, fence); return NO_ERROR; } break; @@ -289,10 +282,6 @@ status_t BnGraphicBufferProducer::onTransact( // ---------------------------------------------------------------------------- -static bool isValid(const sp& fence) { - return fence.get() && fence->isValid(); -} - IGraphicBufferProducer::QueueBufferInput::QueueBufferInput(const Parcel& parcel) { parcel.read(*this); } @@ -303,29 +292,24 @@ size_t IGraphicBufferProducer::QueueBufferInput::getFlattenedSize() const + sizeof(crop) + sizeof(scalingMode) + sizeof(transform) - + sizeof(bool) - + (isValid(fence) ? fence->getFlattenedSize() : 0); + + fence->getFlattenedSize(); } size_t IGraphicBufferProducer::QueueBufferInput::getFdCount() const { - return isValid(fence) ? fence->getFdCount() : 0; + return fence->getFdCount(); } status_t IGraphicBufferProducer::QueueBufferInput::flatten(void* buffer, size_t size, int fds[], size_t count) const { status_t err = NO_ERROR; - bool haveFence = isValid(fence); char* p = (char*)buffer; memcpy(p, ×tamp, sizeof(timestamp)); p += sizeof(timestamp); memcpy(p, &crop, sizeof(crop)); p += sizeof(crop); memcpy(p, &scalingMode, sizeof(scalingMode)); p += sizeof(scalingMode); memcpy(p, &transform, sizeof(transform)); p += sizeof(transform); - memcpy(p, &haveFence, sizeof(haveFence)); p += sizeof(haveFence); - if (haveFence) { - err = fence->flatten(p, size - (p - (char*)buffer), fds, count); - } + err = fence->flatten(p, size - (p - (char*)buffer), fds, count); return err; } @@ -333,17 +317,13 @@ status_t IGraphicBufferProducer::QueueBufferInput::unflatten(void const* buffer, size_t size, int fds[], size_t count) { status_t err = NO_ERROR; - bool haveFence; const char* p = (const char*)buffer; memcpy(×tamp, p, sizeof(timestamp)); p += sizeof(timestamp); memcpy(&crop, p, sizeof(crop)); p += sizeof(crop); memcpy(&scalingMode, p, sizeof(scalingMode)); p += sizeof(scalingMode); memcpy(&transform, p, sizeof(transform)); p += sizeof(transform); - memcpy(&haveFence, p, sizeof(haveFence)); p += sizeof(haveFence); - if (haveFence) { - fence = new Fence(); - err = fence->unflatten(p, size - (p - (const char*)buffer), fds, count); - } + fence = new Fence(); + err = fence->unflatten(p, size - (p - (const char*)buffer), fds, count); return err; } diff --git a/libs/gui/SurfaceTextureClient.cpp b/libs/gui/SurfaceTextureClient.cpp index c015b812f..5ed2e3815 100644 --- a/libs/gui/SurfaceTextureClient.cpp +++ b/libs/gui/SurfaceTextureClient.cpp @@ -216,7 +216,7 @@ int SurfaceTextureClient::dequeueBuffer(android_native_buffer_t** buffer, } } - if (fence.get()) { + if (fence->isValid()) { *fenceFd = fence->dup(); if (*fenceFd == -1) { ALOGE("dequeueBuffer: error duping fence: %d", errno); @@ -241,7 +241,7 @@ int SurfaceTextureClient::cancelBuffer(android_native_buffer_t* buffer, if (i < 0) { return i; } - sp fence(fenceFd >= 0 ? new Fence(fenceFd) : NULL); + sp fence(fenceFd >= 0 ? new Fence(fenceFd) : Fence::NO_FENCE); mSurfaceTexture->cancelBuffer(i, fence); return OK; } @@ -287,7 +287,7 @@ int SurfaceTextureClient::queueBuffer(android_native_buffer_t* buffer, int fence Rect crop; mCrop.intersect(Rect(buffer->width, buffer->height), &crop); - sp fence(fenceFd >= 0 ? new Fence(fenceFd) : NULL); + sp fence(fenceFd >= 0 ? new Fence(fenceFd) : Fence::NO_FENCE); IGraphicBufferProducer::QueueBufferOutput output; IGraphicBufferProducer::QueueBufferInput input(timestamp, crop, mScalingMode, mTransform, fence); diff --git a/libs/gui/tests/BufferQueue_test.cpp b/libs/gui/tests/BufferQueue_test.cpp index 12cbfb0d7..93f8fafc2 100644 --- a/libs/gui/tests/BufferQueue_test.cpp +++ b/libs/gui/tests/BufferQueue_test.cpp @@ -71,7 +71,7 @@ TEST_F(BufferQueueTest, AcquireBuffer_ExceedsMaxAcquireCount_Fails) { sp fence; sp buf; IGraphicBufferProducer::QueueBufferInput qbi(0, Rect(0, 0, 1, 1), - NATIVE_WINDOW_SCALING_MODE_FREEZE, 0, fence); + NATIVE_WINDOW_SCALING_MODE_FREEZE, 0, Fence::NO_FENCE); BufferQueue::BufferItem item; for (int i = 0; i < 2; i++) { diff --git a/libs/gui/tests/SurfaceTexture_test.cpp b/libs/gui/tests/SurfaceTexture_test.cpp index b6020ca2a..9062f84c4 100644 --- a/libs/gui/tests/SurfaceTexture_test.cpp +++ b/libs/gui/tests/SurfaceTexture_test.cpp @@ -202,7 +202,6 @@ protected: while ((err = glGetError()) != GL_NO_ERROR) { msg += String8::format(", %#x", err); } - fprintf(stderr, "pixel check failure: %s\n", msg.string()); return ::testing::AssertionFailure( ::testing::Message(msg.string())); } @@ -228,7 +227,6 @@ protected: msg += String8::format("a(%d isn't %d)", pixel[3], a); } if (!msg.isEmpty()) { - fprintf(stderr, "pixel check failure: %s\n", msg.string()); return ::testing::AssertionFailure( ::testing::Message(msg.string())); } else { diff --git a/libs/ui/Fence.cpp b/libs/ui/Fence.cpp index a01ac293a..b9e0f00fb 100644 --- a/libs/ui/Fence.cpp +++ b/libs/ui/Fence.cpp @@ -29,7 +29,7 @@ namespace android { -const sp Fence::NO_FENCE = sp(); +const sp Fence::NO_FENCE = sp(new Fence); Fence::Fence() : mFenceFd(-1) { @@ -71,7 +71,19 @@ status_t Fence::waitForever(unsigned int warningTimeout, const char* logname) { sp Fence::merge(const String8& name, const sp& f1, const sp& f2) { ATRACE_CALL(); - int result = sync_merge(name.string(), f1->mFenceFd, f2->mFenceFd); + int result; + // Merge the two fences. In the case where one of the fences is not a + // valid fence (e.g. NO_FENCE) we merge the one valid fence with itself so + // that a new fence with the given name is created. + if (f1->isValid() && f2->isValid()) { + result = sync_merge(name.string(), f1->mFenceFd, f2->mFenceFd); + } else if (f1->isValid()) { + result = sync_merge(name.string(), f1->mFenceFd, f1->mFenceFd); + } else if (f2->isValid()) { + result = sync_merge(name.string(), f2->mFenceFd, f2->mFenceFd); + } else { + return NO_FENCE; + } if (result == -1) { status_t err = -errno; ALOGE("merge: sync_merge(\"%s\", %d, %d) returned an error: %s (%d)", @@ -83,9 +95,6 @@ sp Fence::merge(const String8& name, const sp& f1, } int Fence::dup() const { - if (mFenceFd == -1) { - return -1; - } return ::dup(mFenceFd); } @@ -121,22 +130,24 @@ size_t Fence::getFlattenedSize() const { } size_t Fence::getFdCount() const { - return 1; + return isValid() ? 1 : 0; } status_t Fence::flatten(void* buffer, size_t size, int fds[], size_t count) const { - if (size != 0 || count != 1) { + if (size != getFlattenedSize() || count != getFdCount()) { return BAD_VALUE; } - fds[0] = mFenceFd; + if (isValid()) { + fds[0] = mFenceFd; + } return NO_ERROR; } status_t Fence::unflatten(void const* buffer, size_t size, int fds[], size_t count) { - if (size != 0 || count != 1) { + if (size != 0 || (count != 0 && count != 1)) { return BAD_VALUE; } if (mFenceFd != -1) { @@ -144,7 +155,10 @@ status_t Fence::unflatten(void const* buffer, size_t size, int fds[], return INVALID_OPERATION; } - mFenceFd = fds[0]; + if (count == 1) { + mFenceFd = fds[0]; + } + return NO_ERROR; } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index ead158ed8..7a24d4c24 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -539,7 +539,7 @@ status_t HWComposer::setFramebufferTarget(int32_t id, } int acquireFenceFd = -1; - if (acquireFence != NULL) { + if (acquireFence->isValid()) { acquireFenceFd = acquireFence->dup(); } @@ -659,7 +659,7 @@ status_t HWComposer::commit() { for (size_t i=0 ; iretireFenceFd != -1) { disp.lastRetireFence = new Fence(disp.list->retireFenceFd); @@ -725,9 +725,7 @@ int HWComposer::fbPost(int32_t id, if (mHwc && hwcHasApiVersion(mHwc, HWC_DEVICE_API_VERSION_1_1)) { return setFramebufferTarget(id, acquireFence, buffer); } else { - if (acquireFence != NULL) { - acquireFence->waitForever(1000, "HWComposer::fbPost"); - } + acquireFence->waitForever(1000, "HWComposer::fbPost"); return mFbDev->post(mFbDev, buffer->handle); } } diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 99af85736..1401154d3 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -298,7 +298,7 @@ void Layer::setAcquireFence(const sp& hw, if (layer.getCompositionType() == HWC_OVERLAY) { sp fence = mSurfaceFlingerConsumer->getCurrentFence(); - if (fence.get()) { + if (fence->isValid()) { fenceFd = fence->dup(); if (fenceFd == -1) { ALOGW("failed to dup layer fence, skipping sync: %d", errno); diff --git a/services/surfaceflinger/SurfaceFlingerConsumer.cpp b/services/surfaceflinger/SurfaceFlingerConsumer.cpp index dc9089e3d..e42707244 100644 --- a/services/surfaceflinger/SurfaceFlingerConsumer.cpp +++ b/services/surfaceflinger/SurfaceFlingerConsumer.cpp @@ -15,6 +15,7 @@ */ #define ATRACE_TAG ATRACE_TAG_GRAPHICS +//#define LOG_NDEBUG 0 #include "SurfaceFlingerConsumer.h" From c52e16cbf9798b95188c82465dfd84b914ff5199 Mon Sep 17 00:00:00 2001 From: Jamie Gennis Date: Tue, 8 Jan 2013 18:05:17 -0800 Subject: [PATCH 2/2] flatland: remove an unneeded #include Change-Id: I50831d4efd543664ff7df7aaef35d842aebf1bd6 --- cmds/flatland/GLHelper.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmds/flatland/GLHelper.cpp b/cmds/flatland/GLHelper.cpp index 31b1e063f..364789759 100644 --- a/cmds/flatland/GLHelper.cpp +++ b/cmds/flatland/GLHelper.cpp @@ -14,8 +14,6 @@ * limitations under the License. */ -#include - #include #include