Skip to content

Commit

Permalink
si: AHB backing concurrent-read-write support
Browse files Browse the repository at this point in the history 
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 <boliu@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1222583}
  • Loading branch information
Bo Liu authored and Chromium LUCI CQ committed Nov 9, 2023
1 parent e225fef commit 2d5cf02
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 25 deletions.
11 changes: 7 additions & 4 deletions gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down
67 changes: 54 additions & 13 deletions gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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<GrBackendSemaphore> begin_semaphores;
std::vector<GrBackendSemaphore> end_semaphores;
std::unique_ptr<SkiaImageRepresentation::ScopedReadAccess> 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<SkiaImageRepresentation::ScopedWriteAccess>
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)
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand Down
33 changes: 25 additions & 8 deletions gpu/command_buffer/service/shared_image/android_image_backing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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() {
Expand Down
5 changes: 5 additions & 0 deletions gpu/command_buffer/service/shared_image/android_image_backing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 2d5cf02

Please sign in to comment.