From 9bf529ec9421dcd6a27b9d07fbe3edf6bea598d3 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Sat, 9 Mar 2019 00:06:38 +0000 Subject: [PATCH] Revert "Destroy backend implementation for Buffers" This reverts commit b6a80b321e8fa0b79d9a947656ea0ad649ed5a3c. Reason for revert: dawn_end2end_tests are failing on the Chromium GPU FYI bots. Example here: https://ci.chromium.org/p/chromium/builders/ci/Win10%20FYI%20Release%20%28NVIDIA%29/4226 Original change's description: > Destroy backend implementation for Buffers > > Destroy can be used to free the GPU memory associated with resources > without waiting for javascript garbage collection to occur. > The buffer is validated at submission to the queue. > So any buffer that has been destroyed before submission, will then > invalidate the submit and result in an error. > > Bug: dawn:46 > Change-Id: I40df56ce97baef01deea7552d7a6d40b558fc985 > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5320 > Reviewed-by: Kai Ninomiya > Commit-Queue: Natasha Lee TBR=cwallez@chromium.org,kainino@chromium.org,enga@chromium.org,rafael.cintron@microsoft.com,natlee@microsoft.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: dawn:46 Change-Id: Iadf37a8a6675c744207ec7daaa3fd2fde7da3714 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5480 Reviewed-by: Austin Eng Commit-Queue: Austin Eng --- 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 | 133 ----------------------- 14 files changed, 11 insertions(+), 179 deletions(-) delete mode 100644 src/tests/end2end/DestroyBufferTests.cpp diff --git a/BUILD.gn b/BUILD.gn index ac84461540..024119edcb 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -645,7 +645,6 @@ 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 4a68302bdd..d619fa4bd6 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -47,9 +47,6 @@ namespace dawn_native { void UnmapImpl() override { UNREACHABLE(); } - void DestroyImpl() override { - UNREACHABLE(); - } }; } // anonymous namespace @@ -235,7 +232,6 @@ 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 28653dc206..22bd170f53 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -76,7 +76,6 @@ 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 307118d89f..c24ce83a7d 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() { - DestroyImpl(); + ToBackend(GetDevice())->GetResourceAllocator()->Release(mResource); } uint32_t Buffer::GetD3D12Size() const { @@ -189,11 +189,6 @@ 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 9667085107..9398fb574b 100644 --- a/src/dawn_native/d3d12/BufferD3D12.h +++ b/src/dawn_native/d3d12/BufferD3D12.h @@ -42,7 +42,6 @@ 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 4cdbb5a650..1d9a0117be 100644 --- a/src/dawn_native/metal/BufferMTL.h +++ b/src/dawn_native/metal/BufferMTL.h @@ -37,7 +37,6 @@ 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 3e91b43fcc..6d0aa9c79c 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -31,7 +31,8 @@ namespace dawn_native { namespace metal { } Buffer::~Buffer() { - DestroyImpl(); + [mMtlBuffer release]; + mMtlBuffer = nil; } id Buffer::GetMTLBuffer() { @@ -61,11 +62,6 @@ 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 9cf38245af..f2832d08a7 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -226,9 +226,6 @@ 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 e38f88eb1c..0b5d40fe7f 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -146,7 +146,6 @@ 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 50e2d0b0f4..348126e670 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -27,10 +27,6 @@ namespace dawn_native { namespace opengl { glBufferData(GL_ARRAY_BUFFER, GetSize(), nullptr, GL_STATIC_DRAW); } - Buffer::~Buffer() { - DestroyImpl(); - } - GLuint Buffer::GetHandle() const { return mBuffer; } @@ -62,9 +58,4 @@ 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 5a2b4a5cce..ba84267cdc 100644 --- a/src/dawn_native/opengl/BufferGL.h +++ b/src/dawn_native/opengl/BufferGL.h @@ -26,7 +26,6 @@ namespace dawn_native { namespace opengl { class Buffer : public BufferBase { public: Buffer(Device* device, const BufferDescriptor* descriptor); - ~Buffer(); GLuint GetHandle() const; @@ -35,7 +34,6 @@ 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 e38a716ac9..3b7ef7820d 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -137,7 +137,14 @@ namespace dawn_native { namespace vulkan { } Buffer::~Buffer() { - DestroyImpl(); + Device* device = ToBackend(GetDevice()); + + device->GetMemoryAllocator()->Free(&mMemoryAllocation); + + if (mHandle != VK_NULL_HANDLE) { + device->GetFencedDeleter()->DeleteWhenUnused(mHandle); + mHandle = VK_NULL_HANDLE; + } } void Buffer::OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data) { @@ -218,15 +225,6 @@ 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 348f9be370..ee703b4067 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -44,7 +44,6 @@ 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 deleted file mode 100644 index 37722fd60a..0000000000 --- a/src/tests/end2end/DestroyBufferTests.cpp +++ /dev/null @@ -1,133 +0,0 @@ -// 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