Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic support for minBufferBindingSize #726

Merged
merged 1 commit into from
Jun 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions player/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use wgc::device::trace;
use std::{
ffi::CString,
fmt::Debug,
fs::File,
fs,
marker::PhantomData,
path::{Path, PathBuf},
ptr,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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());
Expand Down
13 changes: 10 additions & 3 deletions wgpu-core/src/conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,21 +79,28 @@ 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,
},
format: pso::BufferDescriptorFormat::Structured {
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,
Expand Down
50 changes: 29 additions & 21 deletions wgpu-core/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1458,20 +1458,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 { .. }
Expand All @@ -1481,11 +1481,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
Expand All @@ -1498,21 +1497,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)])
}
Expand Down
85 changes: 76 additions & 9 deletions wgpu-core/src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down
67 changes: 16 additions & 51 deletions wgpu-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))]
Expand Down Expand Up @@ -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.
///
Expand All @@ -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:
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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, .. }
Expand Down