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

cwfitzgerald · 2024-12-23T07:42:09Z

Closes #6737
Closes #6606

Update After Bind x

cwfitzgerald · 2025-01-21T00:32:57Z

This PR is a bit of a mess, but I think it's broadly correct and the following PRs will further refine feature specifics. Specifically, we're moving towards two features for bindless, not many. One for "Baseline Bindless" which gets you non-uniform indexing of Storage Textures, Storage Buffers, and Sampled Textures. Then one feature for non-uniform indexing of uniform buffers, if people end up wanting that.

magcius · 2025-01-24T04:52:42Z

wgpu-hal/src/vulkan/adapter.rs

-            F::TEXTURE_BINDING_ARRAY,
-            self.core.shader_sampled_image_array_dynamic_indexing != 0,
-        );
-        features.set(F::SHADER_PRIMITIVE_INDEX, self.core.geometry_shader != 0);


Is it intentional that support for SHADER_PRIMITIVE_INDEX appears dropped here?

Nope, good catch

Wumpf

I'm a bit confused: The new split out UNIFORM_BUFFER_INDEXING feature is never set, thus we never pass the new capability to naga, therefore it should now always reject uniform buffer indexing as its analyzer already correctly distinguishes that now to emit the (new) naga capability as required.

Wumpf · 2025-01-25T16:14:45Z

wgpu-hal/src/vulkan/adapter.rs

        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);
        }


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

Wumpf · 2025-01-25T16:16:01Z

wgpu-hal/src/metal/adapter.rs

@@ -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


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

Wumpf · 2025-01-25T16:20:50Z

deno_webgpu/lib.rs

-        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) {


missing the new feature, it's not exposed here

Wumpf · 2025-01-25T16:21:02Z

deno_webgpu/lib.rs

@@ -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,


as above, new feature is missing here

cwfitzgerald requested a review from a team as a code owner December 23, 2024 07:42

This comment was marked as outdated.

Sign in to view

cwfitzgerald marked this pull request as draft December 24, 2024 00:38

cwfitzgerald force-pushed the cw/use-update-after-bind-for-binding-arrays branch 5 times, most recently from 88b1ce1 to 78549c4 Compare January 19, 2025 21:51

cwfitzgerald mentioned this pull request Jan 19, 2025

Add New Binding Array Specific Limits #6952

Draft

Use Update After Bind Descriptors for Bind Groups With Binding Arrays

56d33e8

Update After Bind x

cwfitzgerald force-pushed the cw/use-update-after-bind-for-binding-arrays branch from 78549c4 to 56d33e8 Compare January 21, 2025 00:26

cwfitzgerald marked this pull request as ready for review January 21, 2025 00:30

cwfitzgerald requested a review from a team January 21, 2025 00:30

cwfitzgerald requested a review from crowlKats as a code owner January 21, 2025 00:30

cwfitzgerald assigned Wumpf Jan 22, 2025

magcius reviewed Jan 24, 2025

View reviewed changes

BenjaminBrienen mentioned this pull request Jan 25, 2025

Ray tracing support? bevyengine/bevy#639

Open

Wumpf requested changes Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cwfitzgerald commented Dec 23, 2024 •

edited

Loading

This comment was marked as outdated.

cwfitzgerald commented Jan 21, 2025

magcius Jan 24, 2025

cwfitzgerald Jan 24, 2025

Wumpf left a comment

Wumpf Jan 25, 2025

Wumpf Jan 25, 2025

Wumpf Jan 25, 2025

Wumpf Jan 25, 2025

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

Are you sure you want to change the base?

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

Conversation

cwfitzgerald commented Dec 23, 2024 • edited Loading

This comment was marked as outdated.

cwfitzgerald commented Jan 21, 2025

magcius Jan 24, 2025

Choose a reason for hiding this comment

cwfitzgerald Jan 24, 2025

Choose a reason for hiding this comment

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf Jan 25, 2025

Choose a reason for hiding this comment

Wumpf Jan 25, 2025

Choose a reason for hiding this comment

Wumpf Jan 25, 2025

Choose a reason for hiding this comment

Wumpf Jan 25, 2025

Choose a reason for hiding this comment

cwfitzgerald commented Dec 23, 2024 •

edited

Loading