From 69b7f4a61c86ae66c673dc9557b4717225888f95 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Thu, 15 Dec 2022 00:52:53 +0000 Subject: [PATCH] Add more tests for texture corruption: msaa and depth stencil formats 2D array textures with non-color formats like depth/stencil formats are always fine. Workaround is not needed. Multisample textures are treated as array textures from the perspective of texture memory layout on Intel Gen12 and each sample acts like a layer. However, multisample textures are fine. Workaround is not needed. Bug: dawn:1507 Change-Id: I1e5cd6a4e46503f67e4c1ffe2133e2e8fb121016 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113740 Reviewed-by: Austin Eng Kokoro: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Austin Eng --- src/dawn/native/Toggles.cpp | 12 +-- src/dawn/native/Toggles.h | 2 +- src/dawn/native/d3d12/DeviceD3D12.cpp | 2 +- .../d3d12/ResourceAllocatorManagerD3D12.cpp | 40 +++++----- .../d3d12/ResourceAllocatorManagerD3D12.h | 2 +- src/dawn/tests/DawnTest.cpp | 26 ++++++ src/dawn/tests/DawnTest.h | 15 ++++ .../tests/end2end/TextureCorruptionTests.cpp | 80 ++++++++++++++++--- 8 files changed, 139 insertions(+), 40 deletions(-) diff --git a/src/dawn/native/Toggles.cpp b/src/dawn/native/Toggles.cpp index 869b5dce9f..21427db473 100644 --- a/src/dawn/native/Toggles.cpp +++ b/src/dawn/native/Toggles.cpp @@ -283,12 +283,12 @@ static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = {{ "Toggle is enabled by default on the D3D12 platforms where CastingFullyTypedFormatSupported " "is false.", "https://crbug.com/dawn/1276"}}, - {Toggle::D3D12AllocateExtraMemoryFor2DArrayTexture, - {"d3d12_allocate_extra_memory_for_2d_array_texture", - "Memory allocation for 2D array texture may be smaller than it should be on D3D12 on some " - "Intel devices. So texture access can be out-of-bound, which may cause critical security " - "issue. We can workaround this security issue via allocating extra memory and limiting its " - "access in itself.", + {Toggle::D3D12AllocateExtraMemoryFor2DArrayColorTexture, + {"d3d12_allocate_extra_memory_for_2d_array_color_texture", + "Memory allocation for 2D array color texture may be smaller than it should be on D3D12 on " + "some Intel devices. So texture access can be out-of-bound, which may cause critical " + "security issue. We can workaround this security issue via allocating extra memory and " + "limiting its access in itself.", "https://crbug.com/dawn/949"}}, {Toggle::D3D12UseTempBufferInDepthStencilTextureAndBufferCopyWithNonZeroBufferOffset, {"d3d12_use_temp_buffer_in_depth_stencil_texture_and_buffer_copy_with_non_zero_buffer_offset", diff --git a/src/dawn/native/Toggles.h b/src/dawn/native/Toggles.h index d80138f94c..4f97b7138c 100644 --- a/src/dawn/native/Toggles.h +++ b/src/dawn/native/Toggles.h @@ -76,7 +76,7 @@ enum class Toggle { D3D12ForceClearCopyableDepthStencilTextureOnCreation, D3D12DontSetClearValueOnDepthTextureCreation, D3D12AlwaysUseTypelessFormatsForCastableTexture, - D3D12AllocateExtraMemoryFor2DArrayTexture, + D3D12AllocateExtraMemoryFor2DArrayColorTexture, D3D12UseTempBufferInDepthStencilTextureAndBufferCopyWithNonZeroBufferOffset, ApplyClearBigIntegerColorValueWithDraw, MetalUseMockBlitEncoderForWriteTimestamp, diff --git a/src/dawn/native/d3d12/DeviceD3D12.cpp b/src/dawn/native/d3d12/DeviceD3D12.cpp index e8c6fe1179..646c58e292 100644 --- a/src/dawn/native/d3d12/DeviceD3D12.cpp +++ b/src/dawn/native/d3d12/DeviceD3D12.cpp @@ -724,7 +724,7 @@ void Device::InitTogglesFromDriver() { const gpu_info::DriverVersion kFixedDriverVersion = {30, 0, 101, 1692}; if (gpu_info::CompareWindowsDriverVersion(vendorId, GetAdapter()->GetDriverVersion(), kFixedDriverVersion) == -1) { - SetToggle(Toggle::D3D12AllocateExtraMemoryFor2DArrayTexture, true); + SetToggle(Toggle::D3D12AllocateExtraMemoryFor2DArrayColorTexture, true); } } diff --git a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp index 1a760e2ae2..5bcbaf1192 100644 --- a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp +++ b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp @@ -229,7 +229,7 @@ uint32_t ComputeExtraArraySizeForIntelGen12(uint32_t width, uint32_t arrayLayerCount, uint32_t mipLevelCount, uint32_t sampleCount, - uint32_t formatBytesPerBlock) { + uint32_t colorFormatBytesPerBlock) { // For details about texture memory layout on Intel Gen12 GPU, read // https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-tgl-vol05-memory_data_formats.pdf. // - Texture memory layout: from to @@ -243,9 +243,9 @@ uint32_t ComputeExtraArraySizeForIntelGen12(uint32_t width, // around the bug. constexpr uint32_t kTileSize = 16 * kPageSize; - // Tile's width and height vary according to format bit-wise (formatBytesPerBlock) + // Tile's width and height vary according to format bit-wise (colorFormatBytesPerBlock) uint32_t tileHeight = 0; - switch (formatBytesPerBlock) { + switch (colorFormatBytesPerBlock) { case 1: tileHeight = 256; break; @@ -270,7 +270,7 @@ uint32_t ComputeExtraArraySizeForIntelGen12(uint32_t width, uint32_t columnPitch = GetColumnPitch(height, mipLevelCount); - uint64_t totalWidth = width * formatBytesPerBlock; + uint64_t totalWidth = width * colorFormatBytesPerBlock; uint64_t totalHeight = columnPitch * layerxSamples; // Texture should be aligned on both tile width (512 bytes) and tile height (128 rows) on Intel @@ -280,17 +280,17 @@ uint32_t ComputeExtraArraySizeForIntelGen12(uint32_t width, uint64_t mainTileCount = mainTileCols * mainTileRows; // There is a bug in Intel old drivers to compute the auxiliary memory size (auxSize) of the - // texture, which is calculated from the main memory size (mainSize) of the texture. Note that - // memory allocation for mainSize itself is correct. But during memory allocation for auxSize, - // it re-caculated mainSize and did it in a wrong way. The incorrect algorithm doesn't respect - // alignment requirements from tile-based texture memory layout. It just simple aligned to a - // constant value (16K) for each sample and layer. + // color texture, which is calculated from the main memory size (mainSize) of the texture. Note + // that memory allocation for mainSize itself is correct. But during memory allocation for + // auxSize, it re-caculated mainSize and did it in a wrong way. The incorrect algorithm doesn't + // respect alignment requirements from tile-based texture memory layout. It just simple aligned + // to a constant value (16K) for each sample and layer. uint64_t expectedMainSize = mainTileCount * kTileSize; uint64_t actualMainSize = Align(columnPitch * totalWidth, kLinearAlignment) * layerxSamples; // If the incorrect mainSize calculation lead to less-than-expected auxSize, texture corruption - // is very likely to happen for any texture access like texture copy, rendering, sampling, etc. - // So we have to allocate a few more extra layers to offset the less-than-expected auxSize. + // is very likely to happen for any color texture access like texture copy, rendering, sampling, + // etc. So we have to allocate a few more extra layers to offset the less-than-expected auxSize. // However, it is fine if the incorrect mainSize calculation doesn't introduce less auxSize. For // example, if correct mainSize is 3.8M, it requires 4 pages of auxSize (16K). Any incorrect // mainSize between 3.0+ M and 4.0M also requires 16K auxSize according to the calculation: @@ -344,7 +344,7 @@ ResultOrError ResourceAllocatorManager::AllocateMemory( D3D12_HEAP_TYPE heapType, const D3D12_RESOURCE_DESC& resourceDescriptor, D3D12_RESOURCE_STATES initialUsage, - uint32_t formatBytesPerBlock, + uint32_t colorFormatBytesPerBlock, bool forceAllocateAsCommittedResource) { // In order to suppress a warning in the D3D12 debug layer, we need to specify an // optimized clear value. As there are no negative consequences when picking a mismatched @@ -357,16 +357,20 @@ ResultOrError ResourceAllocatorManager::AllocateMemory( optimizedClearValue = &zero; } - // If we are allocating memory for a 2D array texture on D3D12 backend, we need to allocate - // extra layers on some Intel Gen12 devices, see crbug.com/dawn/949 for details. + // If we are allocating memory for a 2D array texture with a color format on D3D12 backend, + // we need to allocate extra layers on some Intel Gen12 devices, see crbug.com/dawn/949 + // for details. D3D12_RESOURCE_DESC revisedDescriptor = resourceDescriptor; - if (mDevice->IsToggleEnabled(Toggle::D3D12AllocateExtraMemoryFor2DArrayTexture) && + if (mDevice->IsToggleEnabled(Toggle::D3D12AllocateExtraMemoryFor2DArrayColorTexture) && resourceDescriptor.Dimension == D3D12_RESOURCE_DIMENSION_TEXTURE2D && - resourceDescriptor.DepthOrArraySize > 1) { + resourceDescriptor.DepthOrArraySize > 1 && colorFormatBytesPerBlock > 0) { + // Multisample textures have one layer at most. Only non-multisample textures need the + // workaround. + ASSERT(revisedDescriptor.SampleDesc.Count <= 1); revisedDescriptor.DepthOrArraySize += ComputeExtraArraySizeForIntelGen12( resourceDescriptor.Width, resourceDescriptor.Height, resourceDescriptor.DepthOrArraySize, resourceDescriptor.MipLevels, - resourceDescriptor.SampleDesc.Count, formatBytesPerBlock); + resourceDescriptor.SampleDesc.Count, colorFormatBytesPerBlock); } // TODO(crbug.com/dawn/849): Conditionally disable sub-allocation. @@ -614,7 +618,7 @@ uint64_t ResourceAllocatorManager::GetResourcePadding( const D3D12_RESOURCE_DESC& resourceDescriptor) const { // If we are allocating memory for a 2D array texture on D3D12 backend, we need to allocate // extra memory on some devices, see crbug.com/dawn/949 for details. - if (mDevice->IsToggleEnabled(Toggle::D3D12AllocateExtraMemoryFor2DArrayTexture) && + if (mDevice->IsToggleEnabled(Toggle::D3D12AllocateExtraMemoryFor2DArrayColorTexture) && resourceDescriptor.Dimension == D3D12_RESOURCE_DIMENSION_TEXTURE2D && resourceDescriptor.DepthOrArraySize > 1) { return kExtraMemoryToMitigateTextureCorruption; diff --git a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h index 1904abe18d..5d9847398c 100644 --- a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h +++ b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h @@ -65,7 +65,7 @@ class ResourceAllocatorManager { D3D12_HEAP_TYPE heapType, const D3D12_RESOURCE_DESC& resourceDescriptor, D3D12_RESOURCE_STATES initialUsage, - uint32_t formatBytesPerBlock, + uint32_t colorFormatBytesPerBlock, bool forceAllocateAsCommittedResource = false); void DeallocateMemory(ResourceHeapAllocation& allocation); diff --git a/src/dawn/tests/DawnTest.cpp b/src/dawn/tests/DawnTest.cpp index e7638e01e2..7b3ebac002 100644 --- a/src/dawn/tests/DawnTest.cpp +++ b/src/dawn/tests/DawnTest.cpp @@ -1627,6 +1627,32 @@ testing::AssertionResult CheckImpl(const float& expected, } // namespace +template +ExpectConstant::ExpectConstant(T constant) : mConstant(constant) {} + +template +uint32_t ExpectConstant::DataSize() { + return sizeof(T); +} + +template +testing::AssertionResult ExpectConstant::Check(const void* data, size_t size) { + DAWN_ASSERT(size % DataSize() == 0 && size > 0); + const T* actual = static_cast(data); + + for (size_t i = 0; i < size / DataSize(); ++i) { + if (actual[i] != mConstant) { + return testing::AssertionFailure() + << "Expected data[" << i << "] to match constant value " << mConstant + << ", actual " << actual[i] << std::endl; + } + } + + return testing::AssertionSuccess(); +} + +template class ExpectConstant; + template testing::AssertionResult ExpectEq::Check(const void* data, size_t size) { DAWN_ASSERT(size == sizeof(U) * mExpected.size()); diff --git a/src/dawn/tests/DawnTest.h b/src/dawn/tests/DawnTest.h index a7832d4544..47046fedc9 100644 --- a/src/dawn/tests/DawnTest.h +++ b/src/dawn/tests/DawnTest.h @@ -134,6 +134,8 @@ namespace detail { class Expectation; class CustomTextureExpectation; +template +class ExpectConstant; template class ExpectEq; template @@ -780,6 +782,19 @@ class Expectation { virtual testing::AssertionResult Check(const void* data, size_t size) = 0; }; +template +class ExpectConstant : public Expectation { + public: + explicit ExpectConstant(T constant); + uint32_t DataSize(); + testing::AssertionResult Check(const void* data, size_t size) override; + + private: + T mConstant; +}; + +extern template class ExpectConstant; + // Expectation that checks the data is equal to some expected values. // T - expected value Type // U - actual value Type (defaults = T) diff --git a/src/dawn/tests/end2end/TextureCorruptionTests.cpp b/src/dawn/tests/end2end/TextureCorruptionTests.cpp index f08446af04..5f54b8e62d 100644 --- a/src/dawn/tests/end2end/TextureCorruptionTests.cpp +++ b/src/dawn/tests/end2end/TextureCorruptionTests.cpp @@ -42,6 +42,7 @@ constexpr wgpu::TextureFormat kDefaultFormat = wgpu::TextureFormat::RGBA8Unorm; constexpr uint32_t kDefaultHeight = 100u; constexpr uint32_t kDefaultArrayLayerCount = 2u; constexpr uint32_t kDefaultMipLevelCount = 1u; +constexpr uint32_t kDefaultSampleCount = 1u; constexpr WriteType kDefaultWriteType = WriteType::B2TCopy; std::ostream& operator<<(std::ostream& o, WriteType writeType) { @@ -73,6 +74,7 @@ using TextureWidth = uint32_t; using TextureHeight = uint32_t; using ArrayLayerCount = uint32_t; using MipLevelCount = uint32_t; +using SampleCount = uint32_t; DAWN_TEST_PARAM_STRUCT(TextureCorruptionTestsParams, TextureFormat, @@ -80,18 +82,20 @@ DAWN_TEST_PARAM_STRUCT(TextureCorruptionTestsParams, TextureHeight, ArrayLayerCount, MipLevelCount, + SampleCount, WriteType); } // namespace class TextureCorruptionTests : public DawnTestWithParams { protected: - std::ostringstream& DoSingleTest(wgpu::Texture texture, - const wgpu::Extent3D textureSize, - uint32_t depthOrArrayLayer, - uint32_t mipLevel, - uint32_t srcValue, - wgpu::TextureFormat format) { + virtual std::ostringstream& DoSingleTest(wgpu::Texture texture, + const wgpu::Extent3D textureSize, + uint32_t depthOrArrayLayer, + uint32_t mipLevel, + uint32_t sampleCount, + uint32_t srcValue, + wgpu::TextureFormat format) { wgpu::Extent3D levelSize = textureSize; if (mipLevel > 0) { levelSize.width = std::max(textureSize.width >> mipLevel, 1u); @@ -172,7 +176,7 @@ class TextureCorruptionTests : public DawnTestWithParams testedLayers = {0}; + if (sampleCount == 1) { + testedLayers.push_back(1); } // Most 2d-array textures being tested have only 2 layers. But if the texture has a lot of // layers, select a few layers to test. - std::vector testedLayers = {0, 1}; if (depthOrArrayLayerCount > 2) { + ASSERT(sampleCount == 1); uint32_t divider = 4; for (uint32_t i = 1; i <= divider; ++i) { int32_t testedLayer = depthOrArrayLayerCount * i / divider - 1; @@ -320,7 +334,8 @@ class TextureCorruptionTests : public DawnTestWithParams(0)); + } +}; + +TEST_P(TextureCorruptionTests_Multisample, Tests) { + DoTest(); +} + +DAWN_INSTANTIATE_TEST_P(TextureCorruptionTests_Multisample, + {D3D12Backend()}, + {kDefaultFormat}, + {100u, 200u, 300u, 400u, 500u, 600u, 700u, 800u, 900u, 1000u, 1200u}, + {100u, 200u}, + {1u}, + {kDefaultMipLevelCount}, + {4u}, + {WriteType::ClearTexture}); + class TextureCorruptionTests_WriteType : public TextureCorruptionTests {}; TEST_P(TextureCorruptionTests_WriteType, Tests) { @@ -404,6 +457,7 @@ DAWN_INSTANTIATE_TEST_P(TextureCorruptionTests_WriteType, {kDefaultHeight}, {kDefaultArrayLayerCount}, {kDefaultMipLevelCount}, + {kDefaultSampleCount}, {WriteType::ClearTexture, WriteType::WriteTexture, WriteType::B2TCopy, WriteType::RenderConstant, WriteType::RenderFromTextureSample, WriteType::RenderFromTextureLoad});