From 2d5cf02ee203feffa5966b79f29b2410ee495b70 Mon Sep 17 00:00:00 2001 From: Bo Liu Date: Thu, 9 Nov 2023 23:07:23 +0000 Subject: [PATCH] si: AHB backing concurrent-read-write support Only relax write synchronizing on read. Read-write and write-write still has synchronization. Bug: 1500280 Change-Id: I90f68dcd0ea11acbd6615a08d5a66ef6ceebe757 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5010573 Commit-Queue: Bo Liu Reviewed-by: Vasiliy Telezhnikov Cr-Commit-Position: refs/heads/main@{#1222583} --- .../ahardwarebuffer_image_backing_factory.cc | 11 +-- ...rebuffer_image_backing_factory_unittest.cc | 67 +++++++++++++++---- .../shared_image/android_image_backing.cc | 33 ++++++--- .../shared_image/android_image_backing.h | 5 ++ 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc b/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc index 32b0aeb8d9b534..521dfecac2239a 100644 --- a/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc +++ b/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc @@ -181,7 +181,8 @@ constexpr uint32_t kSupportedUsage = SHARED_IMAGE_USAGE_VIDEO_DECODE | SHARED_IMAGE_USAGE_WEBGPU_SWAP_CHAIN_TEXTURE | SHARED_IMAGE_USAGE_HIGH_PERFORMANCE_GPU | - SHARED_IMAGE_USAGE_WEBGPU_STORAGE_TEXTURE; + SHARED_IMAGE_USAGE_WEBGPU_STORAGE_TEXTURE | + SHARED_IMAGE_USAGE_CONCURRENT_READ_WRITE; } // namespace @@ -529,7 +530,7 @@ OverlayImage* AHardwareBufferImageBacking::BeginOverlayAccess( DCHECK(!is_overlay_accessing_); - if (is_writing_) { + if (!allow_concurrent_read_write() && is_writing_) { LOG(ERROR) << "BeginOverlayAccess should only be called when there are no writers"; return nullptr; @@ -556,8 +557,10 @@ void AHardwareBufferImageBacking::EndOverlayAccess() { DCHECK(is_overlay_accessing_); is_overlay_accessing_ = false; - auto fence_fd = overlay_image_->TakeEndFence(); - read_sync_fd_ = gl::MergeFDs(std::move(read_sync_fd_), std::move(fence_fd)); + if (!allow_concurrent_read_write()) { + auto fence_fd = overlay_image_->TakeEndFence(); + read_sync_fd_ = gl::MergeFDs(std::move(read_sync_fd_), std::move(fence_fd)); + } } // static diff --git a/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory_unittest.cc b/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory_unittest.cc index 690e1239b24550..57bb519cbbccae 100644 --- a/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory_unittest.cc +++ b/gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory_unittest.cc @@ -55,6 +55,7 @@ class GlLegacySharedImage { GlLegacySharedImage( SharedImageBackingFactory* backing_factory, bool is_thread_safe, + bool concurrent_read_write, SharedImageManager* shared_image_manager, MemoryTypeTracker* memory_type_tracker, SharedImageRepresentationFactory* shared_image_representation_factory); @@ -73,8 +74,9 @@ class GlLegacySharedImage { // Basic test to check creation and deletion of AHB backed shared image. TEST_F(AHardwareBufferImageBackingFactoryTest, Basic) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/false, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/false, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; // Validate a SkiaImageRepresentation. auto skia_representation = shared_image_representation_factory_.ProduceSkia( @@ -270,8 +272,9 @@ TEST_F(AHardwareBufferImageBackingFactoryTest, EstimatedSize) { // Test to check that only one context can write at a time TEST_F(AHardwareBufferImageBackingFactoryTest, OnlyOneWriter) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/true, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/true, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; auto skia_representation = shared_image_representation_factory_.ProduceSkia( gl_legacy_shared_image.mailbox(), context_state_.get()); @@ -308,8 +311,9 @@ TEST_F(AHardwareBufferImageBackingFactoryTest, OnlyOneWriter) { // Test to check that multiple readers are allowed TEST_F(AHardwareBufferImageBackingFactoryTest, CanHaveMultipleReaders) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/true, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/true, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; auto skia_representation = shared_image_representation_factory_.ProduceSkia( gl_legacy_shared_image.mailbox(), context_state_.get()); @@ -342,8 +346,9 @@ TEST_F(AHardwareBufferImageBackingFactoryTest, CanHaveMultipleReaders) { // Test to check that a context cannot write while another context is reading TEST_F(AHardwareBufferImageBackingFactoryTest, CannotWriteWhileReading) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/true, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/true, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; auto skia_representation = shared_image_representation_factory_.ProduceSkia( gl_legacy_shared_image.mailbox(), context_state_.get()); @@ -381,8 +386,9 @@ TEST_F(AHardwareBufferImageBackingFactoryTest, CannotWriteWhileReading) { // Test to check that a context cannot read while another context is writing TEST_F(AHardwareBufferImageBackingFactoryTest, CannotReadWhileWriting) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/true, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/true, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; auto skia_representation = shared_image_representation_factory_.ProduceSkia( gl_legacy_shared_image.mailbox(), context_state_.get()); @@ -413,9 +419,39 @@ TEST_F(AHardwareBufferImageBackingFactoryTest, CannotReadWhileWriting) { skia_representation.reset(); } +TEST_F(AHardwareBufferImageBackingFactoryTest, ConcurrentReadWrite) { + GlLegacySharedImage gl_legacy_shared_image{ + backing_factory_.get(), /*is_thread_safe=*/true, + /*concurrent_read_write=*/true, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; + + auto skia_representation = shared_image_representation_factory_.ProduceSkia( + gl_legacy_shared_image.mailbox(), context_state_.get()); + std::vector begin_semaphores; + std::vector end_semaphores; + std::unique_ptr scoped_read_access; + scoped_read_access = skia_representation->BeginScopedReadAccess( + &begin_semaphores, &end_semaphores); + EXPECT_TRUE(scoped_read_access); + EXPECT_EQ(0u, begin_semaphores.size()); + EXPECT_EQ(0u, end_semaphores.size()); + + auto skia_representation2 = shared_image_representation_factory_.ProduceSkia( + gl_legacy_shared_image.mailbox(), context_state_.get()); + std::unique_ptr + scoped_write_access; + scoped_write_access = skia_representation2->BeginScopedWriteAccess( + &begin_semaphores, &end_semaphores, + SharedImageRepresentation::AllowUnclearedAccess::kYes); + EXPECT_TRUE(scoped_write_access); + EXPECT_EQ(0u, begin_semaphores.size()); + EXPECT_EQ(0u, end_semaphores.size()); +} + GlLegacySharedImage::GlLegacySharedImage( SharedImageBackingFactory* backing_factory, bool is_thread_safe, + bool concurrent_read_write, SharedImageManager* shared_image_manager, MemoryTypeTracker* memory_type_tracker, SharedImageRepresentationFactory* shared_image_representation_factory) @@ -431,8 +467,12 @@ GlLegacySharedImage::GlLegacySharedImage( // SHARED_IMAGE_USAGE_DISPLAY_READ for skia read and SHARED_IMAGE_USAGE_RASTER // for skia write. uint32_t usage = SHARED_IMAGE_USAGE_GLES2 | SHARED_IMAGE_USAGE_RASTER; - if (!is_thread_safe) + if (!is_thread_safe) { usage |= SHARED_IMAGE_USAGE_DISPLAY_READ; + } + if (concurrent_read_write) { + usage |= SHARED_IMAGE_USAGE_CONCURRENT_READ_WRITE; + } backing_ = backing_factory->CreateSharedImage( mailbox_, format, surface_handle, size_, color_space, surface_origin, alpha_type, usage, "TestLabel", is_thread_safe); @@ -466,8 +506,9 @@ GlLegacySharedImage::~GlLegacySharedImage() { TEST_F(AHardwareBufferImageBackingFactoryTest, Overlay) { GlLegacySharedImage gl_legacy_shared_image{ - backing_factory_.get(), /*is_thread_safe=*/false, &shared_image_manager_, - &memory_type_tracker_, &shared_image_representation_factory_}; + backing_factory_.get(), /*is_thread_safe=*/false, + /*concurrent_read_write=*/false, &shared_image_manager_, + &memory_type_tracker_, &shared_image_representation_factory_}; auto skia_representation = shared_image_representation_factory_.ProduceSkia( gl_legacy_shared_image.mailbox(), context_state_.get()); diff --git a/gpu/command_buffer/service/shared_image/android_image_backing.cc b/gpu/command_buffer/service/shared_image/android_image_backing.cc index d32476d3fee3e4..6b8551f82e20cb 100644 --- a/gpu/command_buffer/service/shared_image/android_image_backing.cc +++ b/gpu/command_buffer/service/shared_image/android_image_backing.cc @@ -36,15 +36,30 @@ AndroidImageBacking::~AndroidImageBacking() = default; bool AndroidImageBacking::BeginWrite(base::ScopedFD* fd_to_wait_on) { AutoLock auto_lock(this); - if (is_writing_ || !active_readers_.empty() || is_overlay_accessing_) { - LOG(ERROR) << "BeginWrite should only be called when there are no other " - "readers or writers"; + if (is_writing_) { + LOG(ERROR) + << "BeginWrite should only be called when there are no other writers"; + return false; + } + if (!allow_concurrent_read_write() && + (!active_readers_.empty() || is_overlay_accessing_)) { + LOG(ERROR) + << "BeginWrite should only be called when there are no other readers"; return false; } is_writing_ = true; - (*fd_to_wait_on) = - gl::MergeFDs(std::move(read_sync_fd_), std::move(write_sync_fd_)); + if (allow_concurrent_read_write()) { + if (write_sync_fd_.is_valid()) { + (*fd_to_wait_on) = + base::ScopedFD(HANDLE_EINTR(dup(write_sync_fd_.get()))); + } else { + fd_to_wait_on->reset(); + } + } else { + (*fd_to_wait_on) = + gl::MergeFDs(std::move(read_sync_fd_), std::move(write_sync_fd_)); + } return true; } @@ -67,7 +82,7 @@ bool AndroidImageBacking::BeginRead(const SharedImageRepresentation* reader, base::ScopedFD* fd_to_wait_on) { AutoLock auto_lock(this); - if (is_writing_) { + if (!allow_concurrent_read_write() && is_writing_) { LOG(ERROR) << "BeginRead should only be called when there are no writers"; return false; } @@ -99,8 +114,10 @@ void AndroidImageBacking::EndRead(const SharedImageRepresentation* reader, active_readers_.erase(reader); - read_sync_fd_ = - gl::MergeFDs(std::move(read_sync_fd_), std::move(end_read_fd)); + if (!allow_concurrent_read_write()) { + read_sync_fd_ = + gl::MergeFDs(std::move(read_sync_fd_), std::move(end_read_fd)); + } } base::ScopedFD AndroidImageBacking::TakeReadFence() { diff --git a/gpu/command_buffer/service/shared_image/android_image_backing.h b/gpu/command_buffer/service/shared_image/android_image_backing.h index f79f715a257a18..7a0cb55018fa6a 100644 --- a/gpu/command_buffer/service/shared_image/android_image_backing.h +++ b/gpu/command_buffer/service/shared_image/android_image_backing.h @@ -7,6 +7,7 @@ #include "base/containers/flat_set.h" #include "base/files/scoped_file.h" +#include "gpu/command_buffer/common/shared_image_usage.h" #include "gpu/command_buffer/service/shared_image/shared_image_backing.h" namespace gpu { @@ -37,6 +38,10 @@ class AndroidImageBacking : public ClearTrackingSharedImageBacking { base::ScopedFD TakeReadFence(); protected: + bool allow_concurrent_read_write() const { + return usage() & SHARED_IMAGE_USAGE_CONCURRENT_READ_WRITE; + } + // All reads and writes must wait for exiting writes to complete. base::ScopedFD write_sync_fd_ GUARDED_BY(lock_); bool is_writing_ GUARDED_BY(lock_) = false;