OSDN Git Service

Release virtual display buffer immediately after HWC set
authorJesse Hall <jessehall@google.com>
Wed, 20 Mar 2013 00:18:09 +0000 (17:18 -0700)
committerJesse Hall <jessehall@google.com>
Wed, 20 Mar 2013 18:16:55 +0000 (11:16 -0700)
Previously we only queued a virtual display buffer to the sink when
the next frame was about to be displayed. This may delay the "last"
frame of an animation indefinitely. Now we queue the buffer as soon as
HWC set() returns and gives us the release fence.

Bug: 8384764
Change-Id: I3844a188e0f6ef6ff28f3e11477cfa063a924b1a

services/surfaceflinger/DisplayDevice.cpp
services/surfaceflinger/DisplayHardware/BufferQueueInterposer.cpp
services/surfaceflinger/DisplayHardware/BufferQueueInterposer.h
services/surfaceflinger/DisplayHardware/DisplaySurface.h
services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
services/surfaceflinger/DisplayHardware/FramebufferSurface.h
services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h

index ecd12d0..5493e7d 100644 (file)
@@ -236,7 +236,7 @@ void DisplayDevice::swapBuffers(HWComposer& hwc) const {
 void DisplayDevice::onSwapBuffersCompleted(HWComposer& hwc) const {
     if (hwc.initCheck() == NO_ERROR) {
         int fd = hwc.getAndResetReleaseFenceFd(mType);
-        mDisplaySurface->setReleaseFenceFd(fd);
+        mDisplaySurface->onFrameCommitted(fd);
     }
 }
 
index d8ad224..91f9aea 100644 (file)
@@ -16,6 +16,7 @@
 
 #undef LOG_TAG
 #define LOG_TAG "BQInterposer"
+//#define LOG_NDEBUG 0
 
 #include "BufferQueueInterposer.h"
 
@@ -42,19 +43,6 @@ BufferQueueInterposer::BufferQueueInterposer(
     mAcquired(false)
 {
     BQI_LOGV("BufferQueueInterposer sink=%p", sink.get());
-
-    // We need one additional dequeued buffer beyond what the source needs.
-    // To have more than one (the default), we must call setBufferCount. But
-    // we have no way of knowing what the sink has set as the minimum buffer
-    // count, so if we just call setBufferCount(3) it may fail (and does, on
-    // one device using a video encoder sink). So far on the devices we care
-    // about, this is the smallest value that works.
-    //
-    // TODO: Change IGraphicBufferProducer and implementations to support this.
-    // Maybe change it so both the consumer and producer declare how many
-    // buffers they need, and the IGBP adds them? Then BQInterposer would just
-    // add 1 to the source's buffer count.
-    mSink->setBufferCount(6);
 }
 
 BufferQueueInterposer::~BufferQueueInterposer() {
index 7208630..7e84e97 100644 (file)
@@ -59,20 +59,20 @@ namespace android {
 //   existing interfaces have some problems when the implementation isn't the
 //   final consumer.
 //
-// - The interposer needs at least one buffer in addition to those used by the
-//   source and sink. setBufferCount and QueueBufferOutput both need to
-//   account for this. It's not possible currently to do this generically,
-//   since we can't find out how many buffers the source and sink need. (See
-//   the horrible hack in the BufferQueueInterposer constructor).
+// - The client of the interposer may need one or more buffers in addition to
+//   those used by the source and sink. IGraphicBufferProducer will probably
+//   need to change to allow the producer to specify how many buffers it needs
+//   to dequeue at a time, and then the interposer can add its requirements to
+//   those of the source.
 //
 // - Abandoning, disconnecting, and connecting need to pass through somehow.
 //   There needs to be a way to tell the interposer client to release its
 //   buffer immediately so it can be queued/released, e.g. when the source
 //   calls disconnect().
 //
-// - Right now the source->BQI queue is synchronous even if the BQI->sink
-//   queue is asynchronous. Need to figure out how asynchronous should behave
-//   and implement that.
+// - Right now the source->BQI queue is synchronous even if the BQI->sink queue
+//   is asynchronous. Need to figure out how asynchronous should behave and
+//   implement that.
 
 class BufferQueueInterposer : public BnGraphicBufferProducer {
 public:
index 2de6b4c..6445848 100644 (file)
@@ -43,15 +43,16 @@ public:
     // this frame. Some implementations may only push a new buffer to
     // HWComposer if GLES composition took place, others need to push a new
     // buffer on every frame.
+    //
+    // advanceFrame must be followed by a call to  onFrameCommitted before
+    // advanceFrame may be called again.
     virtual status_t advanceFrame() = 0;
 
-    // setReleaseFenceFd stores a fence file descriptor that will signal when
-    // the current buffer is no longer being read. This fence will be returned
-    // to the producer when the current buffer is released by updateTexImage().
-    // Multiple fences can be set for a given buffer; they will be merged into
-    // a single union fence. The GLConsumer will close the file descriptor
-    // when finished with it.
-    virtual status_t setReleaseFenceFd(int fenceFd) = 0;
+    // onFrameCommitted is called after the frame has been committed to the
+    // hardware composer and a release fence is available for the buffer.
+    // Further operations on the buffer can be queued as long as they wait for
+    // the fence to signal.
+    virtual void onFrameCommitted(int fenceFd) = 0;
 
     virtual void dump(String8& result) const = 0;
 
index b5abaac..83ab38e 100644 (file)
@@ -140,17 +140,15 @@ void FramebufferSurface::freeBufferLocked(int slotIndex) {
     }
 }
 
-status_t FramebufferSurface::setReleaseFenceFd(int fenceFd) {
-    status_t err = NO_ERROR;
+void FramebufferSurface::onFrameCommitted(int fenceFd) {
     if (fenceFd >= 0) {
         sp<Fence> fence(new Fence(fenceFd));
         if (mCurrentBufferSlot != BufferQueue::INVALID_BUFFER_SLOT) {
-            err = addReleaseFence(mCurrentBufferSlot, fence);
+            status_t err = addReleaseFence(mCurrentBufferSlot, fence);
             ALOGE_IF(err, "setReleaseFenceFd: failed to add the fence: %s (%d)",
                     strerror(-err), err);
         }
     }
-    return err;
 }
 
 status_t FramebufferSurface::compositionComplete()
index 0aab742..1402740 100644 (file)
@@ -43,7 +43,7 @@ public:
 
     virtual status_t compositionComplete();
     virtual status_t advanceFrame();
-    virtual status_t setReleaseFenceFd(int fenceFd);
+    virtual void onFrameCommitted(int fenceFd);
 
     // Implementation of DisplaySurface::dump(). Note that ConsumerBase also
     // has a non-virtual dump() with the same signature.
index 433e1eb..fac6c3e 100644 (file)
@@ -26,16 +26,14 @@ VirtualDisplaySurface::VirtualDisplaySurface(HWComposer& hwc, int disp,
 :   mHwc(hwc),
     mDisplayId(disp),
     mSource(new BufferQueueInterposer(sink, name)),
-    mName(name),
-    mReleaseFence(Fence::NO_FENCE)
+    mName(name)
 {}
 
 VirtualDisplaySurface::~VirtualDisplaySurface() {
     if (mAcquiredBuffer != NULL) {
-        status_t result = mSource->releaseBuffer(mReleaseFence);
+        status_t result = mSource->releaseBuffer(Fence::NO_FENCE);
         ALOGE_IF(result != NO_ERROR, "VirtualDisplaySurface \"%s\": "
-                "failed to release previous buffer: %d",
-                mName.string(), result);
+                "failed to release buffer: %d", mName.string(), result);
     }
 }
 
@@ -52,12 +50,10 @@ status_t VirtualDisplaySurface::advanceFrame() {
     status_t result = NO_ERROR;
 
     if (mAcquiredBuffer != NULL) {
-        result = mSource->releaseBuffer(mReleaseFence);
-        ALOGE_IF(result != NO_ERROR, "VirtualDisplaySurface \"%s\": "
-                "failed to release previous buffer: %d",
-                mName.string(), result);
-        mAcquiredBuffer.clear();
-        mReleaseFence = Fence::NO_FENCE;
+        ALOGE("VirtualDisplaySurface \"%s\": "
+                "advanceFrame called twice without onFrameCommitted",
+                mName.string());
+        return INVALID_OPERATION;
     }
 
     sp<Fence> fence;
@@ -74,25 +70,15 @@ status_t VirtualDisplaySurface::advanceFrame() {
     return mHwc.fbPost(mDisplayId, fence, mAcquiredBuffer);
 }
 
-status_t VirtualDisplaySurface::setReleaseFenceFd(int fenceFd) {
-    if (fenceFd >= 0) {
-        sp<Fence> fence(new Fence(fenceFd));
-        Mutex::Autolock lock(mMutex);
-        sp<Fence> mergedFence = Fence::merge(
-                String8::format("VirtualDisplaySurface \"%s\"",
-                        mName.string()),
-                mReleaseFence, fence);
-        if (!mergedFence->isValid()) {
-            ALOGE("VirtualDisplaySurface \"%s\": failed to merge release fence",
-                    mName.string());
-            // synchronization is broken, the best we can do is hope fences
-            // signal in order so the new fence will act like a union
-            mReleaseFence = fence;
-            return BAD_VALUE;
-        }
-        mReleaseFence = mergedFence;
+void VirtualDisplaySurface::onFrameCommitted(int fenceFd) {
+    Mutex::Autolock lock(mMutex);
+    sp<Fence> fence(new Fence(fenceFd));
+    if (mAcquiredBuffer != NULL) {
+        status_t result = mSource->releaseBuffer(fence);
+        ALOGE_IF(result != NO_ERROR, "VirtualDisplaySurface \"%s\": "
+                "failed to release buffer: %d", mName.string(), result);
+        mAcquiredBuffer.clear();
     }
-    return NO_ERROR;
 }
 
 void VirtualDisplaySurface::dump(String8& result) const {
index 66f8580..85a7a87 100644 (file)
@@ -58,7 +58,7 @@ public:
 
     virtual status_t compositionComplete();
     virtual status_t advanceFrame();
-    virtual status_t setReleaseFenceFd(int fenceFd);
+    virtual void onFrameCommitted(int fenceFd);
     virtual void dump(String8& result) const;
 
 private:
@@ -73,7 +73,6 @@ private:
     // mutable, must be synchronized with mMutex
     Mutex mMutex;
     sp<GraphicBuffer> mAcquiredBuffer;
-    sp<Fence> mReleaseFence;
 };
 
 // ---------------------------------------------------------------------------