From b0ca30280ae9888d86b34714fa70ce6c2be15815 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Tue, 9 Feb 2021 20:24:04 +0000 Subject: [PATCH] Skip testing of SNORM formats where non-readable. SNORM textures are non-renderable on vanilla ES, which means they're also non-readable. Use the EXT_render_snorm extension where available, otherwise skip the test for now. Bug: dawn:624 dawn:636 dawn:647 dawn:667 Change-Id: Ic50368032d6168060b6b52889b4ba952ce662f02 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/40420 Reviewed-by: Corentin Wallez Commit-Queue: Stephen White --- src/dawn_native/Toggles.cpp | 4 ++ src/dawn_native/Toggles.h | 1 + src/dawn_native/opengl/CommandBufferGL.cpp | 12 +++++- src/dawn_native/opengl/DeviceGL.cpp | 4 ++ .../end2end/NonzeroTextureCreationTests.cpp | 8 ++++ src/tests/end2end/StorageTextureTests.cpp | 40 +++++++++++-------- src/tests/end2end/TextureZeroInitTests.cpp | 19 ++++++--- 7 files changed, 63 insertions(+), 25 deletions(-) diff --git a/src/dawn_native/Toggles.cpp b/src/dawn_native/Toggles.cpp index 23506698d1..6ea53e5891 100644 --- a/src/dawn_native/Toggles.cpp +++ b/src/dawn_native/Toggles.cpp @@ -125,6 +125,10 @@ namespace dawn_native { "Disables the use of indexed draw buffer state which is unsupported on some " "platforms.", "https://crbug.com/dawn/582"}}, + {Toggle::DisableSnormRead, + {"disable_snorm_read", + "Disables reading from Snorm textures which is unsupported on some platforms.", + "https://crbug.com/dawn/667"}}, {Toggle::UseD3D12SmallShaderVisibleHeapForTesting, {"use_d3d12_small_shader_visible_heap", "Enable use of a small D3D12 shader visible heap, instead of using a large one by " diff --git a/src/dawn_native/Toggles.h b/src/dawn_native/Toggles.h index 2ecf5e0785..8ec9355302 100644 --- a/src/dawn_native/Toggles.h +++ b/src/dawn_native/Toggles.h @@ -40,6 +40,7 @@ namespace dawn_native { DisableBaseVertex, DisableBaseInstance, DisableIndexedDrawBuffers, + DisableSnormRead, UseD3D12SmallShaderVisibleHeapForTesting, UseDXC, DisableRobustness, diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index c1d7cfeb74..b6d9725639 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -519,6 +519,11 @@ namespace dawn_native { namespace opengl { gl.DeleteFramebuffers(1, &readFBO); gl.DeleteFramebuffers(1, &drawFBO); } + bool TextureFormatIsSnorm(wgpu::TextureFormat format) { + return format == wgpu::TextureFormat::RGBA8Snorm || + format == wgpu::TextureFormat::RG8Snorm || + format == wgpu::TextureFormat::R8Snorm; + } } // namespace CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) @@ -752,9 +757,12 @@ namespace dawn_native { namespace opengl { const GLFormat& format = texture->GetGLFormat(); GLenum target = texture->GetGLTarget(); - // TODO(jiawei.shao@intel.com): support texture-to-buffer copy with compressed + // TODO(crbug.com/dawn/667): Implement validation in WebGPU/Compat to + // avoid this codepath. OpenGL does not support readback from non-renderable // texture formats. - if (formatInfo.isCompressed) { + if (formatInfo.isCompressed || + (TextureFormatIsSnorm(formatInfo.format) && + GetDevice()->IsToggleEnabled(Toggle::DisableSnormRead))) { UNREACHABLE(); } diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index bc64d4549f..49256a2bf6 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -68,6 +68,9 @@ namespace dawn_native { namespace opengl { // TODO(crbug.com/dawn/582): Use OES_draw_buffers_indexed where available. bool supportsIndexedDrawBuffers = gl.IsAtLeastGLES(3, 2) || gl.IsAtLeastGL(3, 0); + bool supportsSnormRead = + gl.IsAtLeastGL(4, 4) || gl.IsGLExtensionSupported("GL_EXT_render_snorm"); + // TODO(crbug.com/dawn/343): We can support the extension variants, but need to load the EXT // procs without the extension suffix. // We'll also need emulation of shader builtins gl_BaseVertex and gl_BaseInstance. @@ -86,6 +89,7 @@ namespace dawn_native { namespace opengl { SetToggle(Toggle::DisableBaseVertex, !supportsBaseVertex); SetToggle(Toggle::DisableBaseInstance, !supportsBaseInstance); SetToggle(Toggle::DisableIndexedDrawBuffers, !supportsIndexedDrawBuffers); + SetToggle(Toggle::DisableSnormRead, !supportsSnormRead); SetToggle(Toggle::FlushBeforeClientWaitSync, gl.GetVersion().IsES()); } diff --git a/src/tests/end2end/NonzeroTextureCreationTests.cpp b/src/tests/end2end/NonzeroTextureCreationTests.cpp index efb301bd10..20e8ed2078 100644 --- a/src/tests/end2end/NonzeroTextureCreationTests.cpp +++ b/src/tests/end2end/NonzeroTextureCreationTests.cpp @@ -117,6 +117,10 @@ TEST_P(NonzeroTextureCreationTests, ArrayLayerClears) { // Test that nonrenderable texture formats clear 0x01 because toggle is enabled TEST_P(NonzeroTextureCreationTests, NonrenderableTextureFormat) { + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); + wgpu::TextureDescriptor descriptor; descriptor.dimension = wgpu::TextureDimension::e2D; descriptor.size.width = kSize; @@ -151,6 +155,10 @@ TEST_P(NonzeroTextureCreationTests, NonrenderableTextureFormat) { // Test that textures with more than 1 array layers and nonrenderable texture formats clear to 0x01 // because toggle is enabled TEST_P(NonzeroTextureCreationTests, NonRenderableTextureClearWithMultiArrayLayers) { + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); + wgpu::TextureDescriptor descriptor; descriptor.dimension = wgpu::TextureDimension::e2D; descriptor.size.width = kSize; diff --git a/src/tests/end2end/StorageTextureTests.cpp b/src/tests/end2end/StorageTextureTests.cpp index 19ad706729..662fde5ff5 100644 --- a/src/tests/end2end/StorageTextureTests.cpp +++ b/src/tests/end2end/StorageTextureTests.cpp @@ -770,8 +770,9 @@ TEST_P(StorageTextureTests, ReadonlyStorageTextureInVertexShader) { // Test that read-only storage textures are supported in fragment shader. TEST_P(StorageTextureTests, ReadonlyStorageTextureInFragmentShader) { - // TODO(crbug.com/dawn/624): this test fails on GLES. Investigate why. - DAWN_SKIP_TEST_IF(IsOpenGLES()); + // TODO(crbug.com/dawn/672): Investigate why this test fails on Linux + // NVidia OpenGLES drivers. + DAWN_SKIP_TEST_IF(IsNvidia() && IsLinux() && IsOpenGLES()); for (wgpu::TextureFormat format : utils::kAllTextureFormats) { if (!utils::TextureFormatSupportsStorageTexture(format)) { @@ -807,20 +808,22 @@ TEST_P(StorageTextureTests, ReadonlyStorageTextureInFragmentShader) { // Test that write-only storage textures are supported in compute shader. TEST_P(StorageTextureTests, WriteonlyStorageTextureInComputeShader) { - // TODO(crbug.com/dawn/647): diagnose and fix this OpenGL ES failure. - DAWN_SKIP_TEST_IF(IsOpenGLES()); - for (wgpu::TextureFormat format : utils::kAllTextureFormats) { if (!utils::TextureFormatSupportsStorageTexture(format)) { continue; } - if (!OpenGLESSupportsStorageTexture(format)) { + if (IsOpenGLES() && !OpenGLESSupportsStorageTexture(format)) { continue; } - // TODO(jiawei.shao@intel.com): investigate why this test fails with RGBA8Snorm on Linux - // Intel OpenGL driver. - if (format == wgpu::TextureFormat::RGBA8Snorm && IsIntel() && IsOpenGL() && IsLinux()) { + if (format == wgpu::TextureFormat::RGBA8Snorm && HasToggleEnabled("disable_snorm_read")) { + continue; + } + + // TODO(crbug.com/dawn/676): investigate why this test fails with RGBA8Snorm on Linux + // Intel OpenGL and OpenGLES drivers. + if (format == wgpu::TextureFormat::RGBA8Snorm && IsIntel() && + (IsOpenGL() || IsOpenGLES()) && IsLinux()) { continue; } @@ -878,8 +881,9 @@ TEST_P(StorageTextureTests, ReadWriteDifferentStorageTextureInOneDispatchInCompu // Test that write-only storage textures are supported in fragment shader. TEST_P(StorageTextureTests, WriteonlyStorageTextureInFragmentShader) { - // TODO(crbug.com/dawn/647): diagnose and fix this OpenGL ES failure. - DAWN_SKIP_TEST_IF(IsOpenGLES()); + // TODO(crbug.com/dawn/672): Investigate why this test fails on Linux + // NVidia OpenGLES drivers. + DAWN_SKIP_TEST_IF(IsNvidia() && IsLinux() && IsOpenGLES()); for (wgpu::TextureFormat format : utils::kAllTextureFormats) { if (!utils::TextureFormatSupportsStorageTexture(format)) { @@ -889,9 +893,14 @@ TEST_P(StorageTextureTests, WriteonlyStorageTextureInFragmentShader) { continue; } - // TODO(jiawei.shao@intel.com): investigate why this test fails with RGBA8Snorm on Linux - // Intel OpenGL driver. - if (format == wgpu::TextureFormat::RGBA8Snorm && IsIntel() && IsOpenGL() && IsLinux()) { + if (format == wgpu::TextureFormat::RGBA8Snorm && HasToggleEnabled("disable_snorm_read")) { + continue; + } + + // TODO(crbug.com/dawn/676): investigate why this test fails with RGBA8Snorm on Linux + // Intel OpenGL and OpenGLES drivers. + if (format == wgpu::TextureFormat::RGBA8Snorm && IsIntel() && + (IsOpenGL() || IsOpenGLES()) && IsLinux()) { continue; } @@ -1040,9 +1049,6 @@ TEST_P(StorageTextureTests, ReadonlyAndWriteonlyStorageTexturePingPong) { // Test that multiple dispatches to increment values by ping-ponging between a sampled texture and // a write-only storage texture are synchronized in one pass. TEST_P(StorageTextureTests, SampledAndWriteonlyStorageTexturePingPong) { - // TODO(crbug.com/dawn/636): diagnose and fix this failure on OpenGL ES - DAWN_SKIP_TEST_IF(IsOpenGLES()); - constexpr wgpu::TextureFormat kTextureFormat = wgpu::TextureFormat::R32Uint; wgpu::Texture storageTexture1 = CreateTexture( kTextureFormat, diff --git a/src/tests/end2end/TextureZeroInitTests.cpp b/src/tests/end2end/TextureZeroInitTests.cpp index ff8e3bedb0..139392f8c9 100644 --- a/src/tests/end2end/TextureZeroInitTests.cpp +++ b/src/tests/end2end/TextureZeroInitTests.cpp @@ -1017,8 +1017,9 @@ TEST_P(TextureZeroInitTest, ComputePassSampledTextureClear) { // This tests that the code path of CopyTextureToBuffer clears correctly for non-renderable textures TEST_P(TextureZeroInitTest, NonRenderableTextureClear) { - // TODO(crbug.com/dawn/660): Diagnose and fix this failure on SwANGLE. - DAWN_SKIP_TEST_IF(IsANGLE()); + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); wgpu::TextureDescriptor descriptor = CreateTextureDescriptor(1, 1, wgpu::TextureUsage::CopySrc, kNonrenderableColorFormat); @@ -1049,8 +1050,9 @@ TEST_P(TextureZeroInitTest, NonRenderableTextureClear) { // This tests that the code path of CopyTextureToBuffer clears correctly for non-renderable textures TEST_P(TextureZeroInitTest, NonRenderableTextureClearUnalignedSize) { - // TODO(crbug.com/dawn/660): Diagnose and fix this failure on SwANGLE. - DAWN_SKIP_TEST_IF(IsANGLE()); + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); wgpu::TextureDescriptor descriptor = CreateTextureDescriptor(1, 1, wgpu::TextureUsage::CopySrc, kNonrenderableColorFormat); @@ -1084,8 +1086,9 @@ TEST_P(TextureZeroInitTest, NonRenderableTextureClearUnalignedSize) { // This tests that the code path of CopyTextureToBuffer clears correctly for non-renderable textures // with more than 1 array layers TEST_P(TextureZeroInitTest, NonRenderableTextureClearWithMultiArrayLayers) { - // TODO(crbug.com/dawn/660): Diagnose and fix this failure on SwANGLE. - DAWN_SKIP_TEST_IF(IsANGLE()); + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); wgpu::TextureDescriptor descriptor = CreateTextureDescriptor(1, 2, wgpu::TextureUsage::CopySrc, kNonrenderableColorFormat); @@ -1427,6 +1430,10 @@ TEST_P(TextureZeroInitTest, PreservesInitializedArrayLayer) { // This is a regression test for crbug.com/dawn/451 where the lazy texture // init path on D3D12 had a divide-by-zero exception in the copy split logic. TEST_P(TextureZeroInitTest, CopyTextureToBufferNonRenderableUnaligned) { + // TODO(crbug.com/dawn/667): Work around the fact that some platforms do not support reading + // from Snorm textures. + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_snorm_read")); + wgpu::TextureDescriptor descriptor; descriptor.size.width = kUnalignedSize; descriptor.size.height = kUnalignedSize;