Never create staging buffer for shared buffer memory with CPU access This patch avoids creating staging buffer for the buffers created with `mappedAtCreation == true` on a shared buffer memory with CPU access (for example, created from a shared memory file handle). This patch also ensures that the buffer created from a shared buffer memory with `mappedAtCreation == true` won't be initialized to zero when `beginAccessDesc.initialized` is true, and will always be initialized to zero when `beginAccessDesc.initialized` is false. Bug: 386255678 Change-Id: I49313ef56320acd749131b97422fb8b3f0ab0bb9 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/321855 Commit-Queue: Shao, Jiawei <jiawei.shao@intel.com> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index 759b832..c5ae5e7 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp
@@ -594,6 +594,12 @@ void* ptr = GetMappedPointer(); DeviceBase* device = GetDevice(); + // Don't zero-initialize buffers created from shared buffer memory at creation time. + // They will be initialized in `BeginAccess()` based on the `initialized` flag in + // `wgpu::SharedBufferMemoryBeginAccessDescriptor`. + if (mSharedResourceMemoryContents != nullptr) { + return {}; + } if (device->IsToggleEnabled(Toggle::LazyClearResourceOnFirstUse) && !device->IsToggleEnabled(Toggle::DisableLazyClearForMappedAtCreationBuffer)) { // The staging buffer is created with `MappedAtCreation == true` and the main buffer will
diff --git a/src/dawn/native/SharedBufferMemory.cpp b/src/dawn/native/SharedBufferMemory.cpp index ba278e4..6be7879 100644 --- a/src/dawn/native/SharedBufferMemory.cpp +++ b/src/dawn/native/SharedBufferMemory.cpp
@@ -87,6 +87,10 @@ return ObjectType::SharedBufferMemory; } +bool SharedBufferMemoryBase::CanBeWrittenByCPU() const { + return (mProperties.usage & wgpu::BufferUsage::MapWrite) != 0; +} + wgpu::Status SharedBufferMemoryBase::APIGetProperties( SharedBufferMemoryProperties* properties) const { properties->usage = mProperties.usage; @@ -136,10 +140,9 @@ descriptor->usage, mProperties.usage); // Require MapWrite usage on the shared buffer memory when mappedAtCreation is true. - DAWN_INVALID_IF( - descriptor->mappedAtCreation && !(mProperties.usage & wgpu::BufferUsage::MapWrite), - "Buffer created from SharedBufferMemory with mappedAtCreation=true requires " - "the SharedBufferMemory to have MapWrite usage."); + DAWN_INVALID_IF(descriptor->mappedAtCreation && !CanBeWrittenByCPU(), + "Buffer created from SharedBufferMemory with mappedAtCreation=true requires " + "the SharedBufferMemory to have MapWrite usage."); // Validate that the buffer size does not exceed the shared buffer memory's size. DAWN_INVALID_IF(descriptor->size > mProperties.size,
diff --git a/src/dawn/native/SharedBufferMemory.h b/src/dawn/native/SharedBufferMemory.h index 300a11e..c0005f7 100644 --- a/src/dawn/native/SharedBufferMemory.h +++ b/src/dawn/native/SharedBufferMemory.h
@@ -57,6 +57,10 @@ ObjectType GetType() const override; + // Returns true if the underlying resource can be written by the CPU, detect by checking for + // MapWrite in the properties. + bool CanBeWrittenByCPU() const; + protected: SharedBufferMemoryBase(DeviceBase* device, StringView label,
diff --git a/src/dawn/native/SharedResourceMemory.cpp b/src/dawn/native/SharedResourceMemory.cpp index 878d149..fd203cf 100644 --- a/src/dawn/native/SharedResourceMemory.cpp +++ b/src/dawn/native/SharedResourceMemory.cpp
@@ -28,6 +28,7 @@ #include "src/dawn/native/SharedResourceMemory.h" #include <algorithm> +#include <span> #include <utility> #include "src/dawn/common/Enumerator.h" @@ -196,11 +197,19 @@ DAWN_CHECK(!resource->IsError()); resource->OnBeginAccess(); - // For buffers created with mappedAtCreation=true, MapAtCreation() already called - // SetInitialized(true). Don't override that with descriptor->initialized here, since - // the buffer is mapped and the user is about to write into it through the mapped range. if constexpr (std::is_same_v<Resource, BufferBase>) { - if (resource->GetState() != BufferBase::BufferState::MappedAtCreation) { + if (resource->GetState() == BufferBase::BufferState::MappedAtCreation) { + // MapAtCreation is where zero-initialization of mappedAtCreation buffers is done, but + // it is never called again after the initial buffer creation, so zero-initialize + // manually here. + if (!descriptor->initialized) { + void* ptr = resource->GetMappedPointer(); + DAWN_ASSERT(ptr != nullptr); + std::span<uint8_t> data(static_cast<uint8_t*>(ptr), resource->GetAllocatedSize()); + std::ranges::fill(data, uint8_t{0}); + } + resource->SetInitialized(true); + } else { resource->SetInitialized(descriptor->initialized); } } else {
diff --git a/src/dawn/native/d3d12/BufferD3D12.cpp b/src/dawn/native/d3d12/BufferD3D12.cpp index 29182a2..abd346d 100644 --- a/src/dawn/native/d3d12/BufferD3D12.cpp +++ b/src/dawn/native/d3d12/BufferD3D12.cpp
@@ -452,7 +452,17 @@ // staging buffer, and copied from the staging buffer to the GPU memory of the current // buffer in the unmap() call. // TODO(enga): Handle CPU-visible memory on UMA - return (GetInternalUsage() & wgpu::BufferUsage::MapWrite) != 0; + if ((GetInternalUsage() & wgpu::BufferUsage::MapWrite) != 0) { + return true; + } + // For shared buffer memory buffers, the underlying memory may be CPU writable even if the + // buffer's usage doesn't include MapWrite. Check the shared memory's properties. + if (auto* contents = GetSharedResourceMemoryContents()) { + if (auto sharedMemory = contents->GetSharedResourceMemory().Promote()) { + return static_cast<SharedBufferMemoryBase*>(sharedMemory.Get())->CanBeWrittenByCPU(); + } + } + return false; } MaybeError Buffer::MapInternal(bool isWrite, size_t offset, size_t size, const char* contextInfo) { @@ -490,7 +500,7 @@ // We will use a staging buffer for MapRead buffers instead so we just clear the staging // buffer and initialize the original buffer by copying the staging buffer to the original // buffer one the first time Unmap() is called. - DAWN_ASSERT((GetInternalUsage() & wgpu::BufferUsage::MapWrite) != 0); + DAWN_ASSERT(IsCPUWritableAtCreation()); // The buffers with mappedAtCreation == true will be initialized in // BufferBase::MapAtCreation().
diff --git a/src/dawn/tests/white_box/SharedBufferMemoryTests.cpp b/src/dawn/tests/white_box/SharedBufferMemoryTests.cpp index a38640d..3bca380 100644 --- a/src/dawn/tests/white_box/SharedBufferMemoryTests.cpp +++ b/src/dawn/tests/white_box/SharedBufferMemoryTests.cpp
@@ -29,6 +29,7 @@ #include <gtest/gtest.h> +#include <span> #include <vector> #include "src/dawn/tests/DawnTest.h" @@ -901,6 +902,73 @@ EXPECT_EQ(endState.initialized, true); } +// Test that creating a buffer from shared buffer memory with `mappedAtCreation = true` and no +// `MapWrite` usage correctly handles the `initialized` flag in `BeginAccess()`: +// - `initialized = true` preserves the original data in the buffer at untouched offsets. +// - `initialized = false` clears the entire buffer to zero regardless of the original data. +// Also verifies that the second BeginAccess (after the buffer is no longer MappedAtCreation) +// correctly preserves the data written during the first access. +TEST_P(SharedBufferMemoryTests, CreateBufferMappedAtCreationOnSharedBufferMemoryNoMapWriteUsage) { + constexpr uint32_t kInitialData = 0x12345678; + constexpr uint32_t kWrittenData = 0x9ABCDEF0; + constexpr uint32_t kTotalBufferSize = sizeof(uint32_t) * 2; + + auto runScenario = [&](bool initialized, uint32_t expectedAtOffset0) { + SCOPED_TRACE(testing::Message() << "initialized=" << initialized); + + wgpu::SharedBufferMemory memory = GetParam().mBackend->CreateSharedBufferMemory( + device, kStorageUsages, kTotalBufferSize, kInitialData); + + // `mappedAtCreation == true` is only allowed when the shared buffer memory is CPU + // accessible. + wgpu::SharedBufferMemoryProperties properties; + memory.GetProperties(&properties); + if (!(properties.usage & wgpu::BufferUsage::MapWrite)) { + return; + } + + wgpu::BufferDescriptor bufferDesc = {}; + bufferDesc.size = kTotalBufferSize; + bufferDesc.usage = kStorageUsages; + bufferDesc.mappedAtCreation = true; + wgpu::Buffer buffer = memory.CreateBuffer(&bufferDesc); + + // First access: buffer is in MappedAtCreation state. + wgpu::SharedBufferMemoryBeginAccessDescriptor beginAccessDesc = {}; + beginAccessDesc.initialized = initialized; + memory.BeginAccess(buffer, &beginAccessDesc); + + // Write kWrittenData at offset sizeof(uint32_t); leave offset 0 untouched. + uint32_t* mappedData = static_cast<uint32_t*>(buffer.GetMappedRange(0, kTotalBufferSize)); + ASSERT_NE(mappedData, nullptr); + std::span<uint32_t> mappedSpan(mappedData, kTotalBufferSize / sizeof(uint32_t)); + mappedSpan[1] = kWrittenData; + buffer.Unmap(); + + EXPECT_BUFFER_U32_EQ(expectedAtOffset0, buffer, 0); + EXPECT_BUFFER_U32_EQ(kWrittenData, buffer, sizeof(uint32_t)); + + wgpu::SharedBufferMemoryEndAccessState endState = {}; + memory.EndAccess(buffer, &endState); + + // Second access: buffer is now in Unmapped state (no longer MappedAtCreation). + // Verify that the data written during the first access is preserved. + beginAccessDesc = {}; + beginAccessDesc.initialized = true; + memory.BeginAccess(buffer, &beginAccessDesc); + + EXPECT_BUFFER_U32_EQ(expectedAtOffset0, buffer, 0); + EXPECT_BUFFER_U32_EQ(kWrittenData, buffer, sizeof(uint32_t)); + + memory.EndAccess(buffer, &endState); + }; + + // `initialized = true`: the original data at offset 0 is preserved. + runScenario(true, kInitialData); + // `initialized = false`: the original data at offset 0 is cleared to zero. + runScenario(false, 0u); +} + GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(SharedBufferMemoryTests); } // anonymous namespace