Ban Dynamic Offsets and Binding Arrays in the Same Bind Group (#6811)

* Ban Dynamic Offsets and Binding Arrays in the Same Bind Group

* Add Tests and Uniform Buffer Restriction

CHANGELOG.md

@@ -50,6 +50,8 @@ Bottom level categories:
 
 #### General
 
+- If you use Binding Arrays in a bind group, you may not use Dynamic Offset Buffers or Uniform Buffers in that bind group. By @cwfitzgerald in [#6811](https://github.com/gfx-rs/wgpu/pull/6811)
+
 ##### Refactored internal trace path parameter
 
 Refactored some functions to handle the internal trace path as a string to avoid possible issues with `no_std` support.

examples/src/texture_arrays/indexing.wgsl

@@ -35,7 +35,7 @@ struct Uniforms {
     index: u32,
 }
 
-@group(0) @binding(3)
+@group(1) @binding(0)
 var<uniform> uniforms: Uniforms;
 
 @fragment

examples/src/texture_arrays/mod.rs

@@ -64,6 +64,7 @@ fn create_texture_data(color: Color) -> [u8; 4] {
 struct Example {
     pipeline: wgpu::RenderPipeline,
     bind_group: wgpu::BindGroup,
+    uniform_bind_group: wgpu::BindGroup,
     vertex_buffer: wgpu::Buffer,
     index_buffer: wgpu::Buffer,
     index_format: wgpu::IndexFormat,
@@ -261,8 +262,14 @@ impl crate::framework::Example for Example {
                     ty: wgpu::BindingType::Sampler(wgpu::SamplerBindingType::Filtering),
                     count: NonZeroU32::new(2),
                 },
-                wgpu::BindGroupLayoutEntry {
-                    binding: 3,
+            ],
+        });
+
+        let uniform_bind_group_layout =
+            device.create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor {
+                label: Some("uniform bind group layout"),
+                entries: &[wgpu::BindGroupLayoutEntry {
+                    binding: 0,
                     visibility: wgpu::ShaderStages::FRAGMENT,
                     ty: wgpu::BindingType::Buffer {
                         ty: wgpu::BufferBindingType::Uniform,
@@ -270,9 +277,8 @@ impl crate::framework::Example for Example {
                         min_binding_size: Some(NonZeroU64::new(4).unwrap()),
                     },
                     count: None,
-                },
-            ],
-        });
+                }],
+            });
 
         let bind_group = device.create_bind_group(&wgpu::BindGroupDescriptor {
             entries: &[
@@ -294,22 +300,27 @@ impl crate::framework::Example for Example {
                     binding: 2,
                     resource: wgpu::BindingResource::SamplerArray(&[&sampler, &sampler]),
                 },
-                wgpu::BindGroupEntry {
-                    binding: 3,
-                    resource: wgpu::BindingResource::Buffer(wgpu::BufferBinding {
-                        buffer: &texture_index_buffer,
-                        offset: 0,
-                        size: Some(NonZeroU64::new(4).unwrap()),
-                    }),
-                },
             ],
             layout: &bind_group_layout,
             label: Some("bind group"),
         });
 
+        let uniform_bind_group = device.create_bind_group(&wgpu::BindGroupDescriptor {
+            entries: &[wgpu::BindGroupEntry {
+                binding: 0,
+                resource: wgpu::BindingResource::Buffer(wgpu::BufferBinding {
+                    buffer: &texture_index_buffer,
+                    offset: 0,
+                    size: Some(NonZeroU64::new(4).unwrap()),
+                }),
+            }],
+            layout: &uniform_bind_group_layout,
+            label: Some("uniform bind group"),
+        });
+
         let pipeline_layout = device.create_pipeline_layout(&wgpu::PipelineLayoutDescriptor {
             label: Some("main"),
-            bind_group_layouts: &[&bind_group_layout],
+            bind_group_layouts: &[&bind_group_layout, &uniform_bind_group_layout],
             push_constant_ranges: &[],
         });
 
@@ -347,6 +358,7 @@ impl crate::framework::Example for Example {
         Self {
             pipeline,
             bind_group,
+            uniform_bind_group,
             vertex_buffer,
             index_buffer,
             index_format,
@@ -388,12 +400,14 @@ impl crate::framework::Example for Example {
         rpass.set_vertex_buffer(0, self.vertex_buffer.slice(..));
         rpass.set_index_buffer(self.index_buffer.slice(..), self.index_format);
         if self.uniform_workaround {
-            rpass.set_bind_group(0, &self.bind_group, &[0]);
+            rpass.set_bind_group(0, &self.bind_group, &[]);
+            rpass.set_bind_group(1, &self.uniform_bind_group, &[0]);
             rpass.draw_indexed(0..6, 0, 0..1);
-            rpass.set_bind_group(0, &self.bind_group, &[256]);
+            rpass.set_bind_group(1, &self.uniform_bind_group, &[256]);
             rpass.draw_indexed(6..12, 0, 0..1);
         } else {
-            rpass.set_bind_group(0, &self.bind_group, &[0]);
+            rpass.set_bind_group(0, &self.bind_group, &[]);
+            rpass.set_bind_group(1, &self.uniform_bind_group, &[0]);
             rpass.draw_indexed(0..12, 0, 0..1);
         }
 

tests/src/lib.rs

@@ -42,8 +42,8 @@ pub fn fail<T>(
         assert!(
             lowered_actual.contains(&lowered_expected),
             concat!(
-                "expected validation error case-insensitively containing {:?}, ",
-                "but it was not present in actual error message:\n{:?}"
+                "expected validation error case-insensitively containing {}, ",
+                "but it was not present in actual error message:\n{}"
             ),
             expected_msg_substring,
             validation_error

tests/tests/binding_array/mod.rs

@@ -2,3 +2,4 @@ mod buffers;
 mod sampled_textures;
 mod samplers;
 mod storage_textures;
+mod validation;

tests/tests/binding_array/validation.rs

@@ -0,0 +1,92 @@
+use std::num::NonZeroU32;
+
+use wgpu::*;
+use wgpu_test::{
+    fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters, TestingContext,
+};
+
+#[gpu_test]
+static VALIDATION: GpuTestConfiguration = GpuTestConfiguration::new()
+    .parameters(
+        TestParameters::default()
+            .features(Features::TEXTURE_BINDING_ARRAY)
+            .limits(Limits {
+                max_dynamic_storage_buffers_per_pipeline_layout: 1,
+                ..Limits::downlevel_defaults()
+            })
+            .expect_fail(
+                // https://github.com/gfx-rs/wgpu/issues/6950
+                FailureCase::backend(Backends::VULKAN).validation_error("has not been destroyed"),
+            ),
+    )
+    .run_async(validation);
+
+async fn validation(ctx: TestingContext) {
+    // Check that you can't create a bind group with both dynamic offset and binding array
+    fail(
+        &ctx.device,
+        || {
+            ctx.device
+                .create_bind_group_layout(&BindGroupLayoutDescriptor {
+                    label: Some("Test1"),
+                    entries: &[
+                        BindGroupLayoutEntry {
+                            binding: 0,
+                            visibility: ShaderStages::FRAGMENT,
+                            ty: BindingType::Texture {
+                                sample_type: TextureSampleType::Float { filterable: false },
+                                view_dimension: TextureViewDimension::D2,
+                                multisampled: false,
+                            },
+                            count: Some(NonZeroU32::new(4).unwrap()),
+                        },
+                        BindGroupLayoutEntry {
+                            binding: 1,
+                            visibility: ShaderStages::FRAGMENT,
+                            ty: BindingType::Buffer {
+                                ty: BufferBindingType::Storage { read_only: true },
+                                has_dynamic_offset: true,
+                                min_binding_size: None,
+                            },
+                            count: None,
+                        },
+                    ],
+                })
+        },
+        Some("binding array and a dynamically offset buffer"),
+    );
+
+    // Check that you can't create a bind group with both uniform buffer and binding array
+    fail(
+        &ctx.device,
+        || {
+            ctx.device
+                .create_bind_group_layout(&BindGroupLayoutDescriptor {
+                    label: Some("Test2"),
+                    entries: &[
+                        BindGroupLayoutEntry {
+                            binding: 0,
+                            visibility: ShaderStages::FRAGMENT,
+                            ty: BindingType::Texture {
+                                sample_type: TextureSampleType::Float { filterable: false },
+                                view_dimension: TextureViewDimension::D2,
+                                multisampled: false,
+                            },
+                            count: Some(NonZeroU32::new(4).unwrap()),
+                        },
+                        BindGroupLayoutEntry {
+                            binding: 1,
+                            visibility: ShaderStages::FRAGMENT,
+                            ty: BindingType::Buffer {
+                                ty: BufferBindingType::Uniform,
+                                has_dynamic_offset: false,
+                                min_binding_size: None,
+                            },
+                            count: None,
+                        },
+                    ],
+                })
+        },
+        Some("binding array and a uniform buffer"),
+    );
+}

wgpu-core/src/binding_model.rs

@@ -68,6 +68,10 @@ pub enum CreateBindGroupLayoutError {
     },
     #[error(transparent)]
     TooManyBindings(BindingTypeMaxCountError),
+    #[error("Bind groups may not contain both a binding array and a dynamically offset buffer")]
+    ContainsBothBindingArrayAndDynamicOffsetArray,
+    #[error("Bind groups may not contain both a binding array and a uniform buffer")]
+    ContainsBothBindingArrayAndUniformBuffer,
     #[error("Binding index {binding} is greater than the maximum number {maximum}")]
     InvalidBindingIndex { binding: u32, maximum: u32 },
     #[error("Invalid visibility {0:?}")]
@@ -319,6 +323,7 @@ pub(crate) struct BindingTypeMaxCountValidator {
     storage_textures: PerStageBindingTypeCounter,
     uniform_buffers: PerStageBindingTypeCounter,
     acceleration_structures: PerStageBindingTypeCounter,
+    has_bindless_array: bool,
 }
 
 impl BindingTypeMaxCountValidator {
@@ -358,6 +363,9 @@ impl BindingTypeMaxCountValidator {
                 self.acceleration_structures.add(binding.visibility, count);
             }
         }
+        if binding.count.is_some() {
+            self.has_bindless_array = true;
+        }
     }
 
     pub(crate) fn merge(&mut self, other: &Self) {
@@ -409,6 +417,23 @@ impl BindingTypeMaxCountValidator {
         )?;
         Ok(())
     }
+
+    /// Validate that the bind group layout does not contain both a binding array and a dynamic offset array.
+    ///
+    /// This allows us to use `UPDATE_AFTER_BIND` on vulkan for bindless arrays. Vulkan does not allow
+    /// `UPDATE_AFTER_BIND` on dynamic offset arrays. See <https://github.com/gfx-rs/wgpu/issues/6737>
+    pub(crate) fn validate_binding_arrays(&self) -> Result<(), CreateBindGroupLayoutError> {
+        let has_dynamic_offset_array =
+            self.dynamic_uniform_buffers > 0 || self.dynamic_storage_buffers > 0;
+        let has_uniform_buffer = self.uniform_buffers.max().1 > 0;
+        if self.has_bindless_array && has_dynamic_offset_array {
+            return Err(CreateBindGroupLayoutError::ContainsBothBindingArrayAndDynamicOffsetArray);
+        }
+        if self.has_bindless_array && has_uniform_buffer {
+            return Err(CreateBindGroupLayoutError::ContainsBothBindingArrayAndUniformBuffer);
+        }
+        Ok(())
+    }
 }
 
 /// Bindable resource and the slot to bind it to.

wgpu-core/src/device/resource.rs

@@ -1908,6 +1908,9 @@ impl Device {
             .validate(&self.limits)
             .map_err(binding_model::CreateBindGroupLayoutError::TooManyBindings)?;
 
+        // Validate that binding arrays don't conflict with dynamic offsets.
+        count_validator.validate_binding_arrays()?;
+
         let bgl = BindGroupLayout {
             raw: ManuallyDrop::new(raw),
             device: self.clone(),

wgpu-types/src/lib.rs

@@ -7113,17 +7113,24 @@ impl BindingType {
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct BindGroupLayoutEntry {
     /// Binding index. Must match shader index and be unique inside a BindGroupLayout. A binding
-    /// of index 1, would be described as `layout(set = 0, binding = 1) uniform` in shaders.
+    /// of index 1, would be described as `@group(0) @binding(1)` in shaders.
     pub binding: u32,
     /// Which shader stages can see this binding.
     pub visibility: ShaderStages,
     /// The type of the binding
     pub ty: BindingType,
-    /// If this value is Some, indicates this entry is an array. Array size must be 1 or greater.
+    /// If the binding is an array of multiple resources. Corresponds to `binding_array<T>` in the shader.
     ///
-    /// If this value is Some and `ty` is `BindingType::Texture`, [`Features::TEXTURE_BINDING_ARRAY`] must be supported.
+    /// When this is `Some` the following validation applies:
+    /// - Size must be of value 1 or greater.
+    /// - When `ty == BindingType::Texture`, [`Features::TEXTURE_BINDING_ARRAY`] must be supported.
+    /// - When `ty == BindingType::Sampler`, [`Features::TEXTURE_BINDING_ARRAY`] must be supported.
+    /// - When `ty == BindingType::Buffer`, [`Features::BUFFER_BINDING_ARRAY`] must be supported.
+    /// - When `ty == BindingType::Buffer` and `ty.ty == BufferBindingType::Storage`, [`Features::STORAGE_RESOURCE_BINDING_ARRAY`] must be supported.
+    /// - When `ty == BindingType::StorageTexture`, [`Features::STORAGE_RESOURCE_BINDING_ARRAY`] must be supported.
+    /// - When any binding in the group is an array, no `BindingType::Buffer` in the group may have `has_dynamic_offset == true`
+    /// - When any binding in the group is an array, no `BindingType::Buffer` in the group may have `ty.ty == BufferBindingType::Uniform`.
     ///
-    /// If this value is Some and `ty` is any other variant, bind group creation will fail.
     #[cfg_attr(feature = "serde", serde(default))]
     pub count: Option<NonZeroU32>,
 }