Merge #726

726: Basic support for minBufferBindingSize r=cwfitzgerald a=kvark

**Connections**
Has basic (partial) implementation of https://github.com/gpuweb/gpuweb/pull/678
wgpu-rs update - https://github.com/gfx-rs/wgpu-rs/pull/377

**Description**
This change allows users to optionally specify the expected minimum binding size for buffers. We are then validating this against both the pipelines and bind groups.
If it's not provided, we'll need to validate at draw time - this PR doesn't do this (focus on API changes first).
It also moves out the `read_spirv`, since wgpu-types wasn't the right home for it ever.

**Testing**
Tested on wgpu-rs examples

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

player/src/main.rs

@@ -15,7 +15,7 @@ use wgc::device::trace;
 use std::{
     ffi::CString,
     fmt::Debug,
-    fs::File,
+    fs,
     marker::PhantomData,
     path::{Path, PathBuf},
     ptr,
@@ -307,7 +307,11 @@ impl GlobalExt for wgc::hub::Global<IdentityPassThroughFactory> {
                 self.bind_group_destroy::<B>(id);
             }
             A::CreateShaderModule { id, data } => {
-                let spv = wgt::read_spirv(File::open(dir.join(data)).unwrap()).unwrap();
+                let byte_vec = fs::read(dir.join(data)).unwrap();
+                let spv = byte_vec
+                    .chunks(4)
+                    .map(|c| u32::from_le_bytes([c[0], c[1], c[2], c[3]]))
+                    .collect::<Vec<_>>();
                 self.device_create_shader_module::<B>(
                     device,
                     &wgc::pipeline::ShaderModuleDescriptor {
@@ -470,7 +474,7 @@ fn main() {
     };
 
     log::info!("Loading trace '{:?}'", dir);
-    let file = File::open(dir.join(trace::FILE_NAME)).unwrap();
+    let file = fs::File::open(dir.join(trace::FILE_NAME)).unwrap();
     let mut actions: Vec<trace::Action> = ron::de::from_reader(file).unwrap();
     actions.reverse(); // allows us to pop from the top
     log::info!("Found {} actions", actions.len());

wgpu-core/src/conv.rs

@@ -79,13 +79,20 @@ pub fn map_binding_type(binding: &wgt::BindGroupLayoutEntry) -> hal::pso::Descri
     use hal::pso;
     use wgt::BindingType as Bt;
     match binding.ty {
-        Bt::UniformBuffer { dynamic } => pso::DescriptorType::Buffer {
+        Bt::UniformBuffer {
+            dynamic,
+            min_binding_size: _,
+        } => pso::DescriptorType::Buffer {
             ty: pso::BufferDescriptorType::Uniform,
             format: pso::BufferDescriptorFormat::Structured {
                 dynamic_offset: dynamic,
             },
         },
-        Bt::StorageBuffer { readonly, dynamic } => pso::DescriptorType::Buffer {
+        Bt::StorageBuffer {
+            readonly,
+            dynamic,
+            min_binding_size: _,
+        } => pso::DescriptorType::Buffer {
             ty: pso::BufferDescriptorType::Storage {
                 read_only: readonly,
             },
@@ -93,7 +100,7 @@ pub fn map_binding_type(binding: &wgt::BindGroupLayoutEntry) -> hal::pso::Descri
                 dynamic_offset: dynamic,
             },
         },
-        Bt::Sampler { .. } => pso::DescriptorType::Sampler,
+        Bt::Sampler { comparison: _ } => pso::DescriptorType::Sampler,
         Bt::SampledTexture { .. } => pso::DescriptorType::Image {
             ty: pso::ImageDescriptorType::Sampled {
                 with_sampler: false,

wgpu-core/src/device/mod.rs

@@ -1449,20 +1449,20 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                     .expect("Failed to find binding declaration for binding");
                 let descriptors: SmallVec<[_; 1]> = match b.resource {
                     binding_model::BindingResource::Buffer(ref bb) => {
-                        let (alignment, pub_usage, internal_use) = match decl.ty {
-                            wgt::BindingType::UniformBuffer { .. } => (
-                                BIND_BUFFER_ALIGNMENT,
+                        let (pub_usage, internal_use, min_size) = match decl.ty {
+                            wgt::BindingType::UniformBuffer { dynamic: _, min_binding_size } => (
                                 wgt::BufferUsage::UNIFORM,
                                 resource::BufferUse::UNIFORM,
+                                min_binding_size,
                             ),
-                            wgt::BindingType::StorageBuffer { readonly, .. } => (
-                                BIND_BUFFER_ALIGNMENT,
+                            wgt::BindingType::StorageBuffer { dynamic: _, min_binding_size, readonly } => (
                                 wgt::BufferUsage::STORAGE,
                                 if readonly {
                                     resource::BufferUse::STORAGE_STORE
                                 } else {
                                     resource::BufferUse::STORAGE_LOAD
                                 },
+                                min_binding_size,
                             ),
                             wgt::BindingType::Sampler { .. }
                             | wgt::BindingType::StorageTexture { .. }
@@ -1472,11 +1472,10 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                         };
 
                         assert_eq!(
-                            bb.offset % alignment,
+                            bb.offset % BIND_BUFFER_ALIGNMENT,
                             0,
-                            "Buffer offset {} must be a multiple of alignment {}",
-                            bb.offset,
-                            alignment
+                            "Buffer offset {} must be a multiple of BIND_BUFFER_ALIGNMENT",
+                            bb.offset
                         );
 
                         let buffer = used
@@ -1489,21 +1488,30 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                             buffer.usage,
                             pub_usage
                         );
+                        let bind_size = if bb.size == BufferSize::WHOLE {
+                            buffer.size - bb.offset
+                        } else {
+                            let end = bb.offset + bb.size.0;
+                            assert!(
+                                end <= buffer.size,
+                                "Bound buffer range {:?} does not fit in buffer size {}",
+                                bb.offset..end,
+                                buffer.size
+                            );
+                            bb.size.0
+                        };
+                        match min_size {
+                            Some(non_zero) if non_zero.get() > bind_size => panic!(
+                                "Minimum buffer binding size {} is not respected with size {}",
+                                non_zero.get(),
+                                bind_size
+                            ),
+                            _ => (),
+                        }
 
                         let sub_range = hal::buffer::SubRange {
                             offset: bb.offset,
-                            size: if bb.size == BufferSize::WHOLE {
-                                None
-                            } else {
-                                let end = bb.offset + bb.size.0;
-                                assert!(
-                                    end <= buffer.size,
-                                    "Bound buffer range {:?} does not fit in buffer size {}",
-                                    bb.offset..end,
-                                    buffer.size
-                                );
-                                Some(bb.size.0)
-                            },
+                            size: Some(bind_size),
                         };
                         SmallVec::from([hal::pso::Descriptor::Buffer(&buffer.raw, sub_range)])
                     }

wgpu-core/src/validation.rs

@@ -16,6 +16,9 @@ pub enum BindingError {
     WrongUsage(naga::GlobalUse),
     /// The type on the shader side does not match the pipeline binding.
     WrongType,
+    /// The size of a buffer structure, added to one element of an unbound array,
+    /// if it's the last field, ended up greater than the given `min_binding_size`.
+    WrongBufferSize(wgt::BufferAddress),
     /// The view dimension doesn't match the shader.
     WrongTextureViewDimension { dim: spirv::Dim, is_array: bool },
     /// The component type of a sampled texture doesn't match the shader.
@@ -52,6 +55,45 @@ pub enum StageError {
     },
 }
 
+fn get_aligned_type_size(
+    module: &naga::Module,
+    inner: &naga::TypeInner,
+    allow_unbound: bool,
+) -> wgt::BufferAddress {
+    use naga::TypeInner as Ti;
+    //TODO: take alignment into account!
+    match *inner {
+        Ti::Scalar { kind: _, width } => width as wgt::BufferAddress / 8,
+        Ti::Vector {
+            size,
+            kind: _,
+            width,
+        } => size as wgt::BufferAddress * width as wgt::BufferAddress / 8,
+        Ti::Matrix {
+            rows,
+            columns,
+            kind: _,
+            width,
+        } => {
+            rows as wgt::BufferAddress * columns as wgt::BufferAddress * width as wgt::BufferAddress
+                / 8
+        }
+        Ti::Pointer { .. } => 4,
+        Ti::Array {
+            base,
+            size: naga::ArraySize::Static(size),
+        } => {
+            size as wgt::BufferAddress
+                * get_aligned_type_size(module, &module.types[base].inner, false)
+        }
+        Ti::Array {
+            base,
+            size: naga::ArraySize::Dynamic,
+        } if allow_unbound => get_aligned_type_size(module, &module.types[base].inner, false),
+        _ => panic!("Unexpected struct field"),
+    }
+}
+
 fn check_binding(
     module: &naga::Module,
     var: &naga::GlobalVariable,
@@ -64,17 +106,42 @@ fn check_binding(
         ty_inner = &module.types[base].inner;
     }
     let allowed_usage = match *ty_inner {
-        naga::TypeInner::Struct { .. } => match entry.ty {
-            BindingType::UniformBuffer { .. } => naga::GlobalUse::LOAD,
-            BindingType::StorageBuffer { readonly, .. } => {
-                if readonly {
-                    naga::GlobalUse::LOAD
-                } else {
-                    naga::GlobalUse::all()
+        naga::TypeInner::Struct { ref members } => {
+            let (allowed_usage, min_size) = match entry.ty {
+                BindingType::UniformBuffer {
+                    dynamic: _,
+                    min_binding_size,
+                } => (naga::GlobalUse::LOAD, min_binding_size),
+                BindingType::StorageBuffer {
+                    dynamic: _,
+                    min_binding_size,
+                    readonly,
+                } => {
+                    let global_use = if readonly {
+                        naga::GlobalUse::LOAD
+                    } else {
+                        naga::GlobalUse::all()
+                    };
+                    (global_use, min_binding_size)
                 }
+                _ => return Err(BindingError::WrongType),
+            };
+            let mut actual_size = 0;
+            for (i, member) in members.iter().enumerate() {
+                actual_size += get_aligned_type_size(
+                    module,
+                    &module.types[member.ty].inner,
+                    i + 1 == members.len(),
+                );
             }
-            _ => return Err(BindingError::WrongType),
-        },
+            match min_size {
+                Some(non_zero) if non_zero.get() < actual_size => {
+                    return Err(BindingError::WrongBufferSize(actual_size))
+                }
+                _ => (),
+            }
+            allowed_usage
+        }
         naga::TypeInner::Sampler => match entry.ty {
             BindingType::Sampler { .. } => naga::GlobalUse::empty(),
             _ => return Err(BindingError::WrongType),

wgpu-types/src/lib.rs

@@ -8,9 +8,9 @@ use peek_poke::PeekPoke;
 use serde::Deserialize;
 #[cfg(feature = "trace")]
 use serde::Serialize;
-use std::{io, slice};
 
 pub type BufferAddress = u64;
+pub type NonZeroBufferAddress = std::num::NonZeroU64;
 
 /// Buffer-Texture copies on command encoders have to have the `bytes_per_row`
 /// aligned to this number.
@@ -320,46 +320,6 @@ pub struct DeviceDescriptor {
     pub shader_validation: bool,
 }
 
-// TODO: This is copy/pasted from gfx-hal, so we need to find a new place to put
-// this function
-pub fn read_spirv<R: io::Read + io::Seek>(mut x: R) -> io::Result<Vec<u32>> {
-    let size = x.seek(io::SeekFrom::End(0))?;
-    if size % 4 != 0 {
-        return Err(io::Error::new(
-            io::ErrorKind::InvalidData,
-            "input length not divisible by 4",
-        ));
-    }
-    if size > usize::max_value() as u64 {
-        return Err(io::Error::new(io::ErrorKind::InvalidData, "input too long"));
-    }
-    let words = (size / 4) as usize;
-    let mut result = Vec::<u32>::with_capacity(words);
-    x.seek(io::SeekFrom::Start(0))?;
-    unsafe {
-        // Writing all bytes through a pointer with less strict alignment when our type has no
-        // invalid bitpatterns is safe.
-        x.read_exact(slice::from_raw_parts_mut(
-            result.as_mut_ptr() as *mut u8,
-            words * 4,
-        ))?;
-        result.set_len(words);
-    }
-    const MAGIC_NUMBER: u32 = 0x0723_0203;
-    if !result.is_empty() && result[0] == MAGIC_NUMBER.swap_bytes() {
-        for word in &mut result {
-            *word = word.swap_bytes();
-        }
-    }
-    if result.is_empty() || result[0] != MAGIC_NUMBER {
-        return Err(io::Error::new(
-            io::ErrorKind::InvalidData,
-            "input missing SPIR-V magic number",
-        ));
-    }
-    Ok(result)
-}
-
 bitflags::bitflags! {
     #[repr(transparent)]
     #[cfg_attr(feature = "trace", derive(Serialize))]
@@ -1288,6 +1248,11 @@ pub enum BindingType {
         /// Indicates that the binding has a dynamic offset.
         /// One offset must be passed to [RenderPass::set_bind_group] for each dynamic binding in increasing order of binding number.
         dynamic: bool,
+        /// Minimum size of the corresponding `BufferBinding` required to match this entry.
+        /// When pipeline is created, the size has to cover at least the corresponding structure in the shader
+        /// plus one element of the unbound array, which can only be last in the structure.
+        /// If `None`, the check is performed at draw call time instead of pipeline and bind group creation.
+        min_binding_size: Option<NonZeroBufferAddress>,
     },
     /// A storage buffer.
     ///
@@ -1301,6 +1266,11 @@ pub enum BindingType {
         /// Indicates that the binding has a dynamic offset.
         /// One offset must be passed to [RenderPass::set_bind_group] for each dynamic binding in increasing order of binding number.
         dynamic: bool,
+        /// Minimum size of the corresponding `BufferBinding` required to match this entry.
+        /// When pipeline is created, the size has to cover at least the corresponding structure in the shader
+        /// plus one element of the unbound array, which can only be last in the structure.
+        /// If `None`, the check is performed at draw call time instead of pipeline and bind group creation.
+        min_binding_size: Option<NonZeroBufferAddress>,
         /// The buffer can only be read in the shader and it must be annotated with `readonly`.
         ///
         /// Example GLSL syntax:
@@ -1350,9 +1320,6 @@ pub enum BindingType {
     StorageTexture {
         /// Dimension of the texture view that is going to be sampled.
         dimension: TextureViewDimension,
-        /// Component type of the texture.
-        /// This must be compatible with the format of the texture.
-        component_type: TextureComponentType,
         /// Format of the texture.
         format: TextureFormat,
         /// The texture can only be read in the shader and it must be annotated with `readonly`.
@@ -1384,19 +1351,17 @@ pub struct BindGroupLayoutEntry {
     pub _non_exhaustive: NonExhaustive,
 }
 
-impl Default for BindGroupLayoutEntry {
-    fn default() -> Self {
+impl BindGroupLayoutEntry {
+    pub fn new(binding: u32, visibility: ShaderStage, ty: BindingType) -> Self {
         Self {
-            binding: 0,
-            visibility: ShaderStage::NONE,
-            ty: BindingType::UniformBuffer { dynamic: false },
+            binding,
+            visibility,
+            ty,
             count: None,
             _non_exhaustive: unsafe { NonExhaustive::new() },
         }
     }
-}
 
-impl BindGroupLayoutEntry {
     pub fn has_dynamic_offset(&self) -> bool {
         match self.ty {
             BindingType::UniformBuffer { dynamic, .. }