From 718e1dbb8949e2c45c42eda652d75997d3a37bfc Mon Sep 17 00:00:00 2001 From: Natasha Lee Date: Mon, 11 Mar 2019 17:05:22 +0000 Subject: [PATCH] Reland "Destroy backend implementation for Buffers" This reverts commit 9bf529ec9421dcd6a27b9d07fbe3edf6bea598d3. Reason for revert: Fixed test failure by submitting basic render pass to clear out texture before running the tests. The test was failing previously because the texture pixel color was not cleared before running the tests, causing unexpected pixel colors to be compared. Creating a basic render pass clears the texture, but since the first test fails on submit expectedly, the pixel is never cleared. Bug: dawn:46 Change-Id: Ic190c2d8d6af3f9d8def3370b92c6974a82a0096 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5500 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Commit-Queue: Natasha Lee --- BUILD.gn | 1 + src/dawn_native/Buffer.cpp | 4 + src/dawn_native/Buffer.h | 1 + src/dawn_native/d3d12/BufferD3D12.cpp | 7 +- src/dawn_native/d3d12/BufferD3D12.h | 1 + src/dawn_native/metal/BufferMTL.h | 1 + src/dawn_native/metal/BufferMTL.mm | 8 +- src/dawn_native/null/DeviceNull.cpp | 3 + src/dawn_native/null/DeviceNull.h | 1 + src/dawn_native/opengl/BufferGL.cpp | 9 ++ src/dawn_native/opengl/BufferGL.h | 2 + src/dawn_native/vulkan/BufferVk.cpp | 18 +-- src/dawn_native/vulkan/BufferVk.h | 1 + src/tests/end2end/DestroyBufferTests.cpp | 138 +++++++++++++++++++++++ 14 files changed, 184 insertions(+), 11 deletions(-) create mode 100644 src/tests/end2end/DestroyBufferTests.cpp diff --git a/BUILD.gn b/BUILD.gn index 024119edcb..ac84461540 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -645,6 +645,7 @@ test("dawn_end2end_tests") { "src/tests/end2end/CopyTests.cpp", "src/tests/end2end/DebugMarkerTests.cpp", "src/tests/end2end/DepthStencilStateTests.cpp", + "src/tests/end2end/DestroyBufferTests.cpp", "src/tests/end2end/DrawIndexedTests.cpp", "src/tests/end2end/DrawTests.cpp", "src/tests/end2end/FenceTests.cpp", diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index ad64ab4044..9cfcf03f73 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -47,6 +47,9 @@ namespace dawn_native { void UnmapImpl() override { UNREACHABLE(); } + void DestroyImpl() override { + UNREACHABLE(); + } }; } // anonymous namespace @@ -232,6 +235,7 @@ namespace dawn_native { if (mState == BufferState::Mapped) { Unmap(); } + DestroyImpl(); mState = BufferState::Destroyed; } diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index b4309da559..9c3d40281f 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -76,6 +76,7 @@ namespace dawn_native { virtual void MapReadAsyncImpl(uint32_t serial) = 0; virtual void MapWriteAsyncImpl(uint32_t serial) = 0; virtual void UnmapImpl() = 0; + virtual void DestroyImpl() = 0; MaybeError ValidateSetSubData(uint32_t start, uint32_t count) const; MaybeError ValidateMap(dawn::BufferUsageBit requiredUsage) const; diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index c24ce83a7d..307118d89f 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -105,7 +105,7 @@ namespace dawn_native { namespace d3d12 { } Buffer::~Buffer() { - ToBackend(GetDevice())->GetResourceAllocator()->Release(mResource); + DestroyImpl(); } uint32_t Buffer::GetD3D12Size() const { @@ -189,6 +189,11 @@ namespace dawn_native { namespace d3d12 { mWrittenMappedRange = {}; } + void Buffer::DestroyImpl() { + ToBackend(GetDevice())->GetResourceAllocator()->Release(mResource); + mResource = nullptr; + } + MapRequestTracker::MapRequestTracker(Device* device) : mDevice(device) { } diff --git a/src/dawn_native/d3d12/BufferD3D12.h b/src/dawn_native/d3d12/BufferD3D12.h index 9398fb574b..9667085107 100644 --- a/src/dawn_native/d3d12/BufferD3D12.h +++ b/src/dawn_native/d3d12/BufferD3D12.h @@ -42,6 +42,7 @@ namespace dawn_native { namespace d3d12 { void MapReadAsyncImpl(uint32_t serial) override; void MapWriteAsyncImpl(uint32_t serial) override; void UnmapImpl() override; + void DestroyImpl() override; ComPtr mResource; bool mFixedResourceState = false; diff --git a/src/dawn_native/metal/BufferMTL.h b/src/dawn_native/metal/BufferMTL.h index 1d9a0117be..4cdbb5a650 100644 --- a/src/dawn_native/metal/BufferMTL.h +++ b/src/dawn_native/metal/BufferMTL.h @@ -37,6 +37,7 @@ namespace dawn_native { namespace metal { void MapReadAsyncImpl(uint32_t serial) override; void MapWriteAsyncImpl(uint32_t serial) override; void UnmapImpl() override; + void DestroyImpl() override; id mMtlBuffer = nil; }; diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index 6d0aa9c79c..3e91b43fcc 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -31,8 +31,7 @@ namespace dawn_native { namespace metal { } Buffer::~Buffer() { - [mMtlBuffer release]; - mMtlBuffer = nil; + DestroyImpl(); } id Buffer::GetMTLBuffer() { @@ -62,6 +61,11 @@ namespace dawn_native { namespace metal { // Nothing to do, Metal StorageModeShared buffers are always mapped. } + void Buffer::DestroyImpl() { + [mMtlBuffer release]; + mMtlBuffer = nil; + } + MapRequestTracker::MapRequestTracker(Device* device) : mDevice(device) { } diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index b3983aaed7..693adedd4d 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -226,6 +226,9 @@ namespace dawn_native { namespace null { void Buffer::UnmapImpl() { } + void Buffer::DestroyImpl() { + } + // CommandBuffer CommandBuffer::CommandBuffer(Device* device, CommandEncoderBase* encoder) diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 2a504f8910..973fe13e7c 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -146,6 +146,7 @@ namespace dawn_native { namespace null { void MapReadAsyncImpl(uint32_t serial) override; void MapWriteAsyncImpl(uint32_t serial) override; void UnmapImpl() override; + void DestroyImpl() override; void MapAsyncImplCommon(uint32_t serial, bool isWrite); diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp index 348126e670..50e2d0b0f4 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -27,6 +27,10 @@ namespace dawn_native { namespace opengl { glBufferData(GL_ARRAY_BUFFER, GetSize(), nullptr, GL_STATIC_DRAW); } + Buffer::~Buffer() { + DestroyImpl(); + } + GLuint Buffer::GetHandle() const { return mBuffer; } @@ -58,4 +62,9 @@ namespace dawn_native { namespace opengl { glUnmapBuffer(GL_ARRAY_BUFFER); } + void Buffer::DestroyImpl() { + glDeleteBuffers(1, &mBuffer); + mBuffer = 0; + } + }} // namespace dawn_native::opengl diff --git a/src/dawn_native/opengl/BufferGL.h b/src/dawn_native/opengl/BufferGL.h index ba84267cdc..5a2b4a5cce 100644 --- a/src/dawn_native/opengl/BufferGL.h +++ b/src/dawn_native/opengl/BufferGL.h @@ -26,6 +26,7 @@ namespace dawn_native { namespace opengl { class Buffer : public BufferBase { public: Buffer(Device* device, const BufferDescriptor* descriptor); + ~Buffer(); GLuint GetHandle() const; @@ -34,6 +35,7 @@ namespace dawn_native { namespace opengl { void MapReadAsyncImpl(uint32_t serial) override; void MapWriteAsyncImpl(uint32_t serial) override; void UnmapImpl() override; + void DestroyImpl() override; GLuint mBuffer = 0; }; diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index 3b7ef7820d..e38a716ac9 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -137,14 +137,7 @@ namespace dawn_native { namespace vulkan { } Buffer::~Buffer() { - Device* device = ToBackend(GetDevice()); - - device->GetMemoryAllocator()->Free(&mMemoryAllocation); - - if (mHandle != VK_NULL_HANDLE) { - device->GetFencedDeleter()->DeleteWhenUnused(mHandle); - mHandle = VK_NULL_HANDLE; - } + DestroyImpl(); } void Buffer::OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data) { @@ -225,6 +218,15 @@ namespace dawn_native { namespace vulkan { // No need to do anything, we keep CPU-visible memory mapped at all time. } + void Buffer::DestroyImpl() { + ToBackend(GetDevice())->GetMemoryAllocator()->Free(&mMemoryAllocation); + + if (mHandle != VK_NULL_HANDLE) { + ToBackend(GetDevice())->GetFencedDeleter()->DeleteWhenUnused(mHandle); + mHandle = VK_NULL_HANDLE; + } + } + // MapRequestTracker MapRequestTracker::MapRequestTracker(Device* device) : mDevice(device) { diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h index ee703b4067..348f9be370 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -44,6 +44,7 @@ namespace dawn_native { namespace vulkan { void MapReadAsyncImpl(uint32_t serial) override; void MapWriteAsyncImpl(uint32_t serial) override; void UnmapImpl() override; + void DestroyImpl() override; VkBuffer mHandle = VK_NULL_HANDLE; DeviceMemoryAllocation mMemoryAllocation; diff --git a/src/tests/end2end/DestroyBufferTests.cpp b/src/tests/end2end/DestroyBufferTests.cpp new file mode 100644 index 0000000000..9226a73333 --- /dev/null +++ b/src/tests/end2end/DestroyBufferTests.cpp @@ -0,0 +1,138 @@ +// Copyright 2019 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tests/DawnTest.h" + +#include "utils/ComboRenderPipelineDescriptor.h" +#include "utils/DawnHelpers.h" + +constexpr uint32_t kRTSize = 4; + +class DestroyBufferTest : public DawnTest { + protected: + void SetUp() override { + DawnTest::SetUp(); + + renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize); + dawn::VertexInputDescriptor input; + input.inputSlot = 0; + input.stride = 4 * sizeof(float); + input.stepMode = dawn::InputStepMode::Vertex; + + dawn::VertexAttributeDescriptor attribute; + attribute.shaderLocation = 0; + attribute.inputSlot = 0; + attribute.offset = 0; + attribute.format = dawn::VertexFormat::FloatR32G32B32A32; + + dawn::InputState inputState = + device.CreateInputStateBuilder().SetInput(&input).SetAttribute(&attribute).GetResult(); + + dawn::ShaderModule vsModule = + utils::CreateShaderModule(device, dawn::ShaderStage::Vertex, R"( + #version 450 + layout(location = 0) in vec4 pos; + void main() { + gl_Position = pos; + })"); + + dawn::ShaderModule fsModule = + utils::CreateShaderModule(device, dawn::ShaderStage::Fragment, R"( + #version 450 + layout(location = 0) out vec4 fragColor; + void main() { + fragColor = vec4(0.0, 1.0, 0.0, 1.0); + })"); + + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.cVertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.primitiveTopology = dawn::PrimitiveTopology::TriangleStrip; + descriptor.indexFormat = dawn::IndexFormat::Uint32; + descriptor.inputState = inputState; + descriptor.cColorStates[0]->format = renderPass.colorFormat; + + pipeline = device.CreateRenderPipeline(&descriptor); + + vertexBuffer = utils::CreateBufferFromData( + device, dawn::BufferUsageBit::Vertex, + {// The bottom left triangle + -1.0f, -1.0f, 0.0f, 1.0f, 1.0f, 1.0f, 0.0f, 1.0f, -1.0f, 1.0f, 0.0f, 1.0f}); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.BeginRenderPass(&renderPass.renderPassInfo).EndPass(); + dawn::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + } + + utils::BasicRenderPass renderPass; + dawn::RenderPipeline pipeline; + dawn::Buffer vertexBuffer; + + dawn::CommandBuffer CreateTriangleCommandBuffer() { + uint32_t zeroOffset = 0; + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + { + dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); + pass.SetPipeline(pipeline); + pass.SetVertexBuffers(0, 1, &vertexBuffer, &zeroOffset); + pass.Draw(3, 1, 0, 0); + pass.EndPass(); + } + dawn::CommandBuffer commands = encoder.Finish(); + return commands; + } +}; + +// Destroy before submit will result in error, and nothing drawn +TEST_P(DestroyBufferTest, DestroyBeforeSubmit) { + RGBA8 notFilled(0, 0, 0, 0); + + dawn::CommandBuffer commands = CreateTriangleCommandBuffer(); + vertexBuffer.Destroy(); + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + + EXPECT_PIXEL_RGBA8_EQ(notFilled, renderPass.color, 1, 3); +} + +// Destroy after submit will draw successfully +TEST_P(DestroyBufferTest, DestroyAfterSubmit) { + RGBA8 filled(0, 255, 0, 255); + + dawn::CommandBuffer commands = CreateTriangleCommandBuffer(); + queue.Submit(1, &commands); + + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 1, 3); + vertexBuffer.Destroy(); +} + +// First submit succeeds, draws triangle, second submit fails +// after destroy is called on the buffer, pixel does not change +TEST_P(DestroyBufferTest, SubmitDestroySubmit) { + RGBA8 filled(0, 255, 0, 255); + + dawn::CommandBuffer commands = CreateTriangleCommandBuffer(); + queue.Submit(1, &commands); + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 1, 3); + + vertexBuffer.Destroy(); + + // Submit fails because vertex buffer was destroyed + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + + // Pixel stays the same + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 1, 3); +} + +DAWN_INSTANTIATE_TEST(DestroyBufferTest, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend); \ No newline at end of file