From 851cfe834295224cd64bdd499872b95b19c4de8c Mon Sep 17 00:00:00 2001 From: Jesse Hall Date: Wed, 20 Mar 2013 13:44:00 -0700 Subject: [PATCH 1/2] Isolate knowledge that fb target == output buffer HWComposer didn't allow the virtual display output buffer to be set directly, instead it always used the framebuffer target buffer. DisplayDevice was only providing the framebuffer release fence to DisplaySurfaces after a commit. This change fixes both of these, so both HWComposer and DisplayDevice should continue to work if VirtualDisplaySurface changes to use separate framebuffer and output buffers. It's also more correct since VirtualDisplaySurface uses the correct release fence when queueing the buffer to the sink. Bug: 8384764 Change-Id: I95c71e8d4f67705e23f122259ec8dd5dbce70dcf --- services/surfaceflinger/DisplayDevice.cpp | 3 +- .../DisplayHardware/DisplaySurface.h | 7 ++- .../DisplayHardware/FramebufferSurface.cpp | 3 +- .../DisplayHardware/FramebufferSurface.h | 2 +- .../DisplayHardware/HWComposer.cpp | 47 ++++++++++++------- .../DisplayHardware/HWComposer.h | 13 +++++ .../DisplayHardware/VirtualDisplaySurface.cpp | 14 +++++- .../DisplayHardware/VirtualDisplaySurface.h | 2 +- 8 files changed, 65 insertions(+), 26 deletions(-) diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp index e16abef3d..cdc16edb9 100644 --- a/services/surfaceflinger/DisplayDevice.cpp +++ b/services/surfaceflinger/DisplayDevice.cpp @@ -235,8 +235,7 @@ void DisplayDevice::swapBuffers(HWComposer& hwc) const { void DisplayDevice::onSwapBuffersCompleted(HWComposer& hwc) const { if (hwc.initCheck() == NO_ERROR) { - sp fence = hwc.getAndResetReleaseFence(mType); - mDisplaySurface->onFrameCommitted(fence); + mDisplaySurface->onFrameCommitted(); } } diff --git a/services/surfaceflinger/DisplayHardware/DisplaySurface.h b/services/surfaceflinger/DisplayHardware/DisplaySurface.h index bc717a956..2eca3cbfb 100644 --- a/services/surfaceflinger/DisplayHardware/DisplaySurface.h +++ b/services/surfaceflinger/DisplayHardware/DisplaySurface.h @@ -49,10 +49,9 @@ public: virtual status_t advanceFrame() = 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(const sp& fence) = 0; + // hardware composer. The surface collects the release fence for this + // frame's buffer. + virtual void onFrameCommitted() = 0; virtual void dump(String8& result) const = 0; diff --git a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp index c35ac956c..1936893c8 100644 --- a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp +++ b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp @@ -140,7 +140,8 @@ void FramebufferSurface::freeBufferLocked(int slotIndex) { } } -void FramebufferSurface::onFrameCommitted(const sp& fence) { +void FramebufferSurface::onFrameCommitted() { + sp fence = mHwc.getAndResetReleaseFence(mDisplayType); if (fence->isValid() && mCurrentBufferSlot != BufferQueue::INVALID_BUFFER_SLOT) { status_t err = addReleaseFence(mCurrentBufferSlot, fence); diff --git a/services/surfaceflinger/DisplayHardware/FramebufferSurface.h b/services/surfaceflinger/DisplayHardware/FramebufferSurface.h index 164f81f27..2fde789b4 100644 --- a/services/surfaceflinger/DisplayHardware/FramebufferSurface.h +++ b/services/surfaceflinger/DisplayHardware/FramebufferSurface.h @@ -43,7 +43,7 @@ public: virtual status_t compositionComplete(); virtual status_t advanceFrame(); - virtual void onFrameCommitted(const sp& fence); + virtual void onFrameCommitted(); // Implementation of DisplaySurface::dump(). Note that ConsumerBase also // has a non-virtual dump() with the same signature. diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index 9d3241093..497593bba 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -657,15 +657,12 @@ status_t HWComposer::commit() { mLists[0]->sur = eglGetCurrentSurface(EGL_DRAW); } - // For virtual displays, the framebufferTarget buffer also serves as - // the HWC output buffer, so we need to copy the buffer handle and - // dup() the acquire fence. for (size_t i=VIRTUAL_DISPLAY_ID_BASE; ioutbuf = disp.framebufferTarget->handle; + if (disp.outbufHandle) { + mLists[i]->outbuf = disp.outbufHandle; mLists[i]->outbufAcquireFenceFd = - dup(disp.framebufferTarget->acquireFenceFd); + disp.outbufAcquireFence->dup(); } } @@ -706,17 +703,15 @@ status_t HWComposer::acquire(int disp) { void HWComposer::disconnectDisplay(int disp) { LOG_ALWAYS_FATAL_IF(disp < 0 || disp == HWC_DISPLAY_PRIMARY); - if (disp >= HWC_NUM_DISPLAY_TYPES) { - // nothing to do for these yet - return; - } DisplayData& dd(mDisplayData[disp]); - if (dd.list != NULL) { - free(dd.list); - dd.list = NULL; - dd.framebufferTarget = NULL; // points into dd.list - dd.fbTargetHandle = NULL; - } + free(dd.list); + dd.list = NULL; + dd.framebufferTarget = NULL; // points into dd.list + dd.fbTargetHandle = NULL; + dd.outbufHandle = NULL; + dd.lastRetireFence = Fence::NO_FENCE; + dd.lastDisplayFence = Fence::NO_FENCE; + dd.outbufAcquireFence = Fence::NO_FENCE; } int HWComposer::getVisualID() const { @@ -765,6 +760,25 @@ void HWComposer::fbDump(String8& result) { } } +status_t HWComposer::setOutputBuffer(int32_t id, const sp& acquireFence, + const sp& buf) { + if (uint32_t(id)>31 || !mAllocatedDisplayIDs.hasBit(id)) + return BAD_INDEX; + if (id < VIRTUAL_DISPLAY_ID_BASE) + return INVALID_OPERATION; + + DisplayData& disp(mDisplayData[id]); + disp.outbufHandle = buf->handle; + disp.outbufAcquireFence = acquireFence; + return NO_ERROR; +} + +sp HWComposer::getLastRetireFence(int32_t id) { + if (uint32_t(id)>31 || !mAllocatedDisplayIDs.hasBit(id)) + return Fence::NO_FENCE; + return mDisplayData[id].lastRetireFence; +} + /* * Helper template to implement a concrete HWCLayer * This holds the pointer to the concrete hwc layer type @@ -1071,6 +1085,7 @@ HWComposer::DisplayData::DisplayData() capacity(0), list(NULL), framebufferTarget(NULL), fbTargetHandle(0), lastRetireFence(Fence::NO_FENCE), lastDisplayFence(Fence::NO_FENCE), + outbufHandle(NULL), outbufAcquireFence(Fence::NO_FENCE), events(0) {} diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index 346526aae..58f7e8fff 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer.h @@ -128,6 +128,17 @@ public: int fbCompositionComplete(); void fbDump(String8& result); + // Set the output buffer and acquire fence for a virtual display. + // Returns INVALID_OPERATION if id is not a virtual display. + status_t setOutputBuffer(int32_t id, const sp& acquireFence, + const sp& buf); + + // Get the retire fence for the last committed frame. This fence will + // signal when the h/w composer is completely finished with the frame. + // For physical displays, it is no longer being displayed. For virtual + // displays, writes to the output buffer are complete. + sp getLastRetireFence(int32_t id); + /* * Interface to hardware composer's layers functionality. * This abstracts the HAL interface to layers which can evolve in @@ -306,6 +317,8 @@ private: sp lastRetireFence; // signals when the last set op retires sp lastDisplayFence; // signals when the last set op takes // effect on screen + buffer_handle_t outbufHandle; + sp outbufAcquireFence; // protected by mEventControlLock int32_t events; diff --git a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp index 255b77f65..b28500e8f 100644 --- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp +++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp @@ -70,9 +70,21 @@ status_t VirtualDisplaySurface::advanceFrame() { return mHwc.fbPost(mDisplayId, fence, mAcquiredBuffer); } -void VirtualDisplaySurface::onFrameCommitted(const sp& fence) { +void VirtualDisplaySurface::onFrameCommitted() { Mutex::Autolock lock(mMutex); if (mAcquiredBuffer != NULL) { + // fbFence signals when reads from the framebuffer are finished + // outFence signals when writes to the output buffer are finished + // It's unlikely that there will be an implementation where fbFence + // signals after outFence (in fact they'll typically be the same + // sync_pt), but just to be pedantic we merge them so the sink will + // be sure to wait until both are complete. + sp fbFence = mHwc.getAndResetReleaseFence(mDisplayId); + sp outFence = mHwc.getLastRetireFence(mDisplayId); + sp fence = Fence::merge( + String8::format("HWC done: %.21s", mName.string()), + fbFence, outFence); + status_t result = mSource->releaseBuffer(fence); ALOGE_IF(result != NO_ERROR, "VirtualDisplaySurface \"%s\": " "failed to release buffer: %d", mName.string(), result); diff --git a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h index 13476800f..61bafedde 100644 --- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h +++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h @@ -58,7 +58,7 @@ public: virtual status_t compositionComplete(); virtual status_t advanceFrame(); - virtual void onFrameCommitted(const sp& fence); + virtual void onFrameCommitted(); virtual void dump(String8& result) const; private: From 48bc05b56df9919fc39c5f2e3ea6535560eec98f Mon Sep 17 00:00:00 2001 From: Jesse Hall Date: Thu, 21 Mar 2013 14:06:52 -0700 Subject: [PATCH 2/2] Fix dump when virtual display exists SurfaceFlinger::getLayerSortedByZForHwcDisplay only worked for builtin displays. Bug: 8384764 Change-Id: I989275407fb2f06d166a6e70321c3211e27e562e --- services/surfaceflinger/SurfaceFlinger.cpp | 16 ++++++++++++++-- services/surfaceflinger/SurfaceFlinger.h | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 0e5d60286..2df279ac6 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2381,9 +2381,21 @@ void SurfaceFlinger::dumpAllLocked( } const Vector< sp >& -SurfaceFlinger::getLayerSortedByZForHwcDisplay(int disp) { +SurfaceFlinger::getLayerSortedByZForHwcDisplay(int id) { // Note: mStateLock is held here - return getDisplayDevice( getBuiltInDisplay(disp) )->getVisibleLayersSortedByZ(); + wp dpy; + for (size_t i=0 ; igetHwcDisplayId() == id) { + dpy = mDisplays.keyAt(i); + break; + } + } + if (dpy == NULL) { + ALOGE("getLayerSortedByZForHwcDisplay: invalid hwc display id %d", id); + // Just use the primary display so we have something to return + dpy = getBuiltInDisplay(DisplayDevice::DISPLAY_PRIMARY); + } + return getDisplayDevice(dpy)->getVisibleLayersSortedByZ(); } bool SurfaceFlinger::startDdmConnection() diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 101756024..f0e6deb1a 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -122,7 +122,7 @@ public: // for debugging only // TODO: this should be made accessible only to HWComposer - const Vector< sp >& getLayerSortedByZForHwcDisplay(int disp); + const Vector< sp >& getLayerSortedByZForHwcDisplay(int id); private: friend class Client;