From 13f465015550d727279fc5dfd87a4e53b62b6a6b Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 11 Jun 2020 11:07:05 +0000 Subject: [PATCH] Add tests and fix (create) mapping zero-sized buffers When creating a zero-sized buffer mapped, StagingBuffer creation is skipped. This required adding a new MappedAtCreation state since mStagingBuffer couldn't be used as a tag value for that. Made the OpenGL backend always create non-zero-sized buffers. Finally added tests for MapRead/WriteAsync and CreateBufferMapped of zero-sized buffers. Bug: dawn:446 Change-Id: I04f6fe98fd646f1867c21065cd1cd33a1595e19f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/21481 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Corentin Wallez --- src/dawn_native/Buffer.cpp | 44 +++++++++---- src/dawn_native/Buffer.h | 1 + src/dawn_native/opengl/BufferGL.cpp | 6 +- src/dawn_wire/server/ServerBuffer.cpp | 1 + src/tests/end2end/BufferTests.cpp | 91 +++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 46d00d1f0b..4df366a714 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -163,14 +163,22 @@ namespace dawn_native { ASSERT(!IsError()); ASSERT(mappedPointer != nullptr); - mState = BufferState::Mapped; - + // Mappable buffers don't use a staging buffer and are just as if mapped through MapAsync. if (IsMapWritable()) { DAWN_TRY(MapAtCreationImpl(mappedPointer)); + mState = BufferState::Mapped; ASSERT(*mappedPointer != nullptr); return {}; } + mState = BufferState::MappedAtCreation; + + // 0-sized buffers are not supposed to be written to, Return back any non-null pointer. + if (mSize == 0) { + *mappedPointer = reinterpret_cast(intptr_t(0xCAFED00D)); + return {}; + } + // If any of these fail, the buffer will be deleted and replaced with an // error buffer. // TODO(enga): Suballocate and reuse memory from a larger staging buffer so we don't create @@ -190,6 +198,7 @@ namespace dawn_native { case BufferState::Destroyed: return DAWN_VALIDATION_ERROR("Destroyed buffer used in a submit"); case BufferState::Mapped: + case BufferState::MappedAtCreation: return DAWN_VALIDATION_ERROR("Buffer used in a submit while mapped"); case BufferState::Unmapped: return {}; @@ -309,11 +318,15 @@ namespace dawn_native { ASSERT(!IsError()); if (mState == BufferState::Mapped) { - if (mStagingBuffer == nullptr) { - Unmap(); + Unmap(); + } else if (mState == BufferState::MappedAtCreation) { + if (mStagingBuffer != nullptr) { + mStagingBuffer.reset(); + } else { + ASSERT(mSize == 0); } - mStagingBuffer.reset(); } + DestroyInternal(); } @@ -342,9 +355,7 @@ namespace dawn_native { } ASSERT(!IsError()); - if (mStagingBuffer != nullptr) { - GetDevice()->ConsumedError(CopyFromStagingBuffer()); - } else { + if (mState == BufferState::Mapped) { // A map request can only be called once, so this will fire only if the request wasn't // completed before the Unmap. // Callbacks are not fired if there is no callback registered, so this is correct for @@ -352,11 +363,20 @@ namespace dawn_native { CallMapReadCallback(mMapSerial, WGPUBufferMapAsyncStatus_Unknown, nullptr, 0u); CallMapWriteCallback(mMapSerial, WGPUBufferMapAsyncStatus_Unknown, nullptr, 0u); UnmapImpl(); + + mMapReadCallback = nullptr; + mMapWriteCallback = nullptr; + mMapUserdata = 0; + + } else if (mState == BufferState::MappedAtCreation) { + if (mStagingBuffer != nullptr) { + GetDevice()->ConsumedError(CopyFromStagingBuffer()); + } else { + ASSERT(mSize == 0); + } } + mState = BufferState::Unmapped; - mMapReadCallback = nullptr; - mMapWriteCallback = nullptr; - mMapUserdata = 0; } MaybeError BufferBase::ValidateMap(wgpu::BufferUsage requiredUsage, @@ -369,6 +389,7 @@ namespace dawn_native { switch (mState) { case BufferState::Mapped: + case BufferState::MappedAtCreation: return DAWN_VALIDATION_ERROR("Buffer already mapped"); case BufferState::Destroyed: return DAWN_VALIDATION_ERROR("Buffer is destroyed"); @@ -390,6 +411,7 @@ namespace dawn_native { switch (mState) { case BufferState::Mapped: + case BufferState::MappedAtCreation: // A buffer may be in the Mapped state if it was created with CreateBufferMapped // even if it did not have a mappable usage. return {}; diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index b0e45f9543..b05e341056 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -35,6 +35,7 @@ namespace dawn_native { enum class BufferState { Unmapped, Mapped, + MappedAtCreation, Destroyed, }; diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp index a7f880a8a8..dc9beef7de 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -22,9 +22,13 @@ namespace dawn_native { namespace opengl { Buffer::Buffer(Device* device, const BufferDescriptor* descriptor) : BufferBase(device, descriptor) { + // TODO(cwallez@chromium.org): Have a global "zero" buffer instead of creating a new 4-byte + // buffer? + uint64_t size = std::max(GetSize(), uint64_t(4u)); + device->gl.GenBuffers(1, &mBuffer); device->gl.BindBuffer(GL_ARRAY_BUFFER, mBuffer); - device->gl.BufferData(GL_ARRAY_BUFFER, GetSize(), nullptr, GL_STATIC_DRAW); + device->gl.BufferData(GL_ARRAY_BUFFER, size, nullptr, GL_STATIC_DRAW); } Buffer::~Buffer() { diff --git a/src/dawn_wire/server/ServerBuffer.cpp b/src/dawn_wire/server/ServerBuffer.cpp index 89ebacbbc6..84985129be 100644 --- a/src/dawn_wire/server/ServerBuffer.cpp +++ b/src/dawn_wire/server/ServerBuffer.cpp @@ -202,6 +202,7 @@ namespace dawn_wire { namespace server { // to Unmap and attempt to update mapped data of an error buffer. return false; } + // Deserialize the flush info and flush updated data from the handle into the target // of the handle. The target is set via WriteHandle::SetTarget. return buffer->writeHandle->DeserializeFlush(writeFlushInfo, diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp index 61f4e34b41..ceaf27cff8 100644 --- a/src/tests/end2end/BufferTests.cpp +++ b/src/tests/end2end/BufferTests.cpp @@ -108,6 +108,17 @@ TEST_P(BufferMapReadTests, LargeRead) { UnmapBuffer(buffer); } +// Test mapping a zero-sized buffer. +TEST_P(BufferMapReadTests, ZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + MapReadAsyncAndWait(buffer); + UnmapBuffer(buffer); +} + DAWN_INSTANTIATE_TEST(BufferMapReadTests, D3D12Backend(), MetalBackend(), OpenGLBackend(), VulkanBackend()); class BufferMapWriteTests : public DawnTest { @@ -202,6 +213,17 @@ TEST_P(BufferMapWriteTests, LargeWrite) { EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 0, kDataSize); } +// Test mapping a zero-sized buffer. +TEST_P(BufferMapWriteTests, ZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + MapWriteAsyncAndWait(buffer); + UnmapBuffer(buffer); +} + // Stress test mapping many buffers. TEST_P(BufferMapWriteTests, ManyWrites) { constexpr uint32_t kDataSize = 1000; @@ -370,6 +392,21 @@ TEST_P(CreateBufferMappedTests, NonMappableUsageLarge) { EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), result.buffer, 0, kDataSize); } +// Test destroying a non-mappable buffer mapped at creation. +// This is a regression test for an issue where the D3D12 backend thought the buffer was actually +// mapped and tried to unlock the heap residency (when actually the buffer was using a staging +// buffer) +TEST_P(CreateBufferMappedTests, DestroyNonMappableWhileMappedForCreation) { + wgpu::CreateBufferMappedResult result = CreateBufferMapped(wgpu::BufferUsage::CopySrc, 4); + result.buffer.Destroy(); +} + +// Test destroying a mappable buffer mapped at creation. +TEST_P(CreateBufferMappedTests, DestroyMappableWhileMappedForCreation) { + wgpu::CreateBufferMappedResult result = CreateBufferMapped(wgpu::BufferUsage::MapRead, 4); + result.buffer.Destroy(); +} + // Test that mapping a buffer is valid after CreateBufferMapped and Unmap TEST_P(CreateBufferMappedTests, CreateThenMapSuccess) { static uint32_t myData = 230502; @@ -437,6 +474,48 @@ TEST_P(CreateBufferMappedTests, LargeBufferFails) { ASSERT_DEVICE_ERROR(device.CreateBuffer(&descriptor)); } +// Test that creating a zero-sized buffer mapped is allowed. +TEST_P(CreateBufferMappedTests, ZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::Vertex; + wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&descriptor); + + ASSERT_EQ(0u, result.dataLength); + ASSERT_NE(nullptr, result.data); + + // Check that unmapping the buffer works too. + UnmapBuffer(result.buffer); +} + +// Test that creating a zero-sized mapppable buffer mapped. (it is a different code path) +TEST_P(CreateBufferMappedTests, ZeroSizedMappableBuffer) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapWrite; + wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&descriptor); + + ASSERT_EQ(0u, result.dataLength); + ASSERT_NE(nullptr, result.data); + + // Check that unmapping the buffer works too. + UnmapBuffer(result.buffer); +} + +// Test that creating a zero-sized error buffer mapped. (it is a different code path) +TEST_P(CreateBufferMappedTests, ZeroSizedErrorBuffer) { + DAWN_SKIP_TEST_IF(IsDawnValidationSkipped()); + + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::Storage; + wgpu::CreateBufferMappedResult result; + ASSERT_DEVICE_ERROR(result = device.CreateBufferMapped(&descriptor)); + + ASSERT_EQ(0u, result.dataLength); + ASSERT_NE(nullptr, result.data); +} + DAWN_INSTANTIATE_TEST(CreateBufferMappedTests, D3D12Backend(), D3D12Backend({}, {"use_d3d12_resource_heap_tier2"}), @@ -446,6 +525,7 @@ DAWN_INSTANTIATE_TEST(CreateBufferMappedTests, class BufferTests : public DawnTest {}; +// Test that creating a zero-buffer is allowed. TEST_P(BufferTests, ZeroSizedBuffer) { wgpu::BufferDescriptor desc; desc.size = 0; @@ -453,6 +533,17 @@ TEST_P(BufferTests, ZeroSizedBuffer) { device.CreateBuffer(&desc); } +// Test that creating a very large buffers fails gracefully. +TEST_P(BufferTests, LargeBufferFails) { + // TODO(http://crbug.com/dawn/27): Missing support. + DAWN_SKIP_TEST_IF(IsOpenGL()); + + wgpu::BufferDescriptor descriptor; + descriptor.size = std::numeric_limits::max(); + descriptor.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst; + ASSERT_DEVICE_ERROR(device.CreateBuffer(&descriptor)); +} + DAWN_INSTANTIATE_TEST(BufferTests, D3D12Backend(), MetalBackend(),