From 17e5eec07bc11bf5dc196a223996e97fd3bb0ba8 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Mon, 3 Apr 2017 17:14:31 -0700 Subject: [PATCH 1/7] fix race condition that can cause a use after free Backported from 12a0ccd6f7201bac706d903ac3f436c4358fe203. Bug: 33004354 Test: manual Change-Id: I9b38ee644b02268c9b995a330db758aa2e568399 (cherry picked from commit 59485525a6047453e6ba16c03989381e2a0d56ec) --- libs/gui/BufferQueueProducer.cpp | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index b2169c84c..ff85eb5ce 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -921,7 +921,11 @@ status_t BufferQueueProducer::queueBuffer(int slot, // Call back without the main BufferQueue lock held, but with the callback // lock held so we can ensure that callbacks occur in order - { + + int connectedApi; + sp lastQueuedFence; + + { // scope for the lock Mutex::Autolock lock(mCallbackMutex); while (callbackTicket != mCurrentCallbackTicket) { mCallbackCondition.wait(mCallbackMutex); @@ -933,20 +937,24 @@ status_t BufferQueueProducer::queueBuffer(int slot, frameReplacedListener->onFrameReplaced(item); } + connectedApi = mCore->mConnectedApi; + lastQueuedFence = std::move(mLastQueueBufferFence); + + mLastQueueBufferFence = std::move(fence); + mLastQueuedCrop = item.mCrop; + mLastQueuedTransform = item.mTransform; + ++mCurrentCallbackTicket; mCallbackCondition.broadcast(); } // Wait without lock held - if (mCore->mConnectedApi == NATIVE_WINDOW_API_EGL) { + if (connectedApi == NATIVE_WINDOW_API_EGL) { // Waiting here allows for two full buffers to be queued but not a // third. In the event that frames take varying time, this makes a // small trade-off in favor of latency rather than throughput. - mLastQueueBufferFence->waitForever("Throttling EGL Production"); + lastQueuedFence->waitForever("Throttling EGL Production"); } - mLastQueueBufferFence = fence; - mLastQueuedCrop = item.mCrop; - mLastQueuedTransform = item.mTransform; return NO_ERROR; } From db0e94b4be76dff56791c54646ca25618e488551 Mon Sep 17 00:00:00 2001 From: Namit Solanki Date: Fri, 9 Dec 2016 10:13:54 +0530 Subject: [PATCH 2/7] surfaceflinger: Fix continuous SF dump. - Pass false in dumpdrawCycle() to append SF dumps for each draw cycle in a file. Change-Id: Ibea496635b0d5a63e963f01754b5b6ddf8b09b4c CRs-Fixed: 1099278 --- services/surfaceflinger/SurfaceFlinger_hwc1.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp index c99dd6478..ed144c1bd 100644 --- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp +++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp @@ -1197,7 +1197,7 @@ void SurfaceFlinger::postComposition(nsecs_t refreshStartTime) mAnimFrameTracker.advanceFrame(); } - dumpDrawCycle(true); + dumpDrawCycle(false); if (hw->getPowerMode() == HWC_POWER_MODE_OFF) { return; From 0adab82d0bf871830bb6769f4cfb596c6a57b123 Mon Sep 17 00:00:00 2001 From: Baldev Sahu Date: Thu, 19 Jan 2017 16:32:04 +0530 Subject: [PATCH 3/7] sf: Limit co-ordinates difference fix for high res panel - Limit co-ordinate difference fix for high res panel as this is causing pixel shift on low res panel due to rounding. Change-Id: I1d697bc76ff6f962f399f6f692e347d04a059443 --- services/surfaceflinger/Layer.cpp | 97 ++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 33 deletions(-) diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 736c4d553..261567278 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -58,6 +58,9 @@ #include #define DEBUG_RESIZE 0 +#ifdef QTI_BSP +#define NUM_PIXEL_LOW_RES_PANEL (720*1280) +#endif namespace android { @@ -1059,17 +1062,33 @@ void Layer::drawWithOpenGL(const sp& hw, * like more of a hack. */ #ifdef QTI_BSP + const uint32_t hw_w = hw->getWidth(); + const uint32_t hw_h = hw->getHeight(); Rect win(s.active.w, s.active.h); + if((hw_w * hw_h) > NUM_PIXEL_LOW_RES_PANEL) { + if (!s.crop.isEmpty()) { + win = s.crop; + } - if (!s.crop.isEmpty()) { - win = s.crop; - } + win = s.active.transform.transform(win); + win.intersect(hw->getViewport(), &win); + win = s.active.transform.inverse().transform(win); + win.intersect(Rect(s.active.w, s.active.h), &win); + win = reduce(win, s.activeTransparentRegion); + } else { + win = computeBounds(); - win = s.active.transform.transform(win); - win.intersect(hw->getViewport(), &win); - win = s.active.transform.inverse().transform(win); - win.intersect(Rect(s.active.w, s.active.h), &win); - win = reduce(win, s.activeTransparentRegion); + if (!s.finalCrop.isEmpty()) { + win = s.active.transform.transform(win); + if (!win.intersect(s.finalCrop, &win)) { + win.clear(); + } + win = s.active.transform.inverse().transform(win); + if (!win.intersect(computeBounds(), &win)) { + win.clear(); + } + } + } #else Rect win(computeBounds()); @@ -1281,35 +1300,40 @@ void Layer::computeGeometry(const sp& hw, Mesh& mesh, win.intersect(s.crop, &win); } #ifdef QTI_BSP - win = s.active.transform.transform(win); - win.intersect(hw->getViewport(), &win); - win = s.active.transform.inverse().transform(win); - win.intersect(Rect(s.active.w, s.active.h), &win); - win = reduce(win, s.activeTransparentRegion); + const uint32_t hw_w = hw->getWidth(); + uint32_t orientation = 0; + if((hw_w * hw_h) > NUM_PIXEL_LOW_RES_PANEL) { + win = s.active.transform.transform(win); + win.intersect(hw->getViewport(), &win); + win = s.active.transform.inverse().transform(win); + win.intersect(Rect(s.active.w, s.active.h), &win); + win = reduce(win, s.activeTransparentRegion); - const Transform bufferOrientation(mCurrentTransform); - Transform transform(tr * s.active.transform * bufferOrientation); - if (mSurfaceFlingerConsumer->getTransformToDisplayInverse()) { - uint32_t invTransform = DisplayDevice::getPrimaryDisplayOrientationTransform(); - if (invTransform & NATIVE_WINDOW_TRANSFORM_ROT_90) { - invTransform ^= NATIVE_WINDOW_TRANSFORM_FLIP_V | + const Transform bufferOrientation(mCurrentTransform); + Transform transform(tr * s.active.transform * bufferOrientation); + if (mSurfaceFlingerConsumer->getTransformToDisplayInverse()) { + uint32_t invTransform = DisplayDevice::getPrimaryDisplayOrientationTransform(); + if (invTransform & NATIVE_WINDOW_TRANSFORM_ROT_90) { + invTransform ^= NATIVE_WINDOW_TRANSFORM_FLIP_V | NATIVE_WINDOW_TRANSFORM_FLIP_H; - } - transform = Transform(invTransform) * transform; - } - const uint32_t orientation = transform.getOrientation(); - if (!(orientation | mCurrentTransform | mTransformHint)) { - if (!useIdentityTransform) { - win = s.active.transform.transform(win); - win.intersect(hw->getViewport(), &win); + } + transform = Transform(invTransform) * transform; } + orientation = transform.getOrientation(); + if (!(orientation | mCurrentTransform | mTransformHint)) { + if (!useIdentityTransform) { + win = s.active.transform.transform(win); + win.intersect(hw->getViewport(), &win); + } + } + } else { + win = reduce(win, s.activeTransparentRegion); } #else win = reduce(win, s.activeTransparentRegion); #endif - // subtract the transparent region and snap to the bounds vec2 lt = vec2(win.left, win.top); @@ -1319,17 +1343,24 @@ void Layer::computeGeometry(const sp& hw, Mesh& mesh, if (!useIdentityTransform) { #ifdef QTI_BSP - if (orientation | mCurrentTransform | mTransformHint) { + if((hw_w * hw_h) > NUM_PIXEL_LOW_RES_PANEL) { + if (orientation | mCurrentTransform | mTransformHint) { + lt = s.active.transform.transform(lt); + lb = s.active.transform.transform(lb); + rb = s.active.transform.transform(rb); + rt = s.active.transform.transform(rt); + } + } else { lt = s.active.transform.transform(lt); lb = s.active.transform.transform(lb); rb = s.active.transform.transform(rb); rt = s.active.transform.transform(rt); } #else - lt = s.active.transform.transform(lt); - lb = s.active.transform.transform(lb); - rb = s.active.transform.transform(rb); - rt = s.active.transform.transform(rt); + lt = s.active.transform.transform(lt); + lb = s.active.transform.transform(lb); + rb = s.active.transform.transform(rb); + rt = s.active.transform.transform(rt); #endif } if (!s.finalCrop.isEmpty()) { From 96d3138f7da1416d4564d2d2abe38049398e5ba8 Mon Sep 17 00:00:00 2001 From: Pullakavi Srinivas Date: Thu, 16 Feb 2017 16:21:47 +0530 Subject: [PATCH 4/7] sf: Fix GPU coordinates computation. Along with Viewport, final crop need to be considered to maintain aspect ratio. CRs-Fixed: 2001126 Change-Id: Ia499cc059a2b0ef65002c60215d2f01ac706867f --- services/surfaceflinger/Layer.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 261567278..a4e765097 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -1072,6 +1072,11 @@ void Layer::drawWithOpenGL(const sp& hw, win = s.active.transform.transform(win); win.intersect(hw->getViewport(), &win); + if (!s.finalCrop.isEmpty()) { + if (!win.intersect(s.finalCrop, &win)) { + win.clear(); + } + } win = s.active.transform.inverse().transform(win); win.intersect(Rect(s.active.w, s.active.h), &win); win = reduce(win, s.activeTransparentRegion); @@ -1305,6 +1310,11 @@ void Layer::computeGeometry(const sp& hw, Mesh& mesh, if((hw_w * hw_h) > NUM_PIXEL_LOW_RES_PANEL) { win = s.active.transform.transform(win); win.intersect(hw->getViewport(), &win); + if (!s.finalCrop.isEmpty()) { + if (!win.intersect(s.finalCrop, &win)) { + win.clear(); + } + } win = s.active.transform.inverse().transform(win); win.intersect(Rect(s.active.w, s.active.h), &win); win = reduce(win, s.activeTransparentRegion); From a3b93a7a6c6d758b2c7b125d4ad45a0476c1c6b8 Mon Sep 17 00:00:00 2001 From: Ramakant Singh Date: Thu, 9 Feb 2017 18:23:36 +0530 Subject: [PATCH 5/7] surfaceflinger: Validate setposition parameters Validate setposition parameters width and height to fix hang issues becuase of wrong values set by clients. Change-Id: I786fc6f986fdb1d7de46ac3845f6a77cdcf34f93 CRs-Fixed: 2005153 --- services/surfaceflinger/Layer.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index a4e765097..9114fdc75 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -62,6 +62,7 @@ #define NUM_PIXEL_LOW_RES_PANEL (720*1280) #endif +#define MAX_POSITION 32767 namespace android { // --------------------------------------------------------------------------- @@ -1693,6 +1694,10 @@ uint32_t Layer::setTransactionFlags(uint32_t flags) { bool Layer::setPosition(float x, float y, bool immediate) { if (mCurrentState.requested.transform.tx() == x && mCurrentState.requested.transform.ty() == y) return false; + if ((y > MAX_POSITION) || (x > MAX_POSITION)) { + ALOGE("%s:: failed %s x = %f y = %f",__FUNCTION__,mName.string(),x, y); + return false; + } mCurrentState.sequence++; // We update the requested and active position simultaneously because From c8aefcfcefe4ec1939de36bb3a2e53a789cc3435 Mon Sep 17 00:00:00 2001 From: Francis Hart Date: Mon, 1 Dec 2014 16:04:49 +0200 Subject: [PATCH 6/7] Surface: Ensure synchronisation of copyBlt The Surface::lock() function has an optimisation for copying the previously drawn contents from the frontbuffer to the (current) backbuffer, so that the client does not have to redraw the whole surface contents. This logic was not using the sync fence FD from dequeueBuffer() and so was not correctly synchronised with pending operations on the backbuffer. Change-Id: Ib44574d50f8bdb4a23078cd8da1c5c512c876320 --- libs/gui/Surface.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp index 16ccbc30e..75e1ce0c8 100644 --- a/libs/gui/Surface.cpp +++ b/libs/gui/Surface.cpp @@ -20,6 +20,13 @@ #include +// We would eliminate the non-conforming zero-length array, but we can't since +// this is effectively included from the Linux kernel +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wzero-length-array" +#include +#pragma clang diagnostic pop + #include #include @@ -1285,8 +1292,14 @@ status_t Surface::lock( } } const Region copyback(oldDirtyRegion.subtract(newDirtyRegion)); - if (!copyback.isEmpty()) + if (!copyback.isEmpty()) { + if (fenceFd >= 0) { + sync_wait(fenceFd, -1); + close(fenceFd); + fenceFd = -1; + } copyBlt(backBuffer, frontBuffer, copyback); + } } else { // if we can't copy-back anything, modify the user's dirty // region to make sure they redraw the whole buffer From d461e7abf493a1230875df926cf9c672954b1ce8 Mon Sep 17 00:00:00 2001 From: Francis Hart Date: Fri, 9 Jan 2015 11:10:54 +0200 Subject: [PATCH 7/7] Surface: Use async lock/unlock in copyBlt The Surface::lock() function now uses the asynchronous versions of gralloc lock/unlock when copying the previously drawn content to the backbuffer. This allows for optimisations in the gralloc module implementation and so can improve performance and avoid CPU waits. Change-Id: I57193f327db2ff0422e1b58b3484f613201d994c --- libs/gui/Surface.cpp | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp index 75e1ce0c8..d423d124a 100644 --- a/libs/gui/Surface.cpp +++ b/libs/gui/Surface.cpp @@ -20,13 +20,6 @@ #include -// We would eliminate the non-conforming zero-length array, but we can't since -// this is effectively included from the Linux kernel -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wzero-length-array" -#include -#pragma clang diagnostic pop - #include #include @@ -1185,7 +1178,8 @@ void Surface::setSurfaceDamage(android_native_rect_t* rects, size_t numRects) { static status_t copyBlt( const sp& dst, const sp& src, - const Region& reg) + const Region& reg, + int *dstFenceFd) { // src and dst with, height and format must be identical. no verification // is done here. @@ -1196,9 +1190,10 @@ static status_t copyBlt( ALOGE_IF(err, "error locking src buffer %s", strerror(-err)); uint8_t* dst_bits = NULL; - err = dst->lock(GRALLOC_USAGE_SW_WRITE_OFTEN, reg.bounds(), - reinterpret_cast(&dst_bits)); + err = dst->lockAsync(GRALLOC_USAGE_SW_WRITE_OFTEN, reg.bounds(), + reinterpret_cast(&dst_bits), *dstFenceFd); ALOGE_IF(err, "error locking dst buffer %s", strerror(-err)); + *dstFenceFd = -1; Region::const_iterator head(reg.begin()); Region::const_iterator tail(reg.end()); @@ -1232,7 +1227,7 @@ static status_t copyBlt( src->unlock(); if (dst_bits) - dst->unlock(); + dst->unlockAsync(dstFenceFd); return err; } @@ -1293,12 +1288,7 @@ status_t Surface::lock( } const Region copyback(oldDirtyRegion.subtract(newDirtyRegion)); if (!copyback.isEmpty()) { - if (fenceFd >= 0) { - sync_wait(fenceFd, -1); - close(fenceFd); - fenceFd = -1; - } - copyBlt(backBuffer, frontBuffer, copyback); + copyBlt(backBuffer, frontBuffer, copyback, &fenceFd); } } else { // if we can't copy-back anything, modify the user's dirty