From 70c925d3efdb84569d74ef4d7bd40de42d531418 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Sun, 5 May 2024 12:00:32 +0200 Subject: [PATCH] Command encoder goes now into locked state while compute pass is open --- tests/src/lib.rs | 11 +- ...ownership.rs => compute_pass_ownership.rs} | 9 +- tests/tests/encoder.rs | 241 +++++++++++++++++- tests/tests/root.rs | 2 +- wgpu-core/src/command/clear.rs | 10 +- wgpu-core/src/command/compute.rs | 6 +- wgpu-core/src/command/mod.rs | 90 ++++++- wgpu-core/src/registry.rs | 12 +- 8 files changed, 358 insertions(+), 23 deletions(-) rename tests/tests/{compute_pass_resource_ownership.rs => compute_pass_ownership.rs} (93%) diff --git a/tests/src/lib.rs b/tests/src/lib.rs index 9df1edfd76c..509c12e321d 100644 --- a/tests/src/lib.rs +++ b/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(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(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 } diff --git a/tests/tests/compute_pass_resource_ownership.rs b/tests/tests/compute_pass_ownership.rs similarity index 93% rename from tests/tests/compute_pass_resource_ownership.rs rename to tests/tests/compute_pass_ownership.rs index 27b4f705c51..dc9eba62fa1 100644 --- a/tests/tests/compute_pass_resource_ownership.rs +++ b/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 ------------------------------------------------------------ diff --git a/tests/tests/encoder.rs b/tests/tests/encoder.rs index 3858e3d0703..81aaf557dc4 100644 --- a/tests/tests/encoder.rs +++ b/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)> = 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." + ); + } +} diff --git a/tests/tests/root.rs b/tests/tests/root.rs index ba5e020791c..178e50b7493 100644 --- a/tests/tests/root.rs +++ b/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; diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index faff1779287..9ef0f24d475 100644 --- a/wgpu-core/src/command/clear.rs +++ b/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(); diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index dbd80e398c6..5f463e179df 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -305,6 +305,8 @@ 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( &self, encoder_id: id::CommandEncoderId, @@ -312,7 +314,7 @@ impl Global { ) -> (ComputePass, Option) { 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() diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index 433d7e63f0f..66a002c9cb3 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -50,10 +50,23 @@ pub(crate) enum CommandEncoderStatus { /// [`compute_pass_end`] require the encoder to be in this /// state. /// + /// This corresponds to WebGPU's "open" state. + /// See + /// /// [`command_encoder_clear_buffer`]: Global::command_encoder_clear_buffer /// [`compute_pass_end`]: Global::compute_pass_end Recording, + /// Locked by a render or compute pass. + /// + /// This state is entered when a render/compute pass is created, + /// and exited when the pass is ended. + /// + /// As long as the command encoder is locked, any command building operation on it will fail + /// and put the encoder into the [`CommandEncoderStatus::Error`] state. + /// See + Locked, + /// Command recording is complete, and the buffer is ready for submission. /// /// [`Global::command_encoder_finish`] transitions a @@ -427,15 +440,75 @@ impl CommandBuffer { ) -> Result, CommandEncoderError> { let storage = hub.command_buffers.read(); match storage.get(id.into_command_buffer_id()) { - Ok(cmd_buf) => match cmd_buf.data.lock().as_ref().unwrap().status { - CommandEncoderStatus::Recording => Ok(cmd_buf.clone()), - CommandEncoderStatus::Finished => Err(CommandEncoderError::NotRecording), - CommandEncoderStatus::Error => Err(CommandEncoderError::Invalid), - }, + Ok(cmd_buf) => { + let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_buf_data = cmd_buf_data.as_mut().unwrap(); + match cmd_buf_data.status { + CommandEncoderStatus::Recording => Ok(cmd_buf.clone()), + CommandEncoderStatus::Locked => { + // Any operation on a locked encoder is required to put it into the invalid/error state. + // See https://www.w3.org/TR/webgpu/#encoder-state-locked + cmd_buf_data.encoder.discard(); + cmd_buf_data.status = CommandEncoderStatus::Error; + Err(CommandEncoderError::Locked) + } + CommandEncoderStatus::Finished => Err(CommandEncoderError::NotRecording), + CommandEncoderStatus::Error => Err(CommandEncoderError::Invalid), + } + } Err(_) => Err(CommandEncoderError::Invalid), } } + /// Return the [`CommandBuffer`] for `id` and if successful puts it into the [`CommandEncoderStatus::Locked`] state. + /// + /// See [`CommandBuffer::get_encoder`]. + /// Call [`CommandBuffer::unlock_encoder`] to put the [`CommandBuffer`] back into the [`CommandEncoderStatus::Recording`] state. + fn lock_encoder( + hub: &Hub, + id: id::CommandEncoderId, + ) -> Result, CommandEncoderError> { + let storage = hub.command_buffers.read(); + match storage.get(id.into_command_buffer_id()) { + Ok(cmd_buf) => { + let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_buf_data = cmd_buf_data.as_mut().unwrap(); + match cmd_buf_data.status { + CommandEncoderStatus::Recording => { + cmd_buf_data.status = CommandEncoderStatus::Locked; + Ok(cmd_buf.clone()) + } + CommandEncoderStatus::Locked => { + cmd_buf_data.encoder.discard(); + cmd_buf_data.status = CommandEncoderStatus::Error; + Err(CommandEncoderError::Locked) + } + CommandEncoderStatus::Finished => Err(CommandEncoderError::NotRecording), + CommandEncoderStatus::Error => Err(CommandEncoderError::Invalid), + } + } + Err(_) => Err(CommandEncoderError::Invalid), + } + } + + /// Unlocks the [`CommandBuffer`] for `id` and puts it back into the [`CommandEncoderStatus::Recording`] state. + /// + /// This function is the counterpart to [`CommandBuffer::lock_encoder`]. + /// It is only valid to call this function if the encoder is in the [`CommandEncoderStatus::Locked`] state. + fn unlock_encoder(&self) -> Result<(), CommandEncoderError> { + let mut data_lock = self.data.lock(); + let status = &mut data_lock.as_mut().unwrap().status; + match *status { + CommandEncoderStatus::Recording => Err(CommandEncoderError::Invalid), + CommandEncoderStatus::Locked => { + *status = CommandEncoderStatus::Recording; + Ok(()) + } + CommandEncoderStatus::Finished => Err(CommandEncoderError::Invalid), + CommandEncoderStatus::Error => Err(CommandEncoderError::Invalid), + } + } + pub fn is_finished(&self) -> bool { match self.data.lock().as_ref().unwrap().status { CommandEncoderStatus::Finished => true, @@ -577,6 +650,8 @@ pub enum CommandEncoderError { NotRecording, #[error(transparent)] Device(#[from] DeviceError), + #[error("Command encoder is locked by a previously created render/compute pass. Before recording any new commands, the pass must be ended.")] + Locked, } impl Global { @@ -605,6 +680,11 @@ impl Global { None } } + CommandEncoderStatus::Locked => { + cmd_buf_data.encoder.discard(); + cmd_buf_data.status = CommandEncoderStatus::Error; + Some(CommandEncoderError::Locked) + } CommandEncoderStatus::Finished => Some(CommandEncoderError::NotRecording), CommandEncoderStatus::Error => { cmd_buf_data.encoder.discard(); diff --git a/wgpu-core/src/registry.rs b/wgpu-core/src/registry.rs index e898ccdce4c..96eaeca66e1 100644 --- a/wgpu-core/src/registry.rs +++ b/wgpu-core/src/registry.rs @@ -176,8 +176,14 @@ impl Registry { let guard = self.storage.read(); let type_name = guard.kind(); - match guard.get(id) { - Ok(res) => { + + // Using `get` over `try_get` is fine for the most part. + // However, there's corner cases where it can happen that a resource still holds an Ar + // to another resource that was already dropped explicitly from the registry. + // That resource is now in an invalid state, likely causing an error that lead + // us here, trying to print its label but failing because the id is now vacant. + match guard.try_get(id) { + Ok(Some(res)) => { let label = res.label(); if label.is_empty() { format!("<{}-{:?}>", type_name, id.unzip()) @@ -185,7 +191,7 @@ impl Registry { label } } - Err(_) => format!( + _ => format!( "", type_name, guard.label_for_invalid_id(id)