From aae6bce1fbc8557716c8c92efc55dc8b7285417f Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Wed, 6 Apr 2022 01:21:43 +0000 Subject: [PATCH] Add regression test for crbug.com/1313172 This adds a test, and a toggle disable_resource_suballocation. This enables testing the behavior discovered in the bug without creating enormous resources. Bug: chromium:1313172 Change-Id: I779aad50c051e5022a9c85ebfbf33c18173a748f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/85861 Reviewed-by: Loko Kung Reviewed-by: Brandon Jones Commit-Queue: Austin Eng --- src/dawn/native/Toggles.cpp | 7 ++++ src/dawn/native/Toggles.h | 1 + .../d3d12/ResourceAllocatorManagerD3D12.cpp | 12 ++++--- .../vulkan/ResourceMemoryAllocatorVk.cpp | 3 +- src/dawn/tests/end2end/BufferTests.cpp | 32 +++++++++++++++++++ 5 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/dawn/native/Toggles.cpp b/src/dawn/native/Toggles.cpp index b3bdd10b8c..9b3a65517c 100644 --- a/src/dawn/native/Toggles.cpp +++ b/src/dawn/native/Toggles.cpp @@ -90,6 +90,13 @@ namespace dawn::native { "recently used resources local to the GPU. Turning this component off can cause " "allocation failures when application memory exceeds physical device memory.", "https://crbug.com/dawn/193"}}, + {Toggle::DisableResourceSuballocation, + {"disable_resource_suballocation", + "Force the backends to not perform resource suballocation. This may expose " + "allocation " + "patterns which would otherwise only occur with large or specific types of " + "resources.", + "https://crbug.com/1313172"}}, {Toggle::SkipValidation, {"skip_validation", "Skip expensive validation of Dawn commands.", "https://crbug.com/dawn/271"}}, diff --git a/src/dawn/native/Toggles.h b/src/dawn/native/Toggles.h index 87c32bf2b2..a45a82e3af 100644 --- a/src/dawn/native/Toggles.h +++ b/src/dawn/native/Toggles.h @@ -33,6 +33,7 @@ namespace dawn::native { UseD3D12ResourceHeapTier2, UseD3D12RenderPass, UseD3D12ResidencyManagement, + DisableResourceSuballocation, SkipValidation, VulkanUseD32S8, VulkanUseS8, diff --git a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp index 2be2dfe367..5ed9c1d1d4 100644 --- a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp +++ b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp @@ -199,11 +199,13 @@ namespace dawn::native::d3d12 { // For very small resources, it is inefficent to suballocate given the min. heap // size could be much larger then the resource allocation. // Attempt to satisfy the request using sub-allocation (placed resource in a heap). - ResourceHeapAllocation subAllocation; - DAWN_TRY_ASSIGN(subAllocation, CreatePlacedResource(heapType, resourceDescriptor, - optimizedClearValue, initialUsage)); - if (subAllocation.GetInfo().mMethod != AllocationMethod::kInvalid) { - return std::move(subAllocation); + if (!mDevice->IsToggleEnabled(Toggle::DisableResourceSuballocation)) { + ResourceHeapAllocation subAllocation; + DAWN_TRY_ASSIGN(subAllocation, CreatePlacedResource(heapType, resourceDescriptor, + optimizedClearValue, initialUsage)); + if (subAllocation.GetInfo().mMethod != AllocationMethod::kInvalid) { + return std::move(subAllocation); + } } // If sub-allocation fails, fall-back to direct allocation (committed resource). diff --git a/src/dawn/native/vulkan/ResourceMemoryAllocatorVk.cpp b/src/dawn/native/vulkan/ResourceMemoryAllocatorVk.cpp index 6088ac0643..783300f0d8 100644 --- a/src/dawn/native/vulkan/ResourceMemoryAllocatorVk.cpp +++ b/src/dawn/native/vulkan/ResourceMemoryAllocatorVk.cpp @@ -134,7 +134,8 @@ namespace dawn::native::vulkan { // Sub-allocate non-mappable resources because at the moment the mapped pointer // is part of the resource and not the heap, which doesn't match the Vulkan model. // TODO(crbug.com/dawn/849): allow sub-allocating mappable resources, maybe. - if (requirements.size < kMaxSizeForSubAllocation && kind != MemoryKind::LinearMappable) { + if (requirements.size < kMaxSizeForSubAllocation && kind != MemoryKind::LinearMappable && + !mDevice->IsToggleEnabled(Toggle::DisableResourceSuballocation)) { // When sub-allocating, Vulkan requires that we respect bufferImageGranularity. Some // hardware puts information on the memory's page table entry and allocating a linear // resource in the same page as a non-linear (aka opaque) resource can cause issues. diff --git a/src/dawn/tests/end2end/BufferTests.cpp b/src/dawn/tests/end2end/BufferTests.cpp index 9debcc29f1..0d41e22cdf 100644 --- a/src/dawn/tests/end2end/BufferTests.cpp +++ b/src/dawn/tests/end2end/BufferTests.cpp @@ -908,3 +908,35 @@ DAWN_INSTANTIATE_TEST(BufferTests, OpenGLBackend(), OpenGLESBackend(), VulkanBackend()); + +class BufferNoSuballocationTests : public DawnTest {}; + +// Regression test for crbug.com/1313172 +// This tests a buffer. It then performs writeBuffer and immediately destroys +// it. Though writeBuffer references a destroyed buffer, it should not crash. +TEST_P(BufferNoSuballocationTests, WriteBufferThenDestroy) { + uint32_t myData = 0x01020304; + + wgpu::BufferDescriptor desc; + desc.size = 1024; + desc.usage = wgpu::BufferUsage::CopyDst; + wgpu::Buffer buffer = device.CreateBuffer(&desc); + + // Enqueue a pending write into the buffer. + constexpr size_t kSize = sizeof(myData); + queue.WriteBuffer(buffer, 0, &myData, kSize); + + // Destroy the buffer. + buffer.Destroy(); + + // Flush and wait for all commands. + queue.Submit(0, nullptr); + WaitForAllOperations(); +} + +DAWN_INSTANTIATE_TEST(BufferNoSuballocationTests, + D3D12Backend({"disable_resource_suballocation"}), + MetalBackend({"disable_resource_suballocation"}), + OpenGLBackend({"disable_resource_suballocation"}), + OpenGLESBackend({"disable_resource_suballocation"}), + VulkanBackend({"disable_resource_suballocation"}));