From d757c300a49888b4a4eb51dee75173faf0b65f07 Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Mon, 15 Jul 2019 10:29:08 +0000
Subject: [PATCH] Validate usage of dynamic at the BGL level.

Previously validation of the combination of dynamic with the binding
type was only done when creating the bind group when it should have been
done when the bind group layout is created.

BUG=dawn:55

Change-Id: I976f7e052f9737929fc05908af50e6ad5ceef397
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/8861
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/BindGroup.cpp                 |  7 ------
 src/dawn_native/BindGroupLayout.cpp           | 15 +++++++++++-
 .../validation/BindGroupValidationTests.cpp   | 24 ++++++++++++++++++-
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp
index a6db7cfc41..205a1588ff 100644
--- a/src/dawn_native/BindGroup.cpp
+++ b/src/dawn_native/BindGroup.cpp
@@ -132,17 +132,10 @@ namespace dawn_native {
                     DAWN_TRY(ValidateBufferBinding(device, binding, dawn::BufferUsageBit::Storage));
                     break;
                 case dawn::BindingType::SampledTexture:
-                    if (layoutInfo.dynamic[bindingIndex]) {
-                        return DAWN_VALIDATION_ERROR(
-                            "SampledTextures are expected to be not dynamic");
-                    }
                     DAWN_TRY(
                         ValidateTextureBinding(device, binding, dawn::TextureUsageBit::Sampled));
                     break;
                 case dawn::BindingType::Sampler:
-                    if (layoutInfo.dynamic[bindingIndex]) {
-                        return DAWN_VALIDATION_ERROR("Samplers are expected to be not dynamic");
-                    }
                     DAWN_TRY(ValidateSamplerBinding(device, binding));
                     break;
             }
diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp
index 9dcdce4474..e64635964d 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -31,7 +31,7 @@ namespace dawn_native {
 
         std::bitset<kMaxBindingsPerGroup> bindingsSet;
         for (uint32_t i = 0; i < descriptor->bindingCount; ++i) {
-            auto& binding = descriptor->bindings[i];
+            const BindGroupLayoutBinding& binding = descriptor->bindings[i];
             DAWN_TRY(ValidateShaderStageBit(binding.visibility));
             DAWN_TRY(ValidateBindingType(binding.type));
 
@@ -41,6 +41,19 @@ namespace dawn_native {
             if (bindingsSet[binding.binding]) {
                 return DAWN_VALIDATION_ERROR("some binding index was specified more than once");
             }
+
+            switch (binding.type) {
+                case dawn::BindingType::UniformBuffer:
+                case dawn::BindingType::StorageBuffer:
+                    break;
+                case dawn::BindingType::SampledTexture:
+                case dawn::BindingType::Sampler:
+                    if (binding.dynamic) {
+                        return DAWN_VALIDATION_ERROR("Samplers and textures cannot be dynamic");
+                    }
+                    break;
+            }
+
             bindingsSet.set(binding.binding);
         }
         return {};
diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp
index a222a574e1..4b73cda838 100644
--- a/src/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -422,13 +422,35 @@ TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutBindingOOB) {
 // This test verifies that the BindGroupLayout bindings are correctly validated, even if the
 // binding ids are out-of-order.
 TEST_F(BindGroupLayoutValidationTest, BindGroupBinding) {
-    auto layout = utils::MakeBindGroupLayout(
+    utils::MakeBindGroupLayout(
         device, {
                     {1, dawn::ShaderStageBit::Vertex, dawn::BindingType::UniformBuffer},
                     {0, dawn::ShaderStageBit::Vertex, dawn::BindingType::UniformBuffer},
                 });
 }
 
+// Check that dynamic = true is only allowed with buffer bindings.
+TEST_F(BindGroupLayoutValidationTest, DynamicAndTypeCompatibility) {
+    utils::MakeBindGroupLayout(
+        device, {
+                    {0, dawn::ShaderStageBit::Compute, dawn::BindingType::UniformBuffer, true},
+                });
+
+    utils::MakeBindGroupLayout(
+        device, {
+                    {0, dawn::ShaderStageBit::Compute, dawn::BindingType::StorageBuffer, true},
+                });
+
+    ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout(
+        device, {
+                    {0, dawn::ShaderStageBit::Compute, dawn::BindingType::SampledTexture, true},
+                }));
+
+    ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout(
+        device, {
+                    {0, dawn::ShaderStageBit::Compute, dawn::BindingType::Sampler, true},
+                }));
+}
 
 // This test verifies that the BindGroupLayout cache is successfully caching/deduplicating objects.
 //