From d0eb968d1fe415c20304748b913fa188ba95b97f Mon Sep 17 00:00:00 2001 From: Charlie Lao Date: Fri, 8 Dec 2023 16:11:46 -0800 Subject: Vulkan: Fix the AHB leak for AHB backed buffer object For client buffer backed OpenGL buffer object, we call InitAndroidExternalMemory which calls AHB acquire. But when buffer object is released/destroyed, we never call ReleaseAndroidExternalMemory, which end up leaking AHB. Bug: b/314791770 Change-Id: I693c74213e73008497a6dfeca93ea62e84c71352 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5106599 Reviewed-by: Shahbaz Youssefi Commit-Queue: Shahbaz Youssefi Auto-Submit: Charlie Lao --- src/libANGLE/renderer/vulkan/vk_helpers.cpp | 23 +++++++++++++++++++--- src/libANGLE/renderer/vulkan/vk_helpers.h | 2 ++ src/tests/gl_tests/ExternalBufferTest.cpp | 30 ++++++++++++++++++++++++++++- 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp index 419800416e..163ecd3b9d 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp +++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp @@ -4789,10 +4789,15 @@ BufferHelper::BufferHelper() mCurrentReadAccess(0), mCurrentWriteStages(0), mCurrentReadStages(0), - mSerial() + mSerial(), + mClientBuffer(nullptr) {} -BufferHelper::~BufferHelper() = default; +BufferHelper::~BufferHelper() +{ + // We must have released external buffer properly + ASSERT(mClientBuffer == nullptr); +} BufferHelper::BufferHelper(BufferHelper &&other) { @@ -4812,6 +4817,7 @@ BufferHelper &BufferHelper::operator=(BufferHelper &&other) mCurrentWriteStages = other.mCurrentWriteStages; mCurrentReadStages = other.mCurrentReadStages; mSerial = other.mSerial; + mClientBuffer = std::move(other.mClientBuffer); return *this; } @@ -4916,6 +4922,7 @@ angle::Result BufferHelper::initExternal(ContextVk *contextVk, ANGLE_TRY(InitAndroidExternalMemory(contextVk, clientBuffer, memoryProperties, &buffer.get(), &memoryPropertyFlagsOut, &memoryTypeIndex, &deviceMemory.get(), &allocatedSize)); + mClientBuffer = clientBuffer; mSuballocation.initWithEntireBuffer( contextVk, buffer.get(), MemoryAllocationType::BufferExternal, memoryTypeIndex, @@ -4925,7 +4932,6 @@ angle::Result BufferHelper::initExternal(ContextVk *contextVk, uint8_t *ptrOut; ANGLE_TRY(map(contextVk, &ptrOut)); } - return angle::Result::Continue; } @@ -5090,6 +5096,11 @@ void BufferHelper::destroy(RendererVk *renderer) unmap(renderer); mBufferWithUserSize.destroy(renderer->getDevice()); mSuballocation.destroy(renderer); + if (mClientBuffer != nullptr) + { + ReleaseAndroidExternalMemory(renderer, mClientBuffer); + mClientBuffer = nullptr; + } } void BufferHelper::release(RendererVk *renderer) @@ -5111,6 +5122,12 @@ void BufferHelper::release(RendererVk *renderer) mUse.reset(); mWriteUse.reset(); ASSERT(!mBufferWithUserSize.valid()); + + if (mClientBuffer != nullptr) + { + ReleaseAndroidExternalMemory(renderer, mClientBuffer); + mClientBuffer = nullptr; + } } void BufferHelper::releaseBufferAndDescriptorSetCache(RendererVk *renderer) diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h index bad0c94bd2..5204f09d40 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.h +++ b/src/libANGLE/renderer/vulkan/vk_helpers.h @@ -885,6 +885,8 @@ class BufferHelper : public ReadWriteResource BufferSerial mSerial; // Manages the descriptorSet cache that created with this BufferHelper object. DescriptorSetCacheManager mDescriptorSetCacheManager; + // For external buffer + GLeglClientBufferEXT mClientBuffer; }; class BufferPool : angle::NonCopyable diff --git a/src/tests/gl_tests/ExternalBufferTest.cpp b/src/tests/gl_tests/ExternalBufferTest.cpp index 465f7d66ae..59720c9cb9 100644 --- a/src/tests/gl_tests/ExternalBufferTest.cpp +++ b/src/tests/gl_tests/ExternalBufferTest.cpp @@ -60,7 +60,10 @@ class ExternalBufferTestES31 : public ANGLETest<> // Need to grab the stride the implementation might have enforced AHardwareBuffer_describe(aHardwareBuffer, &aHardwareBufferDescription); - memcpy(mappedMemory, data, size); + if (data) + { + memcpy(mappedMemory, data, size); + } EXPECT_EQ(0, AHardwareBuffer_unlock(aHardwareBuffer, nullptr)); return aHardwareBuffer; @@ -345,6 +348,31 @@ TEST_P(ExternalBufferTestES31, MapBufferDoesNotCauseOrphaning) destroyAndroidHardwareBuffer(aHardwareBuffer); } +// Verify that create and destroy external buffer backed by an AHB doesn't leak AHB +TEST_P(ExternalBufferTestES31, BufferDoesNotLeakAHB) +{ + ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_external_buffer") || + !IsGLExtensionEnabled("GL_EXT_buffer_storage")); + + // Create and destroy 128M AHB backed buffer in a loop. If we leak AHB, it will fail due to AHB + // allocation failure before loop ends. + constexpr size_t kBufferSize = 128 * 1024 * 1024; + for (int loop = 0; loop < 1000; loop++) + { + // Create the AHB + AHardwareBuffer *aHardwareBuffer; + constexpr GLbitfield kFlags = GL_DYNAMIC_STORAGE_BIT_EXT; + aHardwareBuffer = createAndroidHardwareBuffer(kBufferSize, nullptr); + GLBuffer buffer; + glBindBuffer(GL_SHADER_STORAGE_BUFFER, buffer); + glBufferStorageExternalEXT(GL_SHADER_STORAGE_BUFFER, 0, kBufferSize, + eglGetNativeClientBufferANDROID(aHardwareBuffer), kFlags); + ASSERT_GL_NO_ERROR(); + // Delete the source AHB + destroyAndroidHardwareBuffer(aHardwareBuffer); + } +} + GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(ExternalBufferTestES31); ANGLE_INSTANTIATE_TEST_ES31(ExternalBufferTestES31); } // namespace angle -- cgit v1.2.3