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

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

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Draft
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ a soundness issue, the intent is to move an error from runtime to compile time.
### Bindless (`binding_array`) Grew More Capabilities

- DX12 now supports `PARTIALLY_BOUND_BINDING_ARRAY` on Resource Binding Tier 3 Hardware. This is most D3D12 hardware [D3D12 Feature Table] for more information on what hardware supports this feature. By @cwfitzgerald in [#6734](https://github.com/gfx-rs/wgpu/pull/6734).
- Buffer Dynamic Offsets and Binding Arrays may not be used in the same bind group. By @cwfitzgerald in [#6811](https://github.com/gfx-rs/wgpu/pull/6811)

[D3D12 Feature Table]: https://d3d12infodb.boolka.dev/FeatureTable.html

Expand Down
19 changes: 19 additions & 0 deletions wgpu-core/src/binding_model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ pub enum CreateBindGroupLayoutError {
},
#[error(transparent)]
TooManyBindings(BindingTypeMaxCountError),
#[error("Bind groups may not contain both a binding array and a dynamically offset array")]
ContainsBothBindingArrayAndDynamicOffsetArray,
#[error("Binding index {binding} is greater than the maximum number {maximum}")]
InvalidBindingIndex { binding: u32, maximum: u32 },
#[error("Invalid visibility {0:?}")]
Expand Down Expand Up @@ -315,6 +317,7 @@ pub(crate) struct BindingTypeMaxCountValidator {
storage_textures: PerStageBindingTypeCounter,
uniform_buffers: PerStageBindingTypeCounter,
acceleration_structures: PerStageBindingTypeCounter,
has_bindless_array: bool,
}

impl BindingTypeMaxCountValidator {
Expand Down Expand Up @@ -354,6 +357,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) {
Expand Down Expand Up @@ -405,6 +411,19 @@ 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;
if self.has_bindless_array && has_dynamic_offset_array {
return Err(CreateBindGroupLayoutError::ContainsBothBindingArrayAndDynamicOffsetArray);
}
Ok(())
}
}

/// Bindable resource and the slot to bind it to.
Expand Down
3 changes: 3 additions & 0 deletions wgpu-core/src/device/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1896,6 +1896,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(),
Expand Down
14 changes: 10 additions & 4 deletions wgpu-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7011,17 +7011,23 @@ 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`
///
/// 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>,
}
Expand Down
Loading