Add Dawn validation for filterability. Update the Dawn `ShaderModule` code to validate the filterability settings for bind-ful textures and samplers. Change-Id: Iec85fb2de65d73fa568a112d6c4b312732b4e0f4 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/291316 Commit-Queue: dan sinclair <dsinclair@chromium.org> Auto-Submit: dan sinclair <dsinclair@chromium.org> Commit-Queue: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn/native/BindingInfo.h b/src/dawn/native/BindingInfo.h index c097850..b2140fe 100644 --- a/src/dawn/native/BindingInfo.h +++ b/src/dawn/native/BindingInfo.h
@@ -81,11 +81,9 @@ #undef BUFFER_BINDING_INFO_MEMBER // A mirror of wgpu::TextureBindingLayout for use inside dawn::native. -#define TEXTURE_BINDING_INFO_MEMBER(X) \ - /* For shader reflection UnfilterableFloat is never used and the sample type is Float */ \ - /* for any texture_Nd<f32>. */ \ - X(wgpu::TextureSampleType, sampleType) \ - X(wgpu::TextureViewDimension, viewDimension) \ +#define TEXTURE_BINDING_INFO_MEMBER(X) \ + X(wgpu::TextureSampleType, sampleType) \ + X(wgpu::TextureViewDimension, viewDimension) \ X(bool, multisampled) DAWN_SERIALIZABLE(struct, TextureBindingInfo, TEXTURE_BINDING_INFO_MEMBER) { static TextureBindingInfo From(const TextureBindingLayout& layout); @@ -112,10 +110,7 @@ #undef TEXEL_BUFFER_BINDING_INFO_MEMBER // A mirror of wgpu::SamplerBindingLayout for use inside dawn::native. -#define SAMPLER_BINDING_INFO_MEMBER(X) \ - /* For shader reflection NonFiltering is never used and Filtering is used for */ \ - /* any `sampler`. */ \ - X(wgpu::SamplerBindingType, type) +#define SAMPLER_BINDING_INFO_MEMBER(X) X(wgpu::SamplerBindingType, type) DAWN_SERIALIZABLE(struct, SamplerBindingInfo, SAMPLER_BINDING_INFO_MEMBER) { static SamplerBindingInfo From(const SamplerBindingLayout& layout); };
diff --git a/src/dawn/native/PipelineLayout.cpp b/src/dawn/native/PipelineLayout.cpp index 3b94219..5a74fa1 100644 --- a/src/dawn/native/PipelineLayout.cpp +++ b/src/dawn/native/PipelineLayout.cpp
@@ -28,7 +28,6 @@ #include "dawn/native/PipelineLayout.h" #include <algorithm> -#include <map> #include <memory> #include <utility> @@ -37,9 +36,7 @@ #include "dawn/common/Enumerator.h" #include "dawn/common/MatchVariant.h" #include "dawn/common/Math.h" -#include "dawn/common/Numeric.h" #include "dawn/common/Range.h" -#include "dawn/common/ityp_stack_vec.h" #include "dawn/native/BindGroupLayout.h" #include "dawn/native/ChainUtils.h" #include "dawn/native/CommandValidation.h" @@ -334,7 +331,14 @@ entry.buffer.type = bindingInfo.type; entry.buffer.minBindingSize = bindingInfo.minBindingSize; }, - [&](const SamplerBindingInfo& bindingInfo) { entry.sampler.type = bindingInfo.type; }, + [&](const SamplerBindingInfo& bindingInfo) { + entry.sampler.type = bindingInfo.type; + + // TODO(487593147): Support filiterability in default pipeline + if (entry.sampler.type == kUnknownFilteringSamplerBindingType) { + entry.sampler.type = wgpu::SamplerBindingType::Filtering; + } + }, [&](const TextureBindingInfo& bindingInfo) { entry.texture.sampleType = bindingInfo.sampleType; entry.texture.viewDimension = bindingInfo.viewDimension; @@ -342,7 +346,9 @@ // Default to UnfilterableFloat for texture_Nd<f32> as it will be promoted to Float // if it is used with a sampler. - if (entry.texture.sampleType == wgpu::TextureSampleType::Float) { + // TODO(487593147): Support filiterability in default pipeline + if (entry.texture.sampleType == wgpu::TextureSampleType::Float || + entry.texture.sampleType == kUnknownFilterableFloatSampleType) { entry.texture.sampleType = wgpu::TextureSampleType::UnfilterableFloat; } },
diff --git a/src/dawn/native/Sampler.cpp b/src/dawn/native/Sampler.cpp index 9aeb013..0eada8c 100644 --- a/src/dawn/native/Sampler.cpp +++ b/src/dawn/native/Sampler.cpp
@@ -134,6 +134,17 @@ mMipmapFilter == wgpu::MipmapFilterMode::Linear; } +wgpu::SamplerBindingType SamplerBase::GetBindingType() const { + if (IsComparison()) { + return wgpu::SamplerBindingType::Comparison; + } + if (IsFiltering()) { + return wgpu::SamplerBindingType::Filtering; + } + + return wgpu::SamplerBindingType::NonFiltering; +} + bool SamplerBase::IsYCbCr() const { return mIsYCbCr; }
diff --git a/src/dawn/native/Sampler.h b/src/dawn/native/Sampler.h index 3242998..913f76b 100644 --- a/src/dawn/native/Sampler.h +++ b/src/dawn/native/Sampler.h
@@ -68,6 +68,7 @@ bool IsComparison() const; bool IsFiltering() const; + wgpu::SamplerBindingType GetBindingType() const; bool IsYCbCr() const; // Valid to call only if `IsYCbCr()` is true. YCbCrVkDescriptor GetYCbCrVkDescriptor() const;
diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index 4d30e72..2977511 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp
@@ -223,13 +223,13 @@ return wgpu::TextureSampleType::Sint; case tint::inspector::ResourceBinding::SampledKind::kUInt: return wgpu::TextureSampleType::Uint; - case tint::inspector::ResourceBinding::SampledKind::kFilterable: - case tint::inspector::ResourceBinding::SampledKind::kUnfilterable: - case tint::inspector::ResourceBinding::SampledKind::kUnknownFilterable: case tint::inspector::ResourceBinding::SampledKind::kFloat: - // TODO(dsinclair): For now, maintain old behaviour that all types are Float. - // Note that Float is compatible with both Float and UnfilterableFloat. + case tint::inspector::ResourceBinding::SampledKind::kFilterable: return wgpu::TextureSampleType::Float; + case tint::inspector::ResourceBinding::SampledKind::kUnfilterable: + return wgpu::TextureSampleType::UnfilterableFloat; + case tint::inspector::ResourceBinding::SampledKind::kUnknownFilterable: + return kUnknownFilterableFloatSampleType; } DAWN_UNREACHABLE(); } @@ -239,13 +239,12 @@ switch (type) { case tint::inspector::ResourceBinding::SamplerType::kComparison: return wgpu::SamplerBindingType::Comparison; - case tint::inspector::ResourceBinding::SamplerType::kFiltering: - case tint::inspector::ResourceBinding::SamplerType::kNonFiltering: - case tint::inspector::ResourceBinding::SamplerType::kUnknownFiltering: - // TODO(dsinclair): For now, maintain old behaviour that all types are - // filtering. return wgpu::SamplerBindingType::Filtering; + case tint::inspector::ResourceBinding::SamplerType::kNonFiltering: + return wgpu::SamplerBindingType::NonFiltering; + case tint::inspector::ResourceBinding::SamplerType::kUnknownFiltering: + return kUnknownFilteringSamplerBindingType; } DAWN_UNREACHABLE(); } @@ -631,18 +630,37 @@ "flag (%u)", bindingLayout.multisampled, shaderBindingInfo.multisampled); - wgpu::TextureSampleType bglShaderType = bindingLayout.sampleType; - // Both UnfilterableFloat and kInternalResolveAttachmentSampleType are compatible with - // texture_Nd<f32> instead of having a specific WGSL type. - if (bglShaderType == kInternalResolveAttachmentSampleType || - bglShaderType == wgpu::TextureSampleType::UnfilterableFloat) { - bglShaderType = wgpu::TextureSampleType::Float; + wgpu::TextureSampleType bglSampleType = bindingLayout.sampleType; + // `kInternalResolveAttachmentSampleType` is compatible with texture_Nd<f32> instead of + // having a specific WGSL type. + if (bglSampleType == kInternalResolveAttachmentSampleType) { + bglSampleType = wgpu::TextureSampleType::Float; } - DAWN_INVALID_IF(shaderBindingInfo.sampleType != bglShaderType, + + wgpu::TextureSampleType shaderSampleType = shaderBindingInfo.sampleType; + // TODO(487595547): Make Tint return unfilterable for multisampled textures + if (shaderBindingInfo.multisampled && + shaderSampleType == wgpu::TextureSampleType::Float) { + shaderSampleType = wgpu::TextureSampleType::UnfilterableFloat; + } + + bool isSameSampleType = shaderSampleType == bglSampleType; + bool unknownFloatSampleTypeInShader = + shaderSampleType == kUnknownFilterableFloatSampleType && + (bglSampleType == wgpu::TextureSampleType::Float || + bglSampleType == wgpu::TextureSampleType::UnfilterableFloat); + bool shaderSampleTypeConvertsFromRequiredFloat = + shaderSampleType == wgpu::TextureSampleType::UnfilterableFloat && + bglSampleType == wgpu::TextureSampleType::Float; + + bool bglConvertsToShaderSampleType = isSameSampleType || + unknownFloatSampleTypeInShader || + shaderSampleTypeConvertsFromRequiredFloat; + DAWN_INVALID_IF(!bglConvertsToShaderSampleType, "The shader's texture sample type (%s) isn't compatible with the " "layout's texture sample type (%s) (it is only compatible with %s for " "the shader texture sample type).", - shaderBindingInfo.sampleType, bindingLayout.sampleType, bglShaderType); + shaderSampleType, bindingLayout.sampleType, bglSampleType); DAWN_INVALID_IF( bindingLayout.viewDimension != shaderBindingInfo.viewDimension, @@ -719,24 +737,33 @@ return {}; }, [&](const SamplerBindingInfo& shaderBindingInfo) -> MaybeError { - bool comparisonInShader = - shaderBindingInfo.type == wgpu::SamplerBindingType::Comparison; + wgpu::SamplerBindingType shaderSamplerType = shaderBindingInfo.type; - bool comparisonInLayout = true; + wgpu::SamplerBindingType bglSamplerType; if (auto* staticBindingLayout = std::get_if<StaticSamplerBindingInfo>(&layoutInfo.bindingLayout)) { - comparisonInLayout = staticBindingLayout->sampler->IsComparison(); + bglSamplerType = staticBindingLayout->sampler->GetBindingType(); } else { - const SamplerBindingInfo& bindingLayout = - std::get<SamplerBindingInfo>(layoutInfo.bindingLayout); - comparisonInLayout = bindingLayout.type == wgpu::SamplerBindingType::Comparison; + bglSamplerType = std::get<SamplerBindingInfo>(layoutInfo.bindingLayout).type; } - DAWN_INVALID_IF( - comparisonInShader != comparisonInLayout, - "The sampler type in the shader (comparison: %u) doesn't match the type in " - "the layout (comparison: %u).", - comparisonInShader, comparisonInLayout); + bool isSameSamplerType = shaderSamplerType == bglSamplerType; + bool unknownFilteringTypeInShader = + shaderSamplerType == kUnknownFilteringSamplerBindingType && + (bglSamplerType == wgpu::SamplerBindingType::Filtering || + bglSamplerType == wgpu::SamplerBindingType::NonFiltering); + bool shaderSamplerTypeConvertsFromFiltering = + shaderSamplerType == wgpu::SamplerBindingType::NonFiltering && + bglSamplerType == wgpu::SamplerBindingType::Filtering; + + bool bglConvertsToShaderSamplerType = isSameSamplerType || + unknownFilteringTypeInShader || + shaderSamplerTypeConvertsFromFiltering; + DAWN_INVALID_IF(!bglConvertsToShaderSamplerType, + "The sampler type in the shader (%s) doesn't match the type in " + "the layout (%s).", + shaderSamplerType, bglSamplerType); + return {}; }, [](const ExternalTextureBindingInfo&) -> MaybeError { @@ -1146,7 +1173,6 @@ case BindingInfoType::Sampler: { SamplerBindingInfo bindingInfo = {}; - DAWN_ASSERT(resource.resource_type == tint::inspector::ResourceBinding::ResourceType::kSampler);
diff --git a/src/dawn/native/dawn_platform.h b/src/dawn/native/dawn_platform.h index 64e151d..d7696ab 100644 --- a/src/dawn/native/dawn_platform.h +++ b/src/dawn/native/dawn_platform.h
@@ -119,10 +119,19 @@ inline constexpr wgpu::TextureSampleType kInternalResolveAttachmentSampleType = static_cast<wgpu::TextureSampleType>(~0u); +// Extra TextureSampleType used internally when Tint reflects a float sample type of unknown +// filterability. +inline constexpr wgpu::TextureSampleType kUnknownFilterableFloatSampleType = + static_cast<wgpu::TextureSampleType>(~0u - 1); + // Extra TextureViewDimension for input attachment. inline constexpr wgpu::TextureViewDimension kInternalInputAttachmentDim = static_cast<wgpu::TextureViewDimension>(~0u); +// Extra SamplerBindingType used internally when Tint reflects a sampler of unknown filteringness. +inline constexpr wgpu::SamplerBindingType kUnknownFilteringSamplerBindingType = + static_cast<wgpu::SamplerBindingType>(~0u); + inline constexpr uint32_t kEnumPrefixMask = 0xFFFF'0000; inline constexpr uint32_t kDawnEnumPrefix = 0x0005'0000;
diff --git a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp index 2dc2bf2..2e8da70 100644 --- a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -3963,6 +3963,270 @@ TestComputePassBindings(bg.data(), kBindingNum, computePipeline, false); } +class FilterabilityValidationTest : public BindGroupLayoutCompatibilityTest {}; + +TEST_F(FilterabilityValidationTest, FilterableBGL_UnFilterableShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32, unfilterable>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, FilterableBGL_FilterableShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32, filterable>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, FilterableBGL_UnknownShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, UnfilterableBGL_UnFilterableShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32, unfilterable>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::UnfilterableFloat}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, UnfilterableBGL_FilterableShader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32, filterable>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::UnfilterableFloat}, + }); + + ASSERT_DEVICE_ERROR(CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr("isn't compatible")); +} + +TEST_F(FilterabilityValidationTest, UnfilterableBGL_UnknownShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::UnfilterableFloat}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, FilterableBGL_DepthShader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_depth_2d; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + }); + + ASSERT_DEVICE_ERROR(CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr("isn't compatible")); +} + +TEST_F(FilterabilityValidationTest, FilterableBGL_i32Shader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<i32>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + }); + + ASSERT_DEVICE_ERROR(CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr("isn't compatible")); +} + +TEST_F(FilterabilityValidationTest, FilteringBGL_FilteringShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::Filtering}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, FilteringBGL_NonFilteringShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<non_filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::Filtering}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, NonFilteringBGL_NonFilteringShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<non_filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::NonFiltering}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, NonFilteringBGL_FilteringShader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::NonFiltering}, + }); + + ASSERT_DEVICE_ERROR(CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr("doesn't match")); +} + +TEST_F(FilterabilityValidationTest, ComparisonBGL_ComparisonShader_Pass) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler_comparison; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::Comparison}, + }); + CreateComputePipeline(shaderSource, {bgl}); +} + +TEST_F(FilterabilityValidationTest, ComparisonBGL_FilteringShader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::Comparison}, + }); + ASSERT_DEVICE_ERROR( + CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr("(SamplerBindingType::Filtering) doesn't match the type in the layout " + "(SamplerBindingType::Comparison")); +} + +TEST_F(FilterabilityValidationTest, ComparisonBGL_NonFilteringShader_Fail) { + auto shaderSource = R"( + @group(0) @binding(0) var tex1 : texture_2d<f32>; + @group(0) @binding(1) var samp : sampler<non_filtering>; + + @compute @workgroup_size(1) fn main() { + _ = tex1; + _ = samp; + })"; + + auto bgl = utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float}, + {1, wgpu::ShaderStage::Compute, wgpu::SamplerBindingType::Comparison}, + }); + + ASSERT_DEVICE_ERROR( + CreateComputePipeline(shaderSource, {bgl}), + testing::HasSubstr( + "(SamplerBindingType::NonFiltering) doesn't match the type in the layout " + "(SamplerBindingType::Comparison")); +} + class SamplerTypeBindingTest : public ValidationTest { protected: wgpu::RenderPipeline CreateFragmentPipeline(wgpu::BindGroupLayout* bindGroupLayout,