Merge "Caching between SF and HWC extended to Buffer State Layers"

7 files changed, 122 insertions, 71 deletions

diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp
index 16e8e951f7..7cd9e4968b 100644
--- a/services/surfaceflinger/BufferQueueLayer.cpp
+++ b/services/surfaceflinger/BufferQueueLayer.cpp
@@ -362,7 +362,7 @@ void BufferQueueLayer::setHwcLayerBuffer(const sp<const DisplayDevice>& display)
     uint32_t hwcSlot = 0;
     sp<GraphicBuffer> hwcBuffer;
     (*outputLayer->editState().hwc)
-            .hwcBufferCache.getHwcBuffer(mActiveBufferSlot, mActiveBuffer, &hwcSlot, &hwcBuffer);
+            .hwcBufferCache.getHwcBuffer(mActiveBuffer, &hwcSlot, &hwcBuffer);
 
     auto acquireFence = mConsumer->getCurrentFence();
     auto error = hwcLayer->setBuffer(hwcSlot, hwcBuffer, acquireFence);
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 6390e8574f..b2383ad8a7 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -570,14 +570,17 @@ status_t BufferStateLayer::updateFrameNumber(nsecs_t /*latchTime*/) {
 void BufferStateLayer::setHwcLayerBuffer(const sp<const DisplayDevice>& display) {
     const auto outputLayer = findOutputLayerForDisplay(display);
     LOG_FATAL_IF(!outputLayer || !outputLayer->getState().hwc);
-    auto& hwcLayer = (*outputLayer->getState().hwc).hwcLayer;
+    auto& hwcInfo = *outputLayer->editState().hwc;
+    auto& hwcLayer = hwcInfo.hwcLayer;
 
     const State& s(getDrawingState());
 
-    // TODO(marissaw): support more than one slot
+    // obtain slot
     uint32_t hwcSlot = 0;
+    sp<GraphicBuffer> buffer;
+    hwcInfo.hwcBufferCache.getHwcBuffer(s.buffer, &hwcSlot, &buffer);
 
-    auto error = hwcLayer->setBuffer(hwcSlot, s.buffer, s.acquireFence);
+    auto error = hwcLayer->setBuffer(hwcSlot, buffer, s.acquireFence);
     if (error != HWC2::Error::None) {
         ALOGE("[%s] Failed to set buffer %p: %s (%d)", mName.string(),
               s.buffer->handle, to_string(error).c_str(), static_cast<int32_t>(error));
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
index b45de5a619..02d7890c9d 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
@@ -19,6 +19,7 @@
 #include <cstdint>
 #include <vector>
 
+#include <gui/BufferQueue.h>
 #include <utils/StrongPointer.h>
 
 namespace android {
@@ -39,19 +40,28 @@ namespace compositionengine::impl {
 class HwcBufferCache {
 public:
     HwcBufferCache();
-
-    // Given a buffer queue slot and buffer, return the HWC cache slot and
+    // Given a buffer, return the HWC cache slot and
     // buffer to be sent to HWC.
     //
     // outBuffer is set to buffer when buffer is not in the HWC cache;
     // otherwise, outBuffer is set to nullptr.
-    void getHwcBuffer(int slot, const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+    void getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
                       sp<GraphicBuffer>* outBuffer);
 
+protected:
+    bool getSlot(const sp<GraphicBuffer>& buffer, uint32_t* outSlot);
+    uint32_t getLeastRecentlyUsedSlot();
+    uint64_t getCounter();
+
 private:
-    // a vector as we expect "slot" to be in the range of [0, 63] (that is,
-    // less than BufferQueue::NUM_BUFFER_SLOTS).
-    std::vector<sp<GraphicBuffer>> mBuffers;
+    // an array where the index corresponds to a slot and the value corresponds to a (counter,
+    // buffer) pair. "counter" is a unique value that indicates the last time this slot was updated
+    // or used and allows us to keep track of the least-recently used buffer.
+    std::pair<uint64_t, wp<GraphicBuffer>> mBuffers[BufferQueue::NUM_BUFFER_SLOTS];
+
+    // The cache increments this counter value when a slot is updated or used.
+    // Used to track the least recently-used buffer
+    uint64_t mCounter = 1;
 };
 
 } // namespace compositionengine::impl
diff --git a/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp b/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
index 6f340b96d0..8613210cfc 100644
--- a/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
@@ -21,31 +21,52 @@
 namespace android::compositionengine::impl {
 
 HwcBufferCache::HwcBufferCache() {
-    mBuffers.reserve(BufferQueue::NUM_BUFFER_SLOTS);
+    std::fill(std::begin(mBuffers), std::end(mBuffers),
+              std::pair<uint64_t, wp<GraphicBuffer>>(0, nullptr));
 }
-
-void HwcBufferCache::getHwcBuffer(int slot, const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
-                                  sp<GraphicBuffer>* outBuffer) {
-    if (slot == BufferQueue::INVALID_BUFFER_SLOT || slot < 0) {
-        // default to slot 0
-        slot = 0;
+bool HwcBufferCache::getSlot(const sp<GraphicBuffer>& buffer, uint32_t* outSlot) {
+    // search for cached buffer first
+    for (int i = 0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
+        // Weak pointers in the cache may have had their object destroyed.
+        // Comparisons between weak pointers will accurately reflect this case,
+        // but comparisons between weak and strong may not.  Thus, we create a weak
+        // pointer from strong pointer buffer
+        wp<GraphicBuffer> weakCopy(buffer);
+        if (mBuffers[i].second == weakCopy) {
+            *outSlot = i;
+            return true;
+        }
     }
 
-    if (static_cast<size_t>(slot) >= mBuffers.size()) {
-        mBuffers.resize(slot + 1);
-    }
+    // use the least-recently used slot
+    *outSlot = getLeastRecentlyUsedSlot();
+    return false;
+}
 
-    *outSlot = slot;
+uint32_t HwcBufferCache::getLeastRecentlyUsedSlot() {
+    auto iter = std::min_element(std::begin(mBuffers), std::end(mBuffers));
+    return std::distance(std::begin(mBuffers), iter);
+}
+
+void HwcBufferCache::getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+                                  sp<GraphicBuffer>* outBuffer) {
+    bool cached = getSlot(buffer, outSlot);
 
-    if (mBuffers[slot] == buffer) {
+    auto& [currentCounter, currentBuffer] = mBuffers[*outSlot];
+    if (cached) {
         // already cached in HWC, skip sending the buffer
         *outBuffer = nullptr;
+        currentCounter = getCounter();
     } else {
         *outBuffer = buffer;
 
         // update cache
-        mBuffers[slot] = buffer;
+        currentBuffer = buffer;
+        currentCounter = getCounter();
     }
 }
 
+uint64_t HwcBufferCache::getCounter() {
+    return mCounter++;
+}
 } // namespace android::compositionengine::impl
diff --git a/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp b/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
index f2a1aad2fc..ac04cb3f32 100644
--- a/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
@@ -22,60 +22,80 @@
 namespace android::compositionengine {
 namespace {
 
+class TestableHwcBufferCache : public impl::HwcBufferCache {
+public:
+    void getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+                      sp<GraphicBuffer>* outBuffer) {
+        HwcBufferCache::getHwcBuffer(buffer, outSlot, outBuffer);
+    }
+    bool getSlot(const sp<GraphicBuffer>& buffer, uint32_t* outSlot) {
+        return HwcBufferCache::getSlot(buffer, outSlot);
+    }
+    uint32_t getLeastRecentlyUsedSlot() { return HwcBufferCache::getLeastRecentlyUsedSlot(); }
+};
+
 class HwcBufferCacheTest : public testing::Test {
 public:
     ~HwcBufferCacheTest() override = default;
 
-    void testSlot(const int inSlot, const uint32_t expectedSlot) {
-        uint32_t outSlot;
-        sp<GraphicBuffer> outBuffer;
-
-        // The first time, the output  is the same as the input
-        mCache.getHwcBuffer(inSlot, mBuffer1, &outSlot, &outBuffer);
-        EXPECT_EQ(expectedSlot, outSlot);
-        EXPECT_EQ(mBuffer1, outBuffer);
-
-        // The second time with the same buffer, the outBuffer is nullptr.
-        mCache.getHwcBuffer(inSlot, mBuffer1, &outSlot, &outBuffer);
-        EXPECT_EQ(expectedSlot, outSlot);
-        EXPECT_EQ(nullptr, outBuffer.get());
-
-        // With a new buffer, the outBuffer is the input.
-        mCache.getHwcBuffer(inSlot, mBuffer2, &outSlot, &outBuffer);
-        EXPECT_EQ(expectedSlot, outSlot);
-        EXPECT_EQ(mBuffer2, outBuffer);
-
-        // Again, the second request with the same buffer sets outBuffer to nullptr.
-        mCache.getHwcBuffer(inSlot, mBuffer2, &outSlot, &outBuffer);
-        EXPECT_EQ(expectedSlot, outSlot);
-        EXPECT_EQ(nullptr, outBuffer.get());
-
-        // Setting a slot to use nullptr lookslike works, but note that
-        // the output values make it look like no new buffer is being set....
-        mCache.getHwcBuffer(inSlot, sp<GraphicBuffer>(), &outSlot, &outBuffer);
-        EXPECT_EQ(expectedSlot, outSlot);
-        EXPECT_EQ(nullptr, outBuffer.get());
-    }
-
-    impl::HwcBufferCache mCache;
+    TestableHwcBufferCache mCache;
     sp<GraphicBuffer> mBuffer1{new GraphicBuffer(1, 1, HAL_PIXEL_FORMAT_RGBA_8888, 1, 0)};
     sp<GraphicBuffer> mBuffer2{new GraphicBuffer(1, 1, HAL_PIXEL_FORMAT_RGBA_8888, 1, 0)};
 };
 
-TEST_F(HwcBufferCacheTest, cacheWorksForSlotZero) {
-    testSlot(0, 0);
-}
+TEST_F(HwcBufferCacheTest, testSlot) {
+    uint32_t outSlot;
+    sp<GraphicBuffer> outBuffer;
 
-TEST_F(HwcBufferCacheTest, cacheWorksForMaxSlot) {
-    testSlot(BufferQueue::NUM_BUFFER_SLOTS - 1, BufferQueue::NUM_BUFFER_SLOTS - 1);
-}
+    // The first time, the output  is the same as the input
+    mCache.getHwcBuffer(mBuffer1, &outSlot, &outBuffer);
+    EXPECT_EQ(0, outSlot);
+    EXPECT_EQ(mBuffer1, outBuffer);
 
-TEST_F(HwcBufferCacheTest, cacheMapsNegativeSlotToZero) {
-    testSlot(-123, 0);
+    // The second time with the same buffer, the outBuffer is nullptr.
+    mCache.getHwcBuffer(mBuffer1, &outSlot, &outBuffer);
+    EXPECT_EQ(0, outSlot);
+    EXPECT_EQ(nullptr, outBuffer.get());
+
+    // With a new buffer, the outBuffer is the input.
+    mCache.getHwcBuffer(mBuffer2, &outSlot, &outBuffer);
+    EXPECT_EQ(1, outSlot);
+    EXPECT_EQ(mBuffer2, outBuffer);
+
+    // Again, the second request with the same buffer sets outBuffer to nullptr.
+    mCache.getHwcBuffer(mBuffer2, &outSlot, &outBuffer);
+    EXPECT_EQ(1, outSlot);
+    EXPECT_EQ(nullptr, outBuffer.get());
+
+    // Setting a slot to use nullptr lookslike works, but note that
+    // the output values make it look like no new buffer is being set....
+    mCache.getHwcBuffer(sp<GraphicBuffer>(), &outSlot, &outBuffer);
+    EXPECT_EQ(2, outSlot);
+    EXPECT_EQ(nullptr, outBuffer.get());
 }
 
-TEST_F(HwcBufferCacheTest, cacheMapsInvalidBufferSlotToZero) {
-    testSlot(BufferQueue::INVALID_BUFFER_SLOT, 0);
+TEST_F(HwcBufferCacheTest, testGetLeastRecentlyUsedSlot) {
+    int slot;
+    uint32_t outSlot;
+    sp<GraphicBuffer> outBuffer;
+
+    // fill up cache
+    for (int i = 0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
+        sp<GraphicBuffer> buf{new GraphicBuffer(1, 1, HAL_PIXEL_FORMAT_RGBA_8888, 1, 0)};
+        mCache.getHwcBuffer(buf, &outSlot, &outBuffer);
+        EXPECT_EQ(buf, outBuffer);
+        EXPECT_EQ(i, outSlot);
+    }
+
+    slot = mCache.getLeastRecentlyUsedSlot();
+    EXPECT_EQ(0, slot);
+
+    mCache.getHwcBuffer(mBuffer1, &outSlot, &outBuffer);
+    EXPECT_EQ(0, outSlot);
+    EXPECT_EQ(mBuffer1, outBuffer);
+
+    slot = mCache.getLeastRecentlyUsedSlot();
+    EXPECT_EQ(1, slot);
 }
 
 } // namespace
diff --git a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
index 27812f7492..775fb806b8 100644
--- a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
+++ b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
@@ -111,8 +111,7 @@ status_t FramebufferSurface::nextBuffer(uint32_t& outSlot,
     BufferItem item;
     status_t err = acquireBufferLocked(&item, 0);
     if (err == BufferQueue::NO_BUFFER_AVAILABLE) {
-        mHwcBufferCache.getHwcBuffer(mCurrentBufferSlot, mCurrentBuffer,
-                &outSlot, &outBuffer);
+        mHwcBufferCache.getHwcBuffer(mCurrentBuffer, &outSlot, &outBuffer);
         return NO_ERROR;
     } else if (err != NO_ERROR) {
         ALOGE("error acquiring buffer: %s (%d)", strerror(-err), err);
@@ -138,8 +137,7 @@ status_t FramebufferSurface::nextBuffer(uint32_t& outSlot,
     mCurrentFence = item.mFence;
 
     outFence = item.mFence;
-    mHwcBufferCache.getHwcBuffer(mCurrentBufferSlot, mCurrentBuffer,
-            &outSlot, &outBuffer);
+    mHwcBufferCache.getHwcBuffer(mCurrentBuffer, &outSlot, &outBuffer);
     outDataspace = static_cast<Dataspace>(item.mDataSpace);
     status_t result = mHwc.setClientTarget(mDisplayId, outSlot, outFence, outBuffer, outDataspace);
     if (result != NO_ERROR) {
diff --git a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
index 1c2853a857..613dc777ee 100644
--- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
+++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
@@ -224,8 +224,7 @@ status_t VirtualDisplaySurface::advanceFrame() {
     if (fbBuffer != nullptr) {
         uint32_t hwcSlot = 0;
         sp<GraphicBuffer> hwcBuffer;
-        mHwcBufferCache.getHwcBuffer(mFbProducerSlot, fbBuffer,
-                &hwcSlot, &hwcBuffer);
+        mHwcBufferCache.getHwcBuffer(fbBuffer, &hwcSlot, &hwcBuffer);
 
         // TODO: Correctly propagate the dataspace from GL composition
         result = mHwc.setClientTarget(*mDisplayId, hwcSlot, mFbFence, hwcBuffer,