Defer some bind group processing until draw/dispatch

This allows us to save some work when a bind group is replaced,
although WebGPU still requires us to fail a submission if any
resource in any bind group that was ever referenced during
encoding is destroyed.

Fixes gfx-rs#8399

Author: andyleiserson

cts_runner/test.lst

@@ -125,7 +125,13 @@ webgpu:api,validation,render_pass,render_pass_descriptor:color_attachments,depth
 // FAIL: webgpu:api,validation,render_pass,render_pass_descriptor:color_attachments,depthSlice,overlaps,diff_miplevel:*
 webgpu:api,validation,render_pass,render_pass_descriptor:resolveTarget,*
 webgpu:api,validation,render_pass,resolve:resolve_attachment:*
+webgpu:api,validation,resource_usages,buffer,in_pass_encoder:*
+// FAIL: 8 other cases in resource_usages,texture,in_pass_encoder. https://github.com/gfx-rs/wgpu/issues/3126
+webgpu:api,validation,resource_usages,texture,in_pass_encoder:scope,*
+webgpu:api,validation,resource_usages,texture,in_pass_encoder:shader_stages_and_visibility,*
+webgpu:api,validation,resource_usages,texture,in_pass_encoder:subresources_and_binding_types_combination_for_aspect:*
 webgpu:api,validation,resource_usages,texture,in_pass_encoder:subresources_and_binding_types_combination_for_color:compute=false;type0="render-target";type1="render-target"
+webgpu:api,validation,resource_usages,texture,in_pass_encoder:unused_bindings_in_pipeline:*
 webgpu:api,validation,texture,rg11b10ufloat_renderable:*
 webgpu:api,operation,render_pipeline,overrides:*
 webgpu:api,operation,rendering,basic:clear:*

wgpu-core/src/command/bind.rs

@@ -1,3 +1,5 @@
+use core::{iter::zip, ops::Range};
+
 use alloc::{boxed::Box, sync::Arc, vec::Vec};
 
 use arrayvec::ArrayVec;
@@ -192,14 +194,16 @@ mod compat {
     }
 
     #[derive(Debug, Default)]
-    pub(crate) struct BoundBindGroupLayouts {
+    pub(super) struct BoundBindGroupLayouts {
         entries: ArrayVec<Entry, { hal::MAX_BIND_GROUPS }>,
+        rebind_start: usize,
     }
 
     impl BoundBindGroupLayouts {
         pub fn new() -> Self {
             Self {
                 entries: (0..hal::MAX_BIND_GROUPS).map(|_| Entry::empty()).collect(),
+                rebind_start: 0,
             }
         }
 
@@ -211,15 +215,19 @@ mod compat {
                 .unwrap_or(self.entries.len())
         }
 
-        fn make_range(&self, start_index: usize) -> Range<usize> {
+        /// Get the range of entries that needs to be rebound, and clears it.
+        pub fn take_rebind_range(&mut self) -> Range<usize> {
             let end = self.num_valid_entries();
-            start_index..end.max(start_index)
+            let start = self.rebind_start;
+            self.rebind_start = end;
+            start..end.max(start)
+        }
+
+        pub fn update_start_index(&mut self, start_index: usize) {
+            self.rebind_start = self.rebind_start.min(start_index);
         }
 
-        pub fn update_expectations(
-            &mut self,
-            expectations: &[Arc<BindGroupLayout>],
-        ) -> Range<usize> {
+        pub fn update_expectations(&mut self, expectations: &[Arc<BindGroupLayout>]) {
             let start_index = self
                 .entries
                 .iter()
@@ -237,12 +245,12 @@ mod compat {
             for e in self.entries[expectations.len()..].iter_mut() {
                 e.expected = None;
             }
-            self.make_range(start_index)
+            self.update_start_index(start_index);
         }
 
-        pub fn assign(&mut self, index: usize, value: Arc<BindGroupLayout>) -> Range<usize> {
+        pub fn assign(&mut self, index: usize, value: Arc<BindGroupLayout>) {
             self.entries[index].assigned = Some(value);
-            self.make_range(index)
+            self.update_start_index(index);
         }
 
         pub fn list_active(&self) -> impl Iterator<Item = usize> + '_ {
@@ -333,10 +341,10 @@ impl Binder {
         &'a mut self,
         new: &Arc<PipelineLayout>,
         late_sized_buffer_groups: &[LateSizedBufferGroup],
-    ) -> (usize, &'a [EntryPayload]) {
+    ) {
         let old_id_opt = self.pipeline_layout.replace(new.clone());
 
-        let mut bind_range = self.manager.update_expectations(&new.bind_group_layouts);
+        self.manager.update_expectations(&new.bind_group_layouts);
 
         // Update the buffer binding sizes that are required by shaders.
         for (payload, late_group) in self.payloads.iter_mut().zip(late_sized_buffer_groups) {
@@ -363,19 +371,17 @@ impl Binder {
         if let Some(old) = old_id_opt {
             // root constants are the base compatibility property
             if old.push_constant_ranges != new.push_constant_ranges {
-                bind_range.start = 0;
+                self.manager.update_start_index(0);
             }
         }
-
-        (bind_range.start, &self.payloads[bind_range])
     }
 
     pub(super) fn assign_group<'a>(
         &'a mut self,
         index: usize,
         bind_group: &Arc<BindGroup>,
         offsets: &[wgt::DynamicOffset],
-    ) -> &'a [EntryPayload] {
+    ) {
         let payload = &mut self.payloads[index];
         payload.group = Some(bind_group.clone());
         payload.dynamic_offsets.clear();
@@ -401,8 +407,20 @@ impl Binder {
             }
         }
 
-        let bind_range = self.manager.assign(index, bind_group.layout.clone());
-        &self.payloads[bind_range]
+        self.manager.assign(index, bind_group.layout.clone());
+    }
+
+    /// Get the range of entries that needs to be rebound, and clears it.
+    pub(super) fn take_rebind_range(&mut self) -> Range<usize> {
+        self.manager.take_rebind_range()
+    }
+
+    pub(super) fn entries(
+        &self,
+        range: Range<usize>,
+    ) -> impl ExactSizeIterator<Item = (usize, &'_ EntryPayload)> + '_ {
+        let payloads = &self.payloads[range.clone()];
+        zip(range, payloads)
     }
 
     pub(super) fn list_active<'a>(&'a self) -> impl Iterator<Item = &'a Arc<BindGroup>> + 'a {

wgpu-core/src/command/compute.rs

@@ -7,7 +7,10 @@ use wgt::{
 use alloc::{borrow::Cow, boxed::Box, sync::Arc, vec::Vec};
 use core::{convert::Infallible, fmt, str};
 
-use crate::{api_log, binding_model::BindError, resource::RawResourceAccess};
+use crate::{
+    api_log, binding_model::BindError, command::pass::flush_bindings_helper,
+    resource::RawResourceAccess,
+};
 use crate::{
     binding_model::{LateMinBufferBindingSizeMismatch, PushConstantUploadError},
     command::{
@@ -280,27 +283,68 @@ impl<'scope, 'snatch_guard, 'cmd_enc> State<'scope, 'snatch_guard, 'cmd_enc> {
         }
     }
 
-    // `extra_buffer` is there to represent the indirect buffer that is also
-    // part of the usage scope.
-    fn flush_states(
+    /// Flush binding state in preparation for a dispatch.
+    ///
+    /// # Differences between render and compute passes
+    ///
+    /// There are differences between the `flush_bindings` implementations for
+    /// render and compute passes, because render passes have a single usage
+    /// scope for the entire pass, and compute passes have a separate usage
+    /// scope for each dispatch.
+    ///
+    /// For compute passes, bind groups are merged into a fresh usage scope
+    /// here, not into the pass usage scope within calls to `set_bind_group`. As
+    /// specified by WebGPU, for compute passes, we merge only the bind groups
+    /// that are actually used by the pipeline, unlike render passes, which
+    /// merge every bind group that is ever set, even if it is not ultimately
+    /// used by the pipeline.
+    ///
+    /// For compute passes, we call `drain_barriers` here, because barriers may
+    /// be needed before each dispatch if a previous dispatch had a conflicting
+    /// usage. For render passes, barriers are emitted once at the start of the
+    /// render pass.
+    ///
+    /// # Indirect buffer handling
+    ///
+    /// For indirect dispatches without validation, pass both `indirect_buffer`
+    /// and `indirect_buffer_index_if_not_validating`. The indirect buffer will
+    /// be added to the usage scope and the tracker.
+    ///
+    /// For indirect dispatches with validation, pass only `indirect_buffer`.
+    /// The indirect buffer will be added to the usage scope to detect usage
+    /// conflicts. The indirect buffer does not need to be added to the tracker;
+    /// the indirect validation code handles transitions manually.
+    fn flush_bindings(
         &mut self,
-        indirect_buffer: Option<TrackerIndex>,
-    ) -> Result<(), ResourceUsageCompatibilityError> {
+        indirect_buffer: Option<&Arc<Buffer>>,
+        indirect_buffer_index_if_not_validating: Option<TrackerIndex>,
+    ) -> Result<(), ComputePassErrorInner> {
+        let mut scope = self.pass.base.device.new_usage_scope();
+
         for bind_group in self.pass.binder.list_active() {
-            unsafe { self.pass.scope.merge_bind_group(&bind_group.used)? };
-            // Note: stateless trackers are not merged: the lifetime reference
-            // is held to the bind group itself.
+            unsafe { scope.merge_bind_group(&bind_group.used)? };
         }
 
-        for bind_group in self.pass.binder.list_active() {
-            self.intermediate_trackers
-                .set_from_bind_group(&mut self.pass.scope, &bind_group.used);
+        // When indirect validation is turned on, our actual use of the buffer
+        // is `STORAGE_READ_ONLY`, but for usage scope validation, we still want
+        // to treat it as indirect so we can detect the conflicts prescribed by
+        // WebGPU. The usage scope we construct here never leaves this function
+        // (and is not used to populate a tracker), so it's fine to do this.
+        if let Some(buffer) = indirect_buffer {
+            scope
+                .buffers
+                .merge_single(buffer, wgt::BufferUses::INDIRECT)?;
         }
 
-        // Add the state of the indirect buffer if it hasn't been hit before.
+        // Add the state of the indirect buffer, if needed (see above).
         self.intermediate_trackers
             .buffers
-            .set_multiple(&mut self.pass.scope.buffers, indirect_buffer);
+            .set_multiple(&mut scope.buffers, indirect_buffer_index_if_not_validating);
+
+        flush_bindings_helper(&mut self.pass, |bind_group| {
+            self.intermediate_trackers
+                .set_from_bind_group(&mut scope, &bind_group.used)
+        })?;
 
         CommandEncoder::drain_barriers(
             self.pass.base.raw_encoder,
@@ -821,7 +865,7 @@ fn set_pipeline(
     }
 
     // Rebind resources
-    pass::rebind_resources::<ComputePassErrorInner, _>(
+    pass::change_pipeline_layout::<ComputePassErrorInner, _>(
         &mut state.pass,
         &pipeline.layout,
         &pipeline.late_sized_buffer_groups,
@@ -850,7 +894,7 @@ fn dispatch(state: &mut State, groups: [u32; 3]) -> Result<(), ComputePassErrorI
 
     state.is_ready()?;
 
-    state.flush_states(None)?;
+    state.flush_bindings(None, None)?;
 
     let groups_size_limit = state
         .pass
@@ -1051,7 +1095,7 @@ fn dispatch_indirect(
                 }]);
         }
 
-        state.flush_states(None)?;
+        state.flush_bindings(Some(&buffer), None)?;
         unsafe {
             state
                 .pass
@@ -1060,14 +1104,8 @@ fn dispatch_indirect(
                 .dispatch_indirect(params.dst_buffer, 0);
         }
     } else {
-        state
-            .pass
-            .scope
-            .buffers
-            .merge_single(&buffer, wgt::BufferUses::INDIRECT)?;
-
         use crate::resource::Trackable;
-        state.flush_states(Some(buffer.tracker_index()))?;
+        state.flush_bindings(Some(&buffer), Some(buffer.tracker_index()))?;
 
         let buf_raw = buffer.try_raw(state.pass.base.snatch_guard)?;
         unsafe {