From 2c45bb16f577110e16f4b95f455527221604c71a Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Fri, 18 Oct 2019 13:31:36 -0700 Subject: [PATCH] gralloc: add flush and reread for locked buffers When a buffer is locked (mapped to the CPU) by two or more clients, there is no good way to manage a reader/writer relationship. There are no requirements for how the writes should propagate to the readers. Clients must unlock and relock to be sure writes are flushed. They must unlock and relock to get the lastest copy of the buffer. This patch adds explicit flush and reread commands to help readers and writers synchronize without having to unmap and remap the buffer. Bug: 136316517 Test: TODO Change-Id: I10d3de1b0e46c4f3b50dc34aea653701933638a9 --- graphics/mapper/4.0/IMapper.hal | 44 ++++++++ graphics/mapper/4.0/utils/vts/MapperVts.cpp | 28 +++++ .../vts/include/mapper-vts/4.0/MapperVts.h | 3 + graphics/mapper/4.0/vts/functional/Android.bp | 1 + .../VtsHalGraphicsMapperV4_0TargetTest.cpp | 103 +++++++++++++++--- 5 files changed, 166 insertions(+), 13 deletions(-) diff --git a/graphics/mapper/4.0/IMapper.hal b/graphics/mapper/4.0/IMapper.hal index 298f31ed56..fa69987c0b 100644 --- a/graphics/mapper/4.0/IMapper.hal +++ b/graphics/mapper/4.0/IMapper.hal @@ -258,6 +258,50 @@ interface IMapper { */ unlock(pointer buffer) generates (Error error, handle releaseFence); + /** + * Flushes the contents of a locked buffer. + * + * This function flushes the CPUs caches for the range of all the buffer's + * planes and metadata. This should behave similarly to unlock() except the + * buffer should remain mapped to the CPU. + * + * The client is still responsible for calling unlock() when it is done + * with all CPU accesses to the buffer. + * + * If non-CPU blocks are simultaneously writing the buffer, the locked + * copy should still be flushed but what happens is undefined except that + * it should not cause any crashes. + * + * @param buffer Buffer to flush. + * @return error Error status of the call, which may be + * - `NONE` upon success. + * - `BAD_BUFFER` if the buffer is invalid or not locked. + * @return releaseFence Handle containing a file descriptor referring to a + * sync fence object. The sync fence object will be signaled when the + * mapper has completed any pending work. @p releaseFence may be an + * empty fence. + */ + flushLockedBuffer(pointer buffer) generates (Error error, handle releaseFence); + + /** + * Rereads the contents of a locked buffer. + * + * This should fetch the most recent copy of the locked buffer. + * + * It may reread locked copies of the buffer in other processes. + * + * The client is still responsible for calling unlock() when it is done + * with all CPU accesses to the buffer. + * + * @param buffer Buffer to reread. + * @return error Error status of the call, which may be + * - `NONE` upon success. + * - `BAD_BUFFER` if the buffer is invalid or not locked. + * - `NO_RESOURCES` if the buffer cannot be reread at this time. Note + * that rereading may succeed at a later time. + */ + rereadLockedBuffer(pointer buffer) generates(Error error); + /** * Test whether the given BufferDescriptorInfo is allocatable. * diff --git a/graphics/mapper/4.0/utils/vts/MapperVts.cpp b/graphics/mapper/4.0/utils/vts/MapperVts.cpp index 8073e6924c..d0edffb84f 100644 --- a/graphics/mapper/4.0/utils/vts/MapperVts.cpp +++ b/graphics/mapper/4.0/utils/vts/MapperVts.cpp @@ -246,6 +246,34 @@ int Gralloc::unlock(const native_handle_t* bufferHandle) { return releaseFence; } +int Gralloc::flushLockedBuffer(const native_handle_t* bufferHandle) { + auto buffer = const_cast(bufferHandle); + + int releaseFence = -1; + mMapper->flushLockedBuffer(buffer, [&](const auto& tmpError, const auto& tmpReleaseFence) { + ASSERT_EQ(Error::NONE, tmpError) << "failed to flush locked buffer " << buffer; + + auto fenceHandle = tmpReleaseFence.getNativeHandle(); + if (fenceHandle) { + ASSERT_EQ(0, fenceHandle->numInts) << "invalid fence handle " << fenceHandle; + if (fenceHandle->numFds == 1) { + releaseFence = dup(fenceHandle->data[0]); + ASSERT_LT(0, releaseFence) << "failed to dup fence fd"; + } else { + ASSERT_EQ(0, fenceHandle->numFds) << " invalid fence handle " << fenceHandle; + } + } + }); + + return releaseFence; +} + +void Gralloc::rereadLockedBuffer(const native_handle_t* bufferHandle) { + auto buffer = const_cast(bufferHandle); + + ASSERT_EQ(Error::NONE, mMapper->rereadLockedBuffer(buffer)); +} + bool Gralloc::validateBufferSize(const native_handle_t* bufferHandle, const IMapper::BufferDescriptorInfo& descriptorInfo, uint32_t stride) { diff --git a/graphics/mapper/4.0/utils/vts/include/mapper-vts/4.0/MapperVts.h b/graphics/mapper/4.0/utils/vts/include/mapper-vts/4.0/MapperVts.h index 6251e6649c..271041852d 100644 --- a/graphics/mapper/4.0/utils/vts/include/mapper-vts/4.0/MapperVts.h +++ b/graphics/mapper/4.0/utils/vts/include/mapper-vts/4.0/MapperVts.h @@ -73,6 +73,9 @@ class Gralloc { const IMapper::Rect& accessRegion, int acquireFence); int unlock(const native_handle_t* bufferHandle); + int flushLockedBuffer(const native_handle_t* bufferHandle); + void rereadLockedBuffer(const native_handle_t* bufferHandle); + bool validateBufferSize(const native_handle_t* bufferHandle, const IMapper::BufferDescriptorInfo& descriptorInfo, uint32_t stride); void getTransportSize(const native_handle_t* bufferHandle, uint32_t* outNumFds, diff --git a/graphics/mapper/4.0/vts/functional/Android.bp b/graphics/mapper/4.0/vts/functional/Android.bp index a8030ab633..506026d8bf 100644 --- a/graphics/mapper/4.0/vts/functional/Android.bp +++ b/graphics/mapper/4.0/vts/functional/Android.bp @@ -20,6 +20,7 @@ cc_test { srcs: ["VtsHalGraphicsMapperV4_0TargetTest.cpp"], static_libs: [ "android.hardware.graphics.mapper@4.0-vts", + "libsync", ], shared_libs: [ "android.hardware.graphics.allocator@4.0", diff --git a/graphics/mapper/4.0/vts/functional/VtsHalGraphicsMapperV4_0TargetTest.cpp b/graphics/mapper/4.0/vts/functional/VtsHalGraphicsMapperV4_0TargetTest.cpp index d63b078d1b..be53bc5334 100644 --- a/graphics/mapper/4.0/vts/functional/VtsHalGraphicsMapperV4_0TargetTest.cpp +++ b/graphics/mapper/4.0/vts/functional/VtsHalGraphicsMapperV4_0TargetTest.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -273,6 +274,24 @@ class GraphicsMapperHidlTest : public ::testing::VtsHalHidlTargetTestBase { ASSERT_NE(nullptr, outYCbCr->cr); } + void fillRGBA8888(uint8_t* data, uint32_t height, size_t strideInBytes, size_t widthInBytes, + uint32_t seed = 0) { + for (uint32_t y = 0; y < height; y++) { + memset(data, y + seed, widthInBytes); + data += strideInBytes; + } + } + + void verifyRGBA8888(uint8_t* data, uint32_t height, size_t strideInBytes, size_t widthInBytes, + uint32_t seed = 0) { + for (uint32_t y = 0; y < height; y++) { + for (size_t i = 0; i < widthInBytes; i++) { + EXPECT_EQ(static_cast(y + seed), data[i]); + } + data += strideInBytes; + } + } + std::unique_ptr mGralloc; IMapper::BufferDescriptorInfo mDummyDescriptorInfo{}; static const std::set sRequiredMetadataTypes; @@ -529,25 +548,14 @@ TEST_F(GraphicsMapperHidlTest, LockUnlockBasic) { data = static_cast(mGralloc->lock(bufferHandle, info.usage, region, fence))); // RGBA_8888 - size_t strideInBytes = stride * 4; - size_t writeInBytes = info.width * 4; - - for (uint32_t y = 0; y < info.height; y++) { - memset(data, y, writeInBytes); - data += strideInBytes; - } + fillRGBA8888(data, info.height, stride * 4, info.width * 4); ASSERT_NO_FATAL_FAILURE(fence = mGralloc->unlock(bufferHandle)); // lock again for reading ASSERT_NO_FATAL_FAILURE( data = static_cast(mGralloc->lock(bufferHandle, info.usage, region, fence))); - for (uint32_t y = 0; y < info.height; y++) { - for (size_t i = 0; i < writeInBytes; i++) { - EXPECT_EQ(static_cast(y), data[i]); - } - data += strideInBytes; - } + ASSERT_NO_FATAL_FAILURE(verifyRGBA8888(data, info.height, stride * 4, info.width * 4)); ASSERT_NO_FATAL_FAILURE(fence = mGralloc->unlock(bufferHandle)); if (fence >= 0) { @@ -705,6 +713,75 @@ TEST_F(GraphicsMapperHidlTest, UnlockNegative) { #endif } +/** + * Test IMapper::flush and IMapper::reread. + */ +TEST_F(GraphicsMapperHidlTest, FlushRereadBasic) { + const auto& info = mDummyDescriptorInfo; + + const native_handle_t* rawHandle; + uint32_t stride; + ASSERT_NO_FATAL_FAILURE( + rawHandle = mGralloc->allocate(mDummyDescriptorInfo, false, false, &stride)); + + const native_handle_t* writeBufferHandle; + const native_handle_t* readBufferHandle; + ASSERT_NO_FATAL_FAILURE(writeBufferHandle = mGralloc->importBuffer(rawHandle)); + ASSERT_NO_FATAL_FAILURE(readBufferHandle = mGralloc->importBuffer(rawHandle)); + + // lock buffer for writing + const IMapper::Rect region{0, 0, static_cast(info.width), + static_cast(info.height)}; + uint8_t* writeData; + ASSERT_NO_FATAL_FAILURE( + writeData = static_cast(mGralloc->lock( + writeBufferHandle, static_cast(BufferUsage::CPU_WRITE_OFTEN), region, + -1))); + + uint8_t* readData; + ASSERT_NO_FATAL_FAILURE( + readData = static_cast(mGralloc->lock( + readBufferHandle, static_cast(BufferUsage::CPU_READ_OFTEN), region, + -1))); + + fillRGBA8888(writeData, info.height, stride * 4, info.width * 4); + + int fence; + ASSERT_NO_FATAL_FAILURE(fence = mGralloc->flushLockedBuffer(writeBufferHandle)); + ASSERT_EQ(0, sync_wait(fence, 3500)); + close(fence); + + ASSERT_NO_FATAL_FAILURE(mGralloc->rereadLockedBuffer(readBufferHandle)); + + ASSERT_NO_FATAL_FAILURE(verifyRGBA8888(readData, info.height, stride * 4, info.width * 4)); + + ASSERT_NO_FATAL_FAILURE(fence = mGralloc->unlock(readBufferHandle)); + if (fence >= 0) { + close(fence); + } + ASSERT_NO_FATAL_FAILURE(fence = mGralloc->unlock(writeBufferHandle)); + if (fence >= 0) { + close(fence); + } +} + +/** + * Test IMapper::flushLockedBuffer with bad buffer + */ +TEST_F(GraphicsMapperHidlTest, FlushLockedBufferBadBuffer) { + ASSERT_NO_FATAL_FAILURE(mGralloc->getMapper()->flushLockedBuffer( + nullptr, [&](const auto& tmpError, const auto& /*tmpReleaseFence*/) { + ASSERT_EQ(Error::BAD_BUFFER, tmpError); + })); +} + +/** + * Test IMapper::rereadLockedBuffer with bad buffer + */ +TEST_F(GraphicsMapperHidlTest, RereadLockedBufferBadBuffer) { + ASSERT_EQ(Error::BAD_BUFFER, mGralloc->getMapper()->rereadLockedBuffer(nullptr)); +} + /** * Test IMapper::isSupported with required format RGBA_8888 */