diff --git a/BUILD.gn b/BUILD.gn index eb5664fd2d..c05f01ee91 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -660,6 +660,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 d619fa4bd6..4a68302bdd 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 22bd170f53..28653dc206 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 f2832d08a7..9cf38245af 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 0b5d40fe7f..e38f88eb1c 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..37722fd60a --- /dev/null +++ b/src/tests/end2end/DestroyBufferTests.cpp @@ -0,0 +1,133 @@ +// 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}); + } + + 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