From a27341bd6dc369a01e8ddd6e61727ab9f38860ad Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Mon, 15 Jun 2020 22:33:11 -0400 Subject: [PATCH] Basic support for minBufferBindingSize --- player/src/main.rs | 10 +++-- wgpu-core/src/conv.rs | 13 ++++-- wgpu-core/src/device/mod.rs | 50 +++++++++++++--------- wgpu-core/src/validation.rs | 85 +++++++++++++++++++++++++++++++++---- wgpu-types/src/lib.rs | 67 +++++++---------------------- 5 files changed, 138 insertions(+), 87 deletions(-) diff --git a/player/src/main.rs b/player/src/main.rs index 1f6bb5fe0..27400444f 100644 --- a/player/src/main.rs +++ b/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 { self.bind_group_destroy::(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::>(); self.device_create_shader_module::( 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 = ron::de::from_reader(file).unwrap(); actions.reverse(); // allows us to pop from the top log::info!("Found {} actions", actions.len()); diff --git a/wgpu-core/src/conv.rs b/wgpu-core/src/conv.rs index 9f73aa7e8..b344506df 100644 --- a/wgpu-core/src/conv.rs +++ b/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, diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 267eb7f9e..0ccca168e 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1458,20 +1458,20 @@ impl Global { .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 { .. } @@ -1481,11 +1481,10 @@ impl Global { }; 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 @@ -1498,21 +1497,30 @@ impl Global { 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)]) } diff --git a/wgpu-core/src/validation.rs b/wgpu-core/src/validation.rs index 81802fcdd..dbd53ee19 100644 --- a/wgpu-core/src/validation.rs +++ b/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), diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index f8958ce8d..529fda83d 100644 --- a/wgpu-types/src/lib.rs +++ b/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(mut x: R) -> io::Result> { - 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::::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, }, /// 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, /// 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, .. }