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

Use Update After Bind Descriptors for Bind Groups With Binding Arrays #6815

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
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: 4 additions & 6 deletions deno_webgpu/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,8 @@ fn deserialize_features(features: &wgpu_types::Features) -> Vec<&'static str> {
) {
return_features.push("sampled-texture-and-storage-buffer-array-non-uniform-indexing");
}
if features.contains(
wgpu_types::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
) {
return_features.push("uniform-buffer-and-storage-texture-array-non-uniform-indexing");
if features.contains(wgpu_types::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing the new feature, it's not exposed here

return_features.push("storage-texture-array-non-uniform-indexing");
}
if features.contains(wgpu_types::Features::PARTIALLY_BOUND_BINDING_ARRAY) {
return_features.push("partially-bound-binding-array");
Expand Down Expand Up @@ -553,10 +551,10 @@ impl From<GpuRequiredFeatures> for wgpu_types::Features {
.contains("sampled-texture-and-storage-buffer-array-non-uniform-indexing"),
);
features.set(
wgpu_types::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
wgpu_types::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, new feature is missing here

required_features
.0
.contains("uniform-buffer-and-storage-texture-array-non-uniform-indexing"),
.contains("storage-texture-array-non-uniform-indexing"),
);
features.set(
wgpu_types::Features::PARTIALLY_BOUND_BINDING_ARRAY,
Expand Down
7 changes: 4 additions & 3 deletions naga/src/valid/analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,8 @@ impl FunctionInfo {
..
} => {
// these are nasty aliases, but these idents are too long and break rustfmt
let ub_st = super::Capabilities::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING;
let sto = super::Capabilities::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING;
let uni = super::Capabilities::UNIFORM_BUFFER_ARRAY_NON_UNIFORM_INDEXING;
let st_sb = super::Capabilities::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING;
let sampler = super::Capabilities::SAMPLER_NON_UNIFORM_INDEXING;

Expand All @@ -542,7 +543,7 @@ impl FunctionInfo {
needed_caps |= match *array_element_ty {
// If we're an image, use the appropriate limit.
crate::TypeInner::Image { class, .. } => match class {
crate::ImageClass::Storage { .. } => ub_st,
crate::ImageClass::Storage { .. } => sto,
_ => st_sb,
},
crate::TypeInner::Sampler { .. } => sampler,
Expand All @@ -551,7 +552,7 @@ impl FunctionInfo {
if let E::GlobalVariable(global_handle) = expression_arena[base] {
let global = &resolve_context.global_vars[global_handle];
match global.space {
crate::AddressSpace::Uniform => ub_st,
crate::AddressSpace::Uniform => uni,
crate::AddressSpace::Storage { .. } => st_sb,
_ => unreachable!(),
}
Expand Down
44 changes: 23 additions & 21 deletions naga/src/valid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,47 +90,49 @@ bitflags::bitflags! {
const PRIMITIVE_INDEX = 1 << 2;
/// Support for non-uniform indexing of sampled textures and storage buffer arrays.
const SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING = 1 << 3;
/// Support for non-uniform indexing of uniform buffers and storage texture arrays.
const UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING = 1 << 4;
/// Support for non-uniform indexing of storage texture arrays.
const STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING = 1 << 4;
/// Support for non-uniform indexing of uniform buffer arrays.
const UNIFORM_BUFFER_ARRAY_NON_UNIFORM_INDEXING = 1 << 5;
/// Support for non-uniform indexing of samplers.
const SAMPLER_NON_UNIFORM_INDEXING = 1 << 5;
const SAMPLER_NON_UNIFORM_INDEXING = 1 << 6;
/// Support for [`BuiltIn::ClipDistance`].
///
/// [`BuiltIn::ClipDistance`]: crate::BuiltIn::ClipDistance
const CLIP_DISTANCE = 1 << 6;
const CLIP_DISTANCE = 1 << 7;
/// Support for [`BuiltIn::CullDistance`].
///
/// [`BuiltIn::CullDistance`]: crate::BuiltIn::CullDistance
const CULL_DISTANCE = 1 << 7;
const CULL_DISTANCE = 1 << 8;
/// Support for 16-bit normalized storage texture formats.
const STORAGE_TEXTURE_16BIT_NORM_FORMATS = 1 << 8;
const STORAGE_TEXTURE_16BIT_NORM_FORMATS = 1 << 9;
/// Support for [`BuiltIn::ViewIndex`].
///
/// [`BuiltIn::ViewIndex`]: crate::BuiltIn::ViewIndex
const MULTIVIEW = 1 << 9;
const MULTIVIEW = 1 << 10;
/// Support for `early_depth_test`.
const EARLY_DEPTH_TEST = 1 << 10;
const EARLY_DEPTH_TEST = 1 << 11;
/// Support for [`BuiltIn::SampleIndex`] and [`Sampling::Sample`].
///
/// [`BuiltIn::SampleIndex`]: crate::BuiltIn::SampleIndex
/// [`Sampling::Sample`]: crate::Sampling::Sample
const MULTISAMPLED_SHADING = 1 << 11;
const MULTISAMPLED_SHADING = 1 << 12;
/// Support for ray queries and acceleration structures.
const RAY_QUERY = 1 << 12;
const RAY_QUERY = 1 << 13;
/// Support for generating two sources for blending from fragment shaders.
const DUAL_SOURCE_BLENDING = 1 << 13;
const DUAL_SOURCE_BLENDING = 1 << 14;
/// Support for arrayed cube textures.
const CUBE_ARRAY_TEXTURES = 1 << 14;
const CUBE_ARRAY_TEXTURES = 1 << 15;
/// Support for 64-bit signed and unsigned integers.
const SHADER_INT64 = 1 << 15;
const SHADER_INT64 = 1 << 16;
/// Support for subgroup operations.
/// Implies support for subgroup operations in both fragment and compute stages,
/// but not necessarily in the vertex stage, which requires [`Capabilities::SUBGROUP_VERTEX_STAGE`].
const SUBGROUP = 1 << 16;
const SUBGROUP = 1 << 17;
/// Support for subgroup barriers.
const SUBGROUP_BARRIER = 1 << 17;
const SUBGROUP_BARRIER = 1 << 18;
/// Support for subgroup operations in the vertex stage.
const SUBGROUP_VERTEX_STAGE = 1 << 18;
const SUBGROUP_VERTEX_STAGE = 1 << 19;
/// Support for [`AtomicFunction::Min`] and [`AtomicFunction::Max`] on
/// 64-bit integers in the [`Storage`] address space, when the return
/// value is not used.
Expand All @@ -140,9 +142,9 @@ bitflags::bitflags! {
/// [`AtomicFunction::Min`]: crate::AtomicFunction::Min
/// [`AtomicFunction::Max`]: crate::AtomicFunction::Max
/// [`Storage`]: crate::AddressSpace::Storage
const SHADER_INT64_ATOMIC_MIN_MAX = 1 << 19;
const SHADER_INT64_ATOMIC_MIN_MAX = 1 << 20;
/// Support for all atomic operations on 64-bit integers.
const SHADER_INT64_ATOMIC_ALL_OPS = 1 << 20;
const SHADER_INT64_ATOMIC_ALL_OPS = 1 << 21;
/// Support for [`AtomicFunction::Add`], [`AtomicFunction::Sub`],
/// and [`AtomicFunction::Exchange { compare: None }`] on 32-bit floating-point numbers
/// in the [`Storage`] address space.
Expand All @@ -151,11 +153,11 @@ bitflags::bitflags! {
/// [`AtomicFunction::Sub`]: crate::AtomicFunction::Sub
/// [`AtomicFunction::Exchange { compare: None }`]: crate::AtomicFunction::Exchange
/// [`Storage`]: crate::AddressSpace::Storage
const SHADER_FLOAT32_ATOMIC = 1 << 21;
const SHADER_FLOAT32_ATOMIC = 1 << 22;
/// Support for atomic operations on images.
const TEXTURE_ATOMIC = 1 << 22;
const TEXTURE_ATOMIC = 1 << 23;
/// Support for atomic operations on 64-bit images.
const TEXTURE_INT64_ATOMIC = 1 << 23;
const TEXTURE_INT64_ATOMIC = 1 << 24;
}
}

Expand Down
7 changes: 2 additions & 5 deletions tests/tests/binding_array/buffers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ use wgpu_test::{gpu_test, FailureCase, GpuTestConfiguration, TestParameters, Tes
static BINDING_ARRAY_UNIFORM_BUFFERS: GpuTestConfiguration = GpuTestConfiguration::new()
.parameters(
TestParameters::default()
.features(
Features::BUFFER_BINDING_ARRAY
| Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
)
.features(Features::BUFFER_BINDING_ARRAY | Features::UNIFORM_BUFFER_INDEXING)
.limits(Limits {
max_uniform_buffers_per_shader_stage: 16,
..Limits::default()
Expand All @@ -31,7 +28,7 @@ static PARTIAL_BINDING_ARRAY_UNIFORM_BUFFERS: GpuTestConfiguration = GpuTestConf
.features(
Features::BUFFER_BINDING_ARRAY
| Features::PARTIALLY_BOUND_BINDING_ARRAY
| Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
| Features::UNIFORM_BUFFER_INDEXING,
)
.limits(Limits {
max_uniform_buffers_per_shader_stage: 32,
Expand Down
4 changes: 2 additions & 2 deletions tests/tests/binding_array/storage_textures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ static BINDING_ARRAY_STORAGE_TEXTURES: GpuTestConfiguration = GpuTestConfigurati
.features(
Features::TEXTURE_BINDING_ARRAY
| Features::STORAGE_RESOURCE_BINDING_ARRAY
| Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES,
)
.limits(Limits {
Expand All @@ -32,7 +32,7 @@ static PARTIAL_BINDING_ARRAY_STORAGE_TEXTURES: GpuTestConfiguration = GpuTestCon
Features::TEXTURE_BINDING_ARRAY
| Features::PARTIALLY_BOUND_BINDING_ARRAY
| Features::STORAGE_RESOURCE_BINDING_ARRAY
| Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES,
)
.limits(Limits {
Expand Down
9 changes: 6 additions & 3 deletions wgpu-core/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,12 @@ pub fn create_validator(
.contains(wgt::Features::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING),
);
caps.set(
Caps::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
features
.contains(wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING),
Caps::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
features.contains(wgt::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING),
);
caps.set(
Caps::UNIFORM_BUFFER_ARRAY_NON_UNIFORM_INDEXING,
features.contains(wgt::Features::UNIFORM_BUFFER_INDEXING),
);
// TODO: This needs a proper wgpu feature
caps.set(
Expand Down
2 changes: 1 addition & 1 deletion wgpu-hal/src/dx12/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ impl super::Adapter {
features.set(
wgt::Features::TEXTURE_BINDING_ARRAY
| wgt::Features::STORAGE_RESOURCE_BINDING_ARRAY
| wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING,
shader_model >= naga::back::hlsl::ShaderModel::V5_1,
);
Expand Down
2 changes: 1 addition & 1 deletion wgpu-hal/src/metal/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ impl super::PrivateCapabilities {
features.set(
F::TEXTURE_BINDING_ARRAY
| F::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING
| F::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| F::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to vulkan, shouldn't the F::UNIFORM_BUFFER_INDEXING be set somewhere here as well? In wgpu-types you specified with MSL 2.0+ on macOS 10.13+ as the condition

| F::PARTIALLY_BOUND_BINDING_ARRAY,
self.msl_version >= MTLLanguageVersion::V3_0
&& self.supports_arrays_of_textures
Expand Down
90 changes: 39 additions & 51 deletions wgpu-hal/src/vulkan/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ fn depth_stencil_required_flags() -> vk::FormatFeatureFlags {
//TODO: const fn?
fn indexing_features() -> wgt::Features {
wgt::Features::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::UNIFORM_BUFFER_INDEXING
| wgt::Features::PARTIALLY_BOUND_BINDING_ARRAY
}

Expand Down Expand Up @@ -217,14 +218,12 @@ impl PhysicalDeviceFeatures {
| wgt::Features::STORAGE_RESOURCE_BINDING_ARRAY
| wgt::Features::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING,
);
let needs_uniform_buffer_non_uniform = requested_features.contains(
wgt::Features::TEXTURE_BINDING_ARRAY
| wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
);
let needs_uniform_buffer_non_uniform =
requested_features.contains(wgt::Features::UNIFORM_BUFFER_INDEXING);
let needs_storage_image_non_uniform = requested_features.contains(
wgt::Features::TEXTURE_BINDING_ARRAY
| wgt::Features::STORAGE_RESOURCE_BINDING_ARRAY
| wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
| wgt::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
);
let needs_partially_bound =
requested_features.intersects(wgt::Features::PARTIALLY_BOUND_BINDING_ARRAY);
Expand Down Expand Up @@ -498,7 +497,6 @@ impl PhysicalDeviceFeatures {
phd: vk::PhysicalDevice,
caps: &PhysicalDeviceProperties,
) -> (wgt::Features, wgt::DownlevelFlags) {
use crate::auxil::db;
use wgt::{DownlevelFlags as Df, Features as F};
let mut features = F::empty()
| F::SPIRV_SHADER_PASSTHROUGH
Expand Down Expand Up @@ -583,27 +581,7 @@ impl PhysicalDeviceFeatures {
F::VERTEX_WRITABLE_STORAGE,
self.core.vertex_pipeline_stores_and_atomics != 0,
);
//if self.core.shader_image_gather_extended != 0 {
//if self.core.shader_storage_image_extended_formats != 0 {
features.set(
F::BUFFER_BINDING_ARRAY,
self.core.shader_uniform_buffer_array_dynamic_indexing != 0,
);
features.set(
F::TEXTURE_BINDING_ARRAY,
self.core.shader_sampled_image_array_dynamic_indexing != 0,
);
features.set(F::SHADER_PRIMITIVE_INDEX, self.core.geometry_shader != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that support for SHADER_PRIMITIVE_INDEX appears dropped here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, good catch

features.set(
F::STORAGE_RESOURCE_BINDING_ARRAY,
(features.contains(F::BUFFER_BINDING_ARRAY)
&& self.core.shader_storage_buffer_array_dynamic_indexing != 0)
|| (features.contains(F::TEXTURE_BINDING_ARRAY)
&& self.core.shader_storage_image_array_dynamic_indexing != 0),
);
//if self.core.shader_storage_image_array_dynamic_indexing != 0 {
//if self.core.shader_clip_distance != 0 {
//if self.core.shader_cull_distance != 0 {

features.set(F::SHADER_F64, self.core.shader_float64 != 0);
features.set(F::SHADER_INT64, self.core.shader_int64 != 0);
features.set(F::SHADER_I16, self.core.shader_int16 != 0);
Expand Down Expand Up @@ -645,29 +623,38 @@ impl PhysicalDeviceFeatures {
caps.supports_extension(ext::conservative_rasterization::NAME),
);

let intel_windows = caps.properties.vendor_id == db::intel::VENDOR && cfg!(windows);

if let Some(ref descriptor_indexing) = self.descriptor_indexing {
const STORAGE: F = F::STORAGE_RESOURCE_BINDING_ARRAY;
features.set(
F::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING,
(features.contains(F::TEXTURE_BINDING_ARRAY)
&& descriptor_indexing.shader_sampled_image_array_non_uniform_indexing != 0)
&& (features.contains(F::BUFFER_BINDING_ARRAY | STORAGE)
&& descriptor_indexing.shader_storage_buffer_array_non_uniform_indexing
!= 0),
);
features.set(
F::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
(features.contains(F::BUFFER_BINDING_ARRAY)
&& descriptor_indexing.shader_uniform_buffer_array_non_uniform_indexing != 0)
&& (features.contains(F::TEXTURE_BINDING_ARRAY | STORAGE)
&& descriptor_indexing.shader_storage_image_array_non_uniform_indexing
!= 0),
);
if descriptor_indexing.descriptor_binding_partially_bound != 0 && !intel_windows {
features |= F::PARTIALLY_BOUND_BINDING_ARRAY;
}
// We use update-after-bind descriptors for all bind groups containing binding arrays.
//
// In those bind groups, we allow all binding types except uniform buffers to be present.
//
// As we can only switch between update-after-bind and not on a per bind group basis,
// all supported binding types need to be able to be marked update after bind.
//
// As such, we enable all features as a whole, rather individually.
let supports_descriptor_indexing =
// Sampled Images
descriptor_indexing.shader_sampled_image_array_non_uniform_indexing != 0
&& descriptor_indexing.descriptor_binding_sampled_image_update_after_bind != 0
// Storage Images
&& descriptor_indexing.shader_storage_image_array_non_uniform_indexing != 0
&& descriptor_indexing.descriptor_binding_storage_image_update_after_bind != 0
// Storage Buffers
&& descriptor_indexing.shader_storage_buffer_array_non_uniform_indexing != 0
&& descriptor_indexing.descriptor_binding_storage_buffer_update_after_bind != 0;

let descriptor_indexing_features = F::BUFFER_BINDING_ARRAY
| F::TEXTURE_BINDING_ARRAY
| F::STORAGE_RESOURCE_BINDING_ARRAY
| F::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING
| F::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING;

features.set(descriptor_indexing_features, supports_descriptor_indexing);

let supports_partially_bound =
descriptor_indexing.descriptor_binding_partially_bound != 0;

features.set(F::PARTIALLY_BOUND_BINDING_ARRAY, supports_partially_bound);
}
Comment on lines 626 to 658
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but I can't see anywhere where this would set F::UNIFORM_BUFFER_INDEXING. According to the comment block that would be a separate feature that needs to be checked


features.set(F::DEPTH_CLIP_CONTROL, self.core.depth_clamp != 0);
Expand Down Expand Up @@ -1835,7 +1822,8 @@ impl super::Adapter {

if features.intersects(
wgt::Features::SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::UNIFORM_BUFFER_AND_STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING,
| wgt::Features::STORAGE_TEXTURE_ARRAY_NON_UNIFORM_INDEXING
| wgt::Features::UNIFORM_BUFFER_INDEXING,
) {
capabilities.push(spv::Capability::ShaderNonUniform);
}
Expand Down
Loading
Loading