Command encoder goes now into locked state while compute pass is open

tests/src/lib.rs

@@ -28,9 +28,18 @@ pub use wgpu_macros::gpu_test;
 
 /// Run some code in an error scope and assert that validation fails.
 pub fn fail<T>(device: &wgpu::Device, callback: impl FnOnce() -> T) -> T {
+    fail_with(device, callback, "Validation error expected")
+}
+
+/// Run some code in an error scope and assert that validation fails.
+pub fn fail_with<T>(device: &wgpu::Device, callback: impl FnOnce() -> T, message: &str) -> T {
     device.push_error_scope(wgpu::ErrorFilter::Validation);
     let result = callback();
-    assert!(pollster::block_on(device.pop_error_scope()).is_some());
+    assert!(
+        pollster::block_on(device.pop_error_scope()).is_some(),
+        "{}",
+        message
+    );
 
     result
 }

tests/tests/compute_pass_resource_ownership.rs → tests/tests/compute_pass_ownership.rs

@@ -1,9 +1,6 @@
 //! Tests that compute passes take ownership of resources that are associated with.
 //! I.e. once a resource is passed in to a compute pass, it can be dropped.
 //!
-//! TODO: Test doesn't check on timestamp writes & pipeline statistics queries yet.
-//!       (Not important as long as they are lifetime constrained to the command encoder,
-//!       but once we lift this constraint, we should add tests for this as well!)
 //! TODO: Also should test resource ownership for:
 //!       * write_timestamp
 //!       * begin_pipeline_statistics_query
@@ -112,9 +109,11 @@ async fn compute_pass_keep_encoder_alive(ctx: TestingContext) {
     cpass.set_bind_group(0, &bind_group, &[]);
     cpass.dispatch_workgroups_indirect(&indirect_buffer, 0);
 
-    // Dropping the encoder should will execute it which should log an error but not panic (with an installed error handler).
+    // Dropping the encoder should will still execute it even though there's no way to submit it.
+    // Ideally, this would log an error, but the encoder is not dropped until the compute pass is dropped,
+    // so this isn't technically an error.
+    // It would be however, if the encoder was explicitly destroyed or finished.
     drop(cpass);
-    // TODO: Check that the error was logged.
 }
 
 // Setup ------------------------------------------------------------

tests/tests/encoder.rs

@@ -1,4 +1,8 @@
-use wgpu_test::{fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters};
+use wgpu::util::DeviceExt;
+use wgpu::CommandEncoder;
+use wgpu_test::{
+    fail, fail_with, gpu_test, FailureCase, GpuTestConfiguration, TestParameters, TestingContext,
+};
 
 #[gpu_test]
 static DROP_ENCODER: GpuTestConfiguration = GpuTestConfiguration::new().run_sync(|ctx| {
@@ -68,3 +72,238 @@ static DROP_ENCODER_AFTER_ERROR: GpuTestConfiguration = GpuTestConfiguration::ne
         // The encoder is still open!
         drop(encoder);
     });
+
+// TODO: This should also apply to render passes once the lifetime bound is lifted.
+#[gpu_test]
+static ENCODER_OPERATIONS_FAIL_WHILE_COMPUTE_PASS_ALIVE: GpuTestConfiguration =
+    GpuTestConfiguration::new()
+        .parameters(TestParameters::default().features(
+            wgpu::Features::CLEAR_TEXTURE
+                | wgpu::Features::TIMESTAMP_QUERY
+                | wgpu::Features::TIMESTAMP_QUERY_INSIDE_ENCODERS,
+        ))
+        .run_sync(encoder_operations_fail_while_compute_pass_alive);
+
+fn encoder_operations_fail_while_compute_pass_alive(ctx: TestingContext) {
+    let buffer_source = ctx
+        .device
+        .create_buffer_init(&wgpu::util::BufferInitDescriptor {
+            label: None,
+            contents: &[0u8; 4],
+            usage: wgpu::BufferUsages::COPY_SRC,
+        });
+    let buffer_dest = ctx
+        .device
+        .create_buffer_init(&wgpu::util::BufferInitDescriptor {
+            label: None,
+            contents: &[0u8; 4],
+            usage: wgpu::BufferUsages::COPY_DST,
+        });
+
+    let texture_desc = wgpu::TextureDescriptor {
+        label: None,
+        size: wgpu::Extent3d {
+            width: 1,
+            height: 1,
+            depth_or_array_layers: 1,
+        },
+        mip_level_count: 1,
+        sample_count: 1,
+        dimension: wgpu::TextureDimension::D2,
+        format: wgpu::TextureFormat::Rgba8Unorm,
+        usage: wgpu::TextureUsages::COPY_DST,
+        view_formats: &[],
+    };
+    let texture_dst = ctx.device.create_texture(&texture_desc);
+    let texture_src = ctx.device.create_texture(&wgpu::TextureDescriptor {
+        usage: wgpu::TextureUsages::COPY_SRC,
+        ..texture_desc
+    });
+    let query_set = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
+        count: 1,
+        ty: wgpu::QueryType::Timestamp,
+        label: None,
+    });
+
+    #[allow(clippy::type_complexity)]
+    let recording_ops: Vec<(_, Box<dyn Fn(&mut CommandEncoder)>)> = vec![
+        (
+            "begin_compute_pass",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+            }),
+        ),
+        (
+            "begin_render_pass",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.begin_render_pass(&wgpu::RenderPassDescriptor::default());
+            }),
+        ),
+        (
+            "copy_buffer_to_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_buffer_to_buffer(&buffer_source, 0, &buffer_dest, 0, 4);
+            }),
+        ),
+        (
+            "copy_buffer_to_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_buffer_to_texture(
+                    wgpu::ImageCopyBuffer {
+                        buffer: &buffer_source,
+                        layout: wgpu::ImageDataLayout {
+                            offset: 0,
+                            bytes_per_row: Some(4),
+                            rows_per_image: None,
+                        },
+                    },
+                    texture_dst.as_image_copy(),
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "copy_texture_to_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_texture_to_buffer(
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_src,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    wgpu::ImageCopyBuffer {
+                        buffer: &buffer_dest,
+                        layout: wgpu::ImageDataLayout {
+                            offset: 0,
+                            bytes_per_row: Some(4),
+                            rows_per_image: None,
+                        },
+                    },
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "copy_texture_to_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_texture_to_texture(
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_src,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_dst,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "clear_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.clear_texture(&texture_dst, &wgpu::ImageSubresourceRange::default());
+            }),
+        ),
+        (
+            "clear_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.clear_buffer(&buffer_dest, 0, None);
+            }),
+        ),
+        (
+            "insert_debug_marker",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.insert_debug_marker("marker");
+            }),
+        ),
+        (
+            "push_debug_group",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.push_debug_group("marker");
+            }),
+        ),
+        (
+            "pop_debug_group",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.pop_debug_group();
+            }),
+        ),
+        (
+            "resolve_query_set",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.resolve_query_set(&query_set, 0..1, &buffer_dest, 0);
+            }),
+        ),
+        (
+            "write_timestamp",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.write_timestamp(&query_set, 0);
+            }),
+        ),
+    ];
+
+    for (op_name, op) in recording_ops.iter() {
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
+
+        let pass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+
+        ctx.device.push_error_scope(wgpu::ErrorFilter::Validation);
+        op(&mut encoder);
+        let Some(wgpu::Error::Validation {
+            source: _,
+            description,
+        }) = pollster::block_on(ctx.device.pop_error_scope())
+        else {
+            panic!(
+                "Expected validation error for calling {} while a pass is open.",
+                op_name
+            );
+        };
+
+        // Make sure it's actually an error about the encoder being locked and not something else!
+        assert!(
+            description.contains("Command encoder is locked"),
+            "Unexpected error: {:?}",
+            description
+        );
+
+        // Drop the pass - this also fails now since the encoder is invalid.
+        fail_with(&ctx.device,
+            || {
+                drop(pass);
+            }, &format!("Expected dropping pass to fail since this ends the pass but the encoder is invalid by now due to prior failed {} call.", op_name),
+        );
+        fail_with(&ctx.device, || {
+                encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+            }, &format!("Expected call to begin_compute_pass fail since a prior call to {} while another pass was still open should have made the encoder invalid.", op_name)
+        );
+    }
+
+    // Test encoder finishing separately since it consumes the encoder and doesn't fit above pattern.
+    {
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
+        let pass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+        fail_with(
+            &ctx.device,
+            || {
+                encoder.finish();
+            },
+            "Expected calling finish on encoder to fail since a pass is still open.",
+        );
+        fail_with(&ctx.device, || {
+                drop(pass);
+            },
+            "Expected dropping pass to fail since this ends the pass but the encoder is invalid by now due to prior failed finish call."
+        );
+    }
+}

tests/tests/root.rs

@@ -11,7 +11,7 @@ mod buffer;
 mod buffer_copy;
 mod buffer_usages;
 mod clear_texture;
-mod compute_pass_resource_ownership;
+mod compute_pass_ownership;
 mod create_surface_error;
 mod device;
 mod encoder;

wgpu-core/src/command/clear.rs

@@ -26,8 +26,6 @@ use wgt::{math::align_to, BufferAddress, BufferUsages, ImageSubresourceRange, Te
 pub enum ClearError {
     #[error("To use clear_texture the CLEAR_TEXTURE feature needs to be enabled")]
     MissingClearTextureFeature,
-    #[error("Command encoder {0:?} is invalid")]
-    InvalidCommandEncoder(CommandEncoderId),
     #[error("Device {0:?} is invalid")]
     InvalidDevice(DeviceId),
     #[error("Buffer {0:?} is invalid or destroyed")]
@@ -74,6 +72,8 @@ whereas subesource range specified start {subresource_base_array_layer} and coun
     },
     #[error(transparent)]
     Device(#[from] DeviceError),
+    #[error(transparent)]
+    CommandEncoderError(#[from] super::CommandEncoderError),
 }
 
 impl Global {
@@ -89,8 +89,7 @@ impl Global {
 
         let hub = A::hub(self);
 
-        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)
-            .map_err(|_| ClearError::InvalidCommandEncoder(command_encoder_id))?;
+        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)?;
         let mut cmd_buf_data = cmd_buf.data.lock();
         let cmd_buf_data = cmd_buf_data.as_mut().unwrap();
 
@@ -183,8 +182,7 @@ impl Global {
 
         let hub = A::hub(self);
 
-        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)
-            .map_err(|_| ClearError::InvalidCommandEncoder(command_encoder_id))?;
+        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)?;
         let mut cmd_buf_data = cmd_buf.data.lock();
         let cmd_buf_data = cmd_buf_data.as_mut().unwrap();
 

wgpu-core/src/command/compute.rs

@@ -305,14 +305,16 @@ impl Global {
     ///
     /// If creation fails, an invalid pass is returned.
     /// Any operation on an invalid pass will return an error.
+    ///
+    /// If successful, puts the encoder into the [`CommandEncoderStatus::Locked`] state.
     pub fn command_encoder_create_compute_pass<A: HalApi>(
         &self,
         encoder_id: id::CommandEncoderId,
         desc: &ComputePassDescriptor,
     ) -> (ComputePass<A>, Option<CommandEncoderError>) {
         let hub = A::hub(self);
 
-        match CommandBuffer::get_encoder(hub, encoder_id) {
+        match CommandBuffer::lock_encoder(hub, encoder_id) {
             Ok(cmd_buf) => (ComputePass::new(Some(cmd_buf), desc), None),
             Err(err) => (ComputePass::new(None, desc), Some(err)),
         }
@@ -340,6 +342,8 @@ impl Global {
             return Err(ComputePassErrorInner::InvalidParentEncoder).map_pass_err(scope);
         };
 
+        parent.unlock_encoder().map_pass_err(scope)?;
+
         let base = pass
             .base
             .take()