Return to manual slot use instead of slot generation

ION memory increase in composer process due to slot generation.
Revert to old slot generation for BufferQueue

Bug: 127655433
Test: build, boot, HwcBufferCacheTest
Change-Id: Ifbb5747a7878aef97c92ba09e2dc72d663d78066

7 files changed, 83 insertions, 47 deletions

diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp
index e4179ee43a..d3b36fe354 100644
--- a/services/surfaceflinger/BufferQueueLayer.cpp
+++ b/services/surfaceflinger/BufferQueueLayer.cpp
@@ -361,8 +361,12 @@ void BufferQueueLayer::setHwcLayerBuffer(const sp<const DisplayDevice>& display)
 
     uint32_t hwcSlot = 0;
     sp<GraphicBuffer> hwcBuffer;
+
+    // INVALID_BUFFER_SLOT is used to identify BufferStateLayers.  Default to 0
+    // for BufferQueueLayers
+    int slot = (mActiveBufferSlot == BufferQueue::INVALID_BUFFER_SLOT) ? 0 : mActiveBufferSlot;
     (*outputLayer->editState().hwc)
-            .hwcBufferCache.getHwcBuffer(mActiveBuffer, &hwcSlot, &hwcBuffer);
+            .hwcBufferCache.getHwcBuffer(slot, 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 5efa4e0a1c..64dfdc3d5e 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -585,10 +585,10 @@ void BufferStateLayer::setHwcLayerBuffer(const sp<const DisplayDevice>& display)
 
     const State& s(getDrawingState());
 
-    // obtain slot
-    uint32_t hwcSlot = 0;
+    uint32_t hwcSlot;
     sp<GraphicBuffer> buffer;
-    hwcInfo.hwcBufferCache.getHwcBuffer(s.buffer, &hwcSlot, &buffer);
+    hwcInfo.hwcBufferCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, s.buffer, &hwcSlot,
+                                        &buffer);
 
     auto error = hwcLayer->setBuffer(hwcSlot, buffer, s.acquireFence);
     if (error != HWC2::Error::None) {
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
index 02d7890c9d..97bdc8ff67 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/HwcBufferCache.h
@@ -45,7 +45,7 @@ public:
     //
     // outBuffer is set to buffer when buffer is not in the HWC cache;
     // otherwise, outBuffer is set to nullptr.
-    void getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+    void getHwcBuffer(int slot, const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
                       sp<GraphicBuffer>* outBuffer);
 
 protected:
diff --git a/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp b/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
index 8613210cfc..a941e0962d 100644
--- a/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/HwcBufferCache.cpp
@@ -48,12 +48,20 @@ uint32_t HwcBufferCache::getLeastRecentlyUsedSlot() {
     return std::distance(std::begin(mBuffers), iter);
 }
 
-void HwcBufferCache::getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+void HwcBufferCache::getHwcBuffer(int slot, const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
                                   sp<GraphicBuffer>* outBuffer) {
-    bool cached = getSlot(buffer, outSlot);
+    // if this slot corresponds to a BufferStateLayer, generate the slot
+    if (slot == BufferQueue::INVALID_BUFFER_SLOT) {
+        getSlot(buffer, outSlot);
+    } else if (slot < 0 || slot >= BufferQueue::NUM_BUFFER_SLOTS) {
+        *outSlot = 0;
+    } else {
+        *outSlot = slot;
+    }
 
     auto& [currentCounter, currentBuffer] = mBuffers[*outSlot];
-    if (cached) {
+    wp<GraphicBuffer> weakCopy(buffer);
+    if (currentBuffer == weakCopy) {
         // already cached in HWC, skip sending the buffer
         *outBuffer = nullptr;
         currentCounter = getCounter();
diff --git a/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp b/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
index ac04cb3f32..b261493a20 100644
--- a/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/HwcBufferCacheTest.cpp
@@ -24,9 +24,9 @@ namespace {
 
 class TestableHwcBufferCache : public impl::HwcBufferCache {
 public:
-    void getHwcBuffer(const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
+    void getHwcBuffer(int slot, const sp<GraphicBuffer>& buffer, uint32_t* outSlot,
                       sp<GraphicBuffer>* outBuffer) {
-        HwcBufferCache::getHwcBuffer(buffer, outSlot, outBuffer);
+        HwcBufferCache::getHwcBuffer(slot, buffer, outSlot, outBuffer);
     }
     bool getSlot(const sp<GraphicBuffer>& buffer, uint32_t* outSlot) {
         return HwcBufferCache::getSlot(buffer, outSlot);
@@ -38,64 +38,88 @@ class HwcBufferCacheTest : public testing::Test {
 public:
     ~HwcBufferCacheTest() override = default;
 
-    TestableHwcBufferCache mCache;
+    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;
     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, testSlot) {
+TEST_F(HwcBufferCacheTest, cacheWorksForSlotZero) {
+    testSlot(0, 0);
+}
+
+TEST_F(HwcBufferCacheTest, cacheWorksForMaxSlot) {
+    testSlot(BufferQueue::NUM_BUFFER_SLOTS - 1, BufferQueue::NUM_BUFFER_SLOTS - 1);
+}
+
+TEST_F(HwcBufferCacheTest, cacheMapsNegativeSlotToZero) {
+    testSlot(-123, 0);
+}
+
+TEST_F(HwcBufferCacheTest, cacheGeneratesSlotForInvalidBufferSlot) {
     uint32_t outSlot;
     sp<GraphicBuffer> outBuffer;
 
-    // The first time, the output  is the same as the input
-    mCache.getHwcBuffer(mBuffer1, &outSlot, &outBuffer);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, mBuffer1, &outSlot, &outBuffer);
     EXPECT_EQ(0, outSlot);
     EXPECT_EQ(mBuffer1, outBuffer);
 
-    // The second time with the same buffer, the outBuffer is nullptr.
-    mCache.getHwcBuffer(mBuffer1, &outSlot, &outBuffer);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, 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);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, 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);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, 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);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, sp<GraphicBuffer>(), &outSlot,
+                        &outBuffer);
     EXPECT_EQ(2, outSlot);
     EXPECT_EQ(nullptr, outBuffer.get());
-}
 
-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);
+    // note that sending mBuffer1 with explicit slot 1 will overwrite mBuffer2
+    // and also cause mBuffer1 to be stored in two places
+    mCache.getHwcBuffer(1, mBuffer1, &outSlot, &outBuffer);
+    EXPECT_EQ(1, outSlot);
     EXPECT_EQ(mBuffer1, outBuffer);
 
-    slot = mCache.getLeastRecentlyUsedSlot();
-    EXPECT_EQ(1, slot);
+    mCache.getHwcBuffer(BufferQueue::INVALID_BUFFER_SLOT, mBuffer2, &outSlot, &outBuffer);
+    EXPECT_EQ(3, outSlot);
+    EXPECT_EQ(mBuffer2, outBuffer);
 }
 
 } // namespace
diff --git a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
index 775fb806b8..7370b0ccb2 100644
--- a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
+++ b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
@@ -111,7 +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(mCurrentBuffer, &outSlot, &outBuffer);
+        mHwcBufferCache.getHwcBuffer(mCurrentBufferSlot, mCurrentBuffer, &outSlot, &outBuffer);
         return NO_ERROR;
     } else if (err != NO_ERROR) {
         ALOGE("error acquiring buffer: %s (%d)", strerror(-err), err);
@@ -137,7 +137,7 @@ status_t FramebufferSurface::nextBuffer(uint32_t& outSlot,
     mCurrentFence = item.mFence;
 
     outFence = item.mFence;
-    mHwcBufferCache.getHwcBuffer(mCurrentBuffer, &outSlot, &outBuffer);
+    mHwcBufferCache.getHwcBuffer(mCurrentBufferSlot, 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 613dc777ee..4e0e4df524 100644
--- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
+++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp
@@ -224,7 +224,7 @@ status_t VirtualDisplaySurface::advanceFrame() {
     if (fbBuffer != nullptr) {
         uint32_t hwcSlot = 0;
         sp<GraphicBuffer> hwcBuffer;
-        mHwcBufferCache.getHwcBuffer(fbBuffer, &hwcSlot, &hwcBuffer);
+        mHwcBufferCache.getHwcBuffer(mFbProducerSlot, fbBuffer, &hwcSlot, &hwcBuffer);
 
         // TODO: Correctly propagate the dataspace from GL composition
         result = mHwc.setClientTarget(*mDisplayId, hwcSlot, mFbFence, hwcBuffer,