diff --git a/deno_webgpu/command_encoder.rs b/deno_webgpu/command_encoder.rs index 318c52a7102..0106b0c0c8d 100644 --- a/deno_webgpu/command_encoder.rs +++ b/deno_webgpu/command_encoder.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use std::cell::RefCell; -use std::sync::atomic::{AtomicBool, Ordering}; use deno_core::cppgc::Ptr; use deno_core::op2; @@ -29,19 +28,11 @@ pub struct GPUCommandEncoder { pub id: wgpu_core::id::CommandEncoderId, pub label: String, - - pub finished: AtomicBool, } impl Drop for GPUCommandEncoder { fn drop(&mut self) { - // Command encoders and command buffers are both the same wgpu object. - // At the time `finished` is set, ownership of the id (and - // responsibility for dropping it) transfers from the encoder to the - // buffer. - if !self.finished.load(Ordering::SeqCst) { - self.instance.command_encoder_drop(self.id); - } + self.instance.command_encoder_drop(self.id); } } @@ -416,34 +407,22 @@ impl GPUCommandEncoder { fn finish( &self, #[webidl] descriptor: crate::command_buffer::GPUCommandBufferDescriptor, - ) -> Result { + ) -> GPUCommandBuffer { let wgpu_descriptor = wgpu_types::CommandBufferDescriptor { label: crate::transform_label(descriptor.label.clone()), }; - // TODO(https://github.com/gfx-rs/wgpu/issues/7812): This is not right, - // it should be a validation error, and it would be nice if we can just - // let wgpu generate it for us. The problem is that if the encoder was - // already finished, we transferred ownership of the id to a command - // buffer, so we have to bail out before we mint a duplicate command - // buffer with the same id below. - if self.finished.fetch_or(true, Ordering::SeqCst) { - return Err(JsErrorBox::type_error( - "The command encoder has already finished.", - )); - } - let (id, err) = self .instance - .command_encoder_finish(self.id, &wgpu_descriptor); + .command_encoder_finish(self.id, &wgpu_descriptor, None); self.error_handler.push_error(err); - Ok(GPUCommandBuffer { + GPUCommandBuffer { instance: self.instance.clone(), id, label: descriptor.label, - }) + } } fn push_debug_group(&self, #[webidl] group_label: String) { diff --git a/deno_webgpu/device.rs b/deno_webgpu/device.rs index 7a124824617..07915431756 100644 --- a/deno_webgpu/device.rs +++ b/deno_webgpu/device.rs @@ -498,7 +498,6 @@ impl GPUDevice { error_handler: self.error_handler.clone(), id, label, - finished: Default::default(), } } diff --git a/player/src/bin/play.rs b/player/src/bin/play.rs index 5d908a3a060..607c0c4f640 100644 --- a/player/src/bin/play.rs +++ b/player/src/bin/play.rs @@ -7,6 +7,7 @@ fn main() { use player::GlobalPlay as _; use wgc::device::trace; + use wgpu_core::identity::IdentityManager; use std::{ fs, @@ -52,7 +53,8 @@ fn main() { .unwrap(); let global = wgc::global::Global::new("player", &wgt::InstanceDescriptor::default()); - let mut command_buffer_id_manager = wgc::identity::IdentityManager::new(); + let mut command_encoder_id_manager = IdentityManager::new(); + let mut command_buffer_id_manager = IdentityManager::new(); #[cfg(feature = "winit")] let surface = unsafe { @@ -102,7 +104,14 @@ fn main() { unsafe { global.device_start_graphics_debugger_capture(device) }; while let Some(action) = actions.pop() { - global.process(device, queue, action, &dir, &mut command_buffer_id_manager); + global.process( + device, + queue, + action, + &dir, + &mut command_encoder_id_manager, + &mut command_buffer_id_manager, + ); } unsafe { global.device_stop_graphics_debugger_capture(device) }; @@ -164,6 +173,7 @@ fn main() { queue, action, &dir, + &mut command_encoder_id_manager, &mut command_buffer_id_manager, ); } diff --git a/player/src/lib.rs b/player/src/lib.rs index 4b805e9d478..8b5040c90fb 100644 --- a/player/src/lib.rs +++ b/player/src/lib.rs @@ -6,7 +6,7 @@ extern crate wgpu_core as wgc; extern crate wgpu_types as wgt; -use wgc::device::trace; +use wgc::{device::trace, identity::IdentityManager}; use std::{borrow::Cow, fs, path::Path}; @@ -15,6 +15,7 @@ pub trait GlobalPlay { &self, encoder: wgc::id::CommandEncoderId, commands: Vec, + command_buffer_id_manager: &mut IdentityManager, ) -> wgc::id::CommandBufferId; fn process( &self, @@ -22,7 +23,8 @@ pub trait GlobalPlay { queue: wgc::id::QueueId, action: trace::Action, dir: &Path, - comb_manager: &mut wgc::identity::IdentityManager, + command_encoder_id_manager: &mut IdentityManager, + command_buffer_id_manager: &mut IdentityManager, ); } @@ -31,6 +33,7 @@ impl GlobalPlay for wgc::global::Global { &self, encoder: wgc::id::CommandEncoderId, commands: Vec, + command_buffer_id_manager: &mut IdentityManager, ) -> wgc::id::CommandBufferId { for command in commands { match command { @@ -172,8 +175,11 @@ impl GlobalPlay for wgc::global::Global { } } } - let (cmd_buf, error) = - self.command_encoder_finish(encoder, &wgt::CommandBufferDescriptor { label: None }); + let (cmd_buf, error) = self.command_encoder_finish( + encoder, + &wgt::CommandBufferDescriptor { label: None }, + Some(command_buffer_id_manager.process()), + ); if let Some(e) = error { panic!("{e}"); } @@ -186,7 +192,8 @@ impl GlobalPlay for wgc::global::Global { queue: wgc::id::QueueId, action: trace::Action, dir: &Path, - comb_manager: &mut wgc::identity::IdentityManager, + command_encoder_id_manager: &mut IdentityManager, + command_buffer_id_manager: &mut IdentityManager, ) { use wgc::device::trace::Action; log::debug!("action {action:?}"); @@ -379,12 +386,12 @@ impl GlobalPlay for wgc::global::Global { let (encoder, error) = self.device_create_command_encoder( device, &wgt::CommandEncoderDescriptor { label: None }, - Some(comb_manager.process().into_command_encoder_id()), + Some(command_encoder_id_manager.process()), ); if let Some(e) = error { panic!("{e}"); } - let cmdbuf = self.encode_commands(encoder, commands); + let cmdbuf = self.encode_commands(encoder, commands, command_buffer_id_manager); self.queue_submit(queue, &[cmdbuf]).unwrap(); } Action::CreateBlas { id, desc, sizes } => { diff --git a/player/tests/player/main.rs b/player/tests/player/main.rs index 18f76e76e2c..aa5eb90e74a 100644 --- a/player/tests/player/main.rs +++ b/player/tests/player/main.rs @@ -20,6 +20,7 @@ use std::{ path::{Path, PathBuf}, slice, }; +use wgc::identity::IdentityManager; #[derive(serde::Deserialize)] struct RawId { @@ -104,7 +105,8 @@ impl Test<'_> { panic!("{e:?}"); } - let mut command_buffer_id_manager = wgc::identity::IdentityManager::new(); + let mut command_encoder_id_manager = IdentityManager::new(); + let mut command_buffer_id_manager = IdentityManager::new(); println!("\t\t\tRunning..."); for action in self.actions { global.process( @@ -112,6 +114,7 @@ impl Test<'_> { queue_id, action, dir, + &mut command_encoder_id_manager, &mut command_buffer_id_manager, ); } diff --git a/tests/tests/wgpu-gpu/mem_leaks.rs b/tests/tests/wgpu-gpu/mem_leaks.rs index 775d3aa5cb4..57df08fc8b3 100644 --- a/tests/tests/wgpu-gpu/mem_leaks.rs +++ b/tests/tests/wgpu-gpu/mem_leaks.rs @@ -179,7 +179,7 @@ async fn draw_test_with_reports( let global_report = ctx.instance.generate_report().unwrap(); let report = global_report.hub_report(); - assert_eq!(report.command_buffers.num_allocated, 1); + assert_eq!(report.command_encoders.num_allocated, 1); assert_eq!(report.buffers.num_allocated, 1); let mut rpass = encoder.begin_render_pass(&wgpu::RenderPassDescriptor { @@ -206,7 +206,7 @@ async fn draw_test_with_reports( assert_eq!(report.pipeline_layouts.num_allocated, 1); assert_eq!(report.render_pipelines.num_allocated, 1); assert_eq!(report.compute_pipelines.num_allocated, 0); - assert_eq!(report.command_buffers.num_allocated, 1); + assert_eq!(report.command_encoders.num_allocated, 1); assert_eq!(report.render_bundles.num_allocated, 0); assert_eq!(report.texture_views.num_allocated, 1); assert_eq!(report.textures.num_allocated, 1); @@ -223,7 +223,7 @@ async fn draw_test_with_reports( let global_report = ctx.instance.generate_report().unwrap(); let report = global_report.hub_report(); - assert_eq!(report.command_buffers.num_kept_from_user, 1); + assert_eq!(report.command_encoders.num_kept_from_user, 1); assert_eq!(report.render_pipelines.num_kept_from_user, 0); assert_eq!(report.pipeline_layouts.num_kept_from_user, 0); assert_eq!(report.bind_group_layouts.num_kept_from_user, 0); @@ -231,7 +231,7 @@ async fn draw_test_with_reports( assert_eq!(report.buffers.num_kept_from_user, 0); assert_eq!(report.texture_views.num_kept_from_user, 0); assert_eq!(report.textures.num_kept_from_user, 0); - assert_eq!(report.command_buffers.num_allocated, 1); + assert_eq!(report.command_encoders.num_allocated, 1); assert_eq!(report.render_pipelines.num_allocated, 0); assert_eq!(report.pipeline_layouts.num_allocated, 0); assert_eq!(report.bind_group_layouts.num_allocated, 0); @@ -240,12 +240,18 @@ async fn draw_test_with_reports( assert_eq!(report.texture_views.num_allocated, 0); assert_eq!(report.textures.num_allocated, 0); - let submit_index = ctx.queue.submit(Some(encoder.finish())); + let command_buffer = encoder.finish(); + + let global_report = ctx.instance.generate_report().unwrap(); + let report = global_report.hub_report(); + assert_eq!(report.command_encoders.num_allocated, 0); + assert_eq!(report.command_buffers.num_allocated, 1); + + let submit_index = ctx.queue.submit(Some(command_buffer)); - // TODO: fix in https://github.com/gfx-rs/wgpu/pull/5141 - // let global_report = ctx.instance.generate_report().unwrap(); - // let report = global_report.hub_report(); - // assert_eq!(report.command_buffers.num_allocated, 0); + let global_report = ctx.instance.generate_report().unwrap(); + let report = global_report.hub_report(); + assert_eq!(report.command_buffers.num_allocated, 0); ctx.async_poll(wgpu::PollType::wait_for(submit_index)) .await diff --git a/wgpu-core/src/as_hal.rs b/wgpu-core/src/as_hal.rs index 8ad9cf3ea2f..390c496236e 100644 --- a/wgpu-core/src/as_hal.rs +++ b/wgpu-core/src/as_hal.rs @@ -346,8 +346,8 @@ impl Global { let hub = &self.hub; - let cmd_buf = hub.command_buffers.get(id.into_command_buffer_id()); - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = hub.command_encoders.get(id); + let mut cmd_buf_data = cmd_enc.data.lock(); cmd_buf_data.record_as_hal_mut(|opt_cmd_buf| -> R { hal_command_encoder_callback(opt_cmd_buf.and_then(|cmd_buf| { cmd_buf diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index e18335fb69d..48896ffa053 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -6,7 +6,7 @@ use crate::device::trace::Command as TraceCommand; use crate::{ api_log, command::EncoderStateError, - device::DeviceError, + device::{DeviceError, MissingFeatures}, get_lowest_common_denom, global::Global, id::{BufferId, CommandEncoderId, TextureId}, @@ -30,10 +30,10 @@ use wgt::{ #[derive(Clone, Debug, Error)] #[non_exhaustive] pub enum ClearError { - #[error("To use clear_texture the CLEAR_TEXTURE feature needs to be enabled")] - MissingClearTextureFeature, #[error(transparent)] DestroyedResource(#[from] DestroyedResourceError), + #[error(transparent)] + MissingFeatures(#[from] MissingFeatures), #[error("{0} can not be cleared")] NoValidTextureClearMode(ResourceErrorIdent), #[error("Buffer clear size {0:?} is not a multiple of `COPY_BUFFER_ALIGNMENT`")] @@ -84,12 +84,12 @@ impl WebGpuError for ClearError { fn webgpu_error_type(&self) -> ErrorType { let e: &dyn WebGpuError = match self { Self::DestroyedResource(e) => e, + Self::MissingFeatures(e) => e, Self::MissingBufferUsage(e) => e, Self::Device(e) => e, Self::EncoderState(e) => e, Self::InvalidResource(e) => e, Self::NoValidTextureClearMode(..) - | Self::MissingClearTextureFeature | Self::UnalignedFillSize(..) | Self::UnalignedBufferOffset(..) | Self::OffsetPlusSizeExceeds64BitBounds { .. } @@ -115,21 +115,19 @@ impl Global { let hub = &self.hub; - let cmd_buf = hub - .command_buffers - .get(command_encoder_id.into_command_buffer_id()); - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = hub.command_encoders.get(command_encoder_id); + let mut cmd_buf_data = cmd_enc.data.lock(); cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> { #[cfg(feature = "trace")] if let Some(ref mut list) = cmd_buf_data.commands { list.push(TraceCommand::ClearBuffer { dst, offset, size }); } - cmd_buf.device.check_is_valid()?; + cmd_enc.device.check_is_valid()?; let dst_buffer = hub.buffers.get(dst).get()?; - dst_buffer.same_device_as(cmd_buf.as_ref())?; + dst_buffer.same_device_as(cmd_enc.as_ref())?; let dst_pending = cmd_buf_data .trackers @@ -202,10 +200,8 @@ impl Global { let hub = &self.hub; - let cmd_buf = hub - .command_buffers - .get(command_encoder_id.into_command_buffer_id()); - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = hub.command_encoders.get(command_encoder_id); + let mut cmd_buf_data = cmd_enc.data.lock(); cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> { #[cfg(feature = "trace")] if let Some(ref mut list) = cmd_buf_data.commands { @@ -215,15 +211,15 @@ impl Global { }); } - cmd_buf.device.check_is_valid()?; + cmd_enc.device.check_is_valid()?; - if !cmd_buf.support_clear_texture { - return Err(ClearError::MissingClearTextureFeature); - } + cmd_enc + .device + .require_features(wgt::Features::CLEAR_TEXTURE)?; let dst_texture = hub.textures.get(dst).get()?; - dst_texture.same_device_as(cmd_buf.as_ref())?; + dst_texture.same_device_as(cmd_enc.as_ref())?; // Check if subresource aspects are valid. let clear_aspects = @@ -260,7 +256,7 @@ impl Global { }); } - let device = &cmd_buf.device; + let device = &cmd_enc.device; device.check_is_valid()?; let (encoder, tracker) = cmd_buf_data.open_encoder_and_tracker()?; diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index e6793a32d7b..16b556c7b95 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -7,7 +7,9 @@ use wgt::{ use alloc::{borrow::Cow, boxed::Box, sync::Arc, vec::Vec}; use core::{fmt, str}; -use crate::command::{pass, EncoderStateError, PassStateError, TimestampWritesError}; +use crate::command::{ + pass, CommandEncoder, EncoderStateError, PassStateError, TimestampWritesError, +}; use crate::resource::DestroyedResourceError; use crate::{binding_model::BindError, resource::RawResourceAccess}; use crate::{ @@ -18,8 +20,8 @@ use crate::{ end_pipeline_statistics_query, memory_init::{fixup_discarded_surfaces, SurfacesInDiscardState}, pass_base, pass_try, validate_and_begin_pipeline_statistics_query, ArcPassTimestampWrites, - BasePass, BindGroupStateChange, CommandBuffer, CommandEncoderError, MapPassErr, - PassErrorScope, PassTimestampWrites, QueryUseError, StateChange, + BasePass, BindGroupStateChange, CommandEncoderError, MapPassErr, PassErrorScope, + PassTimestampWrites, QueryUseError, StateChange, }, device::{DeviceError, MissingDownlevelFlags, MissingFeatures}, global::Global, @@ -46,12 +48,12 @@ pub struct ComputePass { /// All pass data & records is stored here. base: ComputeBasePass, - /// Parent command buffer that this pass records commands into. + /// Parent command encoder that this pass records commands into. /// /// If this is `Some`, then the pass is in WebGPU's "open" state. If it is /// `None`, then the pass is in the "ended" state. /// See - parent: Option>, + parent: Option>, timestamp_writes: Option, @@ -61,8 +63,8 @@ pub struct ComputePass { } impl ComputePass { - /// If the parent command buffer is invalid, the returned pass will be invalid. - fn new(parent: Arc, desc: ArcComputePassDescriptor) -> Self { + /// If the parent command encoder is invalid, the returned pass will be invalid. + fn new(parent: Arc, desc: ArcComputePassDescriptor) -> Self { let ArcComputePassDescriptor { label, timestamp_writes, @@ -78,7 +80,7 @@ impl ComputePass { } } - fn new_invalid(parent: Arc, label: &Label, err: ComputePassError) -> Self { + fn new_invalid(parent: Arc, label: &Label, err: ComputePassError) -> Self { Self { base: BasePass::new_invalid(label, err), parent: Some(parent), @@ -97,7 +99,7 @@ impl ComputePass { impl fmt::Debug for ComputePass { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.parent { - Some(ref cmd_buf) => write!(f, "ComputePass {{ parent: {} }}", cmd_buf.error_ident()), + Some(ref cmd_enc) => write!(f, "ComputePass {{ parent: {} }}", cmd_enc.error_ident()), None => write!(f, "ComputePass {{ parent: None }}"), } } @@ -251,10 +253,10 @@ impl WebGpuError for ComputePassError { } } -struct State<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder> { +struct State<'scope, 'snatch_guard, 'cmd_enc, 'raw_encoder> { pipeline: Option>, - general: pass::BaseState<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder>, + general: pass::BaseState<'scope, 'snatch_guard, 'cmd_enc, 'raw_encoder>, active_query: Option<(Arc, u32)>, @@ -263,8 +265,8 @@ struct State<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder> { intermediate_trackers: Tracker, } -impl<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder> - State<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder> +impl<'scope, 'snatch_guard, 'cmd_enc, 'raw_encoder> + State<'scope, 'snatch_guard, 'cmd_enc, 'raw_encoder> { fn is_ready(&self) -> Result<(), DispatchError> { if let Some(pipeline) = self.pipeline.as_ref() { @@ -308,7 +310,7 @@ impl<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder> ); } - CommandBuffer::drain_barriers( + CommandEncoder::drain_barriers( self.general.raw_encoder, &mut self.intermediate_trackers, self.general.snatch_guard, @@ -342,15 +344,15 @@ impl Global { let label = desc.label.as_deref().map(Cow::Borrowed); - let cmd_buf = hub.command_buffers.get(encoder_id.into_command_buffer_id()); - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = hub.command_encoders.get(encoder_id); + let mut cmd_buf_data = cmd_enc.data.lock(); match cmd_buf_data.lock_encoder() { Ok(()) => { drop(cmd_buf_data); - if let Err(err) = cmd_buf.device.check_is_valid() { + if let Err(err) = cmd_enc.device.check_is_valid() { return ( - ComputePass::new_invalid(cmd_buf, &label, err.map_pass_err(scope)), + ComputePass::new_invalid(cmd_enc, &label, err.map_pass_err(scope)), None, ); } @@ -360,7 +362,7 @@ impl Global { .as_ref() .map(|tw| { Self::validate_pass_timestamp_writes::( - &cmd_buf.device, + &cmd_enc.device, &hub.query_sets.read(), tw, ) @@ -372,10 +374,10 @@ impl Global { label, timestamp_writes, }; - (ComputePass::new(cmd_buf, arc_desc), None) + (ComputePass::new(cmd_enc, arc_desc), None) } Err(err) => ( - ComputePass::new_invalid(cmd_buf, &label, err.map_pass_err(scope)), + ComputePass::new_invalid(cmd_enc, &label, err.map_pass_err(scope)), None, ), } @@ -387,7 +389,7 @@ impl Global { cmd_buf_data.invalidate(err.clone()); drop(cmd_buf_data); ( - ComputePass::new_invalid(cmd_buf, &label, err.map_pass_err(scope)), + ComputePass::new_invalid(cmd_enc, &label, err.map_pass_err(scope)), None, ) } @@ -396,7 +398,7 @@ impl Global { // generates an immediate validation error. drop(cmd_buf_data); ( - ComputePass::new_invalid(cmd_buf, &label, err.clone().map_pass_err(scope)), + ComputePass::new_invalid(cmd_enc, &label, err.clone().map_pass_err(scope)), Some(err.into()), ) } @@ -408,7 +410,7 @@ impl Global { // invalid pass to save that work. drop(cmd_buf_data); ( - ComputePass::new_invalid(cmd_buf, &label, err.map_pass_err(scope)), + ComputePass::new_invalid(cmd_enc, &label, err.map_pass_err(scope)), None, ) } @@ -433,11 +435,8 @@ impl Global { ) { #[cfg(feature = "trace")] { - let cmd_buf = self - .hub - .command_buffers - .get(encoder_id.into_command_buffer_id()); - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = self.hub.command_encoders.get(encoder_id); + let mut cmd_buf_data = cmd_enc.data.lock(); let cmd_buf_data = cmd_buf_data.get_inner(); if let Some(ref mut list) = cmd_buf_data.commands { @@ -495,8 +494,8 @@ impl Global { pass.base.label.as_deref().unwrap_or("") ); - let cmd_buf = pass.parent.take().ok_or(EncoderStateError::Ended)?; - let mut cmd_buf_data = cmd_buf.data.lock(); + let cmd_enc = pass.parent.take().ok_or(EncoderStateError::Ended)?; + let mut cmd_buf_data = cmd_enc.data.lock(); if let Some(err) = pass.base.error.take() { if matches!( @@ -521,7 +520,7 @@ impl Global { } cmd_buf_data.unlock_and_record(|cmd_buf_data| -> Result<(), ComputePassError> { - let device = &cmd_buf.device; + let device = &cmd_enc.device; device.check_is_valid().map_pass_err(pass_scope)?; let base = &mut pass.base; @@ -582,7 +581,7 @@ impl Global { let timestamp_writes: Option> = if let Some(tw) = pass.timestamp_writes.take() { tw.query_set - .same_device_as(cmd_buf.as_ref()) + .same_device_as(cmd_enc.as_ref()) .map_pass_err(pass_scope)?; let query_set = state.general.tracker.query_sets.insert_single(tw.query_set); @@ -637,7 +636,7 @@ impl Global { let scope = PassErrorScope::SetBindGroup; pass::set_bind_group::( &mut state.general, - cmd_buf.as_ref(), + cmd_enc.as_ref(), &base.dynamic_offsets, index, num_dynamic_offsets, @@ -648,7 +647,7 @@ impl Global { } ArcComputeCommand::SetPipeline(pipeline) => { let scope = PassErrorScope::SetPipelineCompute; - set_pipeline(&mut state, cmd_buf.as_ref(), pipeline).map_pass_err(scope)?; + set_pipeline(&mut state, cmd_enc.as_ref(), pipeline).map_pass_err(scope)?; } ArcComputeCommand::SetPushConstant { offset, @@ -680,7 +679,7 @@ impl Global { } ArcComputeCommand::DispatchIndirect { buffer, offset } => { let scope = PassErrorScope::Dispatch { indirect: true }; - dispatch_indirect(&mut state, cmd_buf.as_ref(), buffer, offset) + dispatch_indirect(&mut state, cmd_enc.as_ref(), buffer, offset) .map_pass_err(scope)?; } ArcComputeCommand::PushDebugGroup { color: _, len } => { @@ -701,7 +700,7 @@ impl Global { let scope = PassErrorScope::WriteTimestamp; pass::write_timestamp::( &mut state.general, - cmd_buf.as_ref(), + cmd_enc.as_ref(), None, query_set, query_index, @@ -717,7 +716,7 @@ impl Global { query_set, state.general.raw_encoder, &mut state.general.tracker.query_sets, - cmd_buf.as_ref(), + cmd_enc.as_ref(), query_index, None, &mut state.active_query, @@ -750,10 +749,10 @@ impl Global { .. } = state; - // Stop the current command buffer. + // Stop the current command encoder. encoder.close().map_pass_err(pass_scope)?; - // Create a new command buffer, which we will insert _before_ the body of the compute pass. + // Create a new command encoder, which we will insert _before_ the body of the compute pass. // // Use that buffer to insert barriers and clear discarded images. let transit = encoder @@ -766,13 +765,13 @@ impl Global { device, &snatch_guard, ); - CommandBuffer::insert_barriers_from_tracker( + CommandEncoder::insert_barriers_from_tracker( transit, tracker, &intermediate_trackers, &snatch_guard, ); - // Close the command buffer, and swap it with the previous. + // Close the command encoder, and swap it with the previous. encoder.close_and_swap().map_pass_err(pass_scope)?; Ok(()) @@ -782,10 +781,10 @@ impl Global { fn set_pipeline( state: &mut State, - cmd_buf: &CommandBuffer, + cmd_enc: &CommandEncoder, pipeline: Arc, ) -> Result<(), ComputePassErrorInner> { - pipeline.same_device_as(cmd_buf)?; + pipeline.same_device_as(cmd_enc)?; state.pipeline = Some(pipeline.clone()); @@ -859,11 +858,11 @@ fn dispatch(state: &mut State, groups: [u32; 3]) -> Result<(), ComputePassErrorI fn dispatch_indirect( state: &mut State, - cmd_buf: &CommandBuffer, + cmd_enc: &CommandEncoder, buffer: Arc, offset: u64, ) -> Result<(), ComputePassErrorInner> { - buffer.same_device_as(cmd_buf)?; + buffer.same_device_as(cmd_enc)?; state.is_ready()?; diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index 8c23e4aef2f..bbab51b16cb 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -86,6 +86,8 @@ pub(crate) enum CommandEncoderStatus { /// Locked(CommandBufferMutable), + Consumed, + /// Command recording is complete, and the buffer is ready for submission. /// /// [`Global::command_encoder_finish`] transitions a @@ -145,6 +147,7 @@ impl CommandEncoderStatus { // Encoder is ended. Invalidate the encoder, do not record anything, // and return an immediate validation error. Self::Finished(_) => Err(self.invalidate(EncoderStateError::Ended)), + Self::Consumed => Err(EncoderStateError::Ended), // Encoder is already invalid. Do not record anything, but do not // return an immediate validation error. Self::Error(_) => Ok(()), @@ -173,6 +176,7 @@ impl CommandEncoderStatus { self.invalidate(EncoderStateError::Ended); f(None) } + Self::Consumed => f(None), Self::Error(_) => f(None), Self::Transitioning => unreachable!(), } @@ -186,6 +190,7 @@ impl CommandEncoderStatus { // playing back a recorded trace. If only to avoid having to // implement serialization for all the error types, we don't support // storing the errors in a trace. + Self::Consumed => unreachable!("command encoder is consumed"), Self::Error(_) => unreachable!("passes in a trace do not store errors"), Self::Transitioning => unreachable!(), } @@ -210,6 +215,10 @@ impl CommandEncoderStatus { Err(EncoderStateError::Ended) } Self::Locked(_) => Err(self.invalidate(EncoderStateError::Locked)), + st @ Self::Consumed => { + *self = st; + Err(EncoderStateError::Ended) + } st @ Self::Error(_) => { *self = st; Err(EncoderStateError::Invalid) @@ -254,6 +263,10 @@ impl CommandEncoderStatus { *self = Self::Error(EncoderStateError::Unlocked.into()); Err(EncoderStateError::Unlocked) } + st @ Self::Consumed => { + *self = st; + Err(EncoderStateError::Ended) + } st @ Self::Error(_) => { // Encoder is invalid. Do not record anything, but do not // return an immediate validation error. @@ -264,21 +277,22 @@ impl CommandEncoderStatus { } } - fn finish(&mut self) -> Result<(), CommandEncoderError> { - match mem::replace(self, Self::Transitioning) { + fn finish(&mut self) -> Self { + // Replace our state with `Consumed`, and return either the inner + // state or an error, to be transferred to the command buffer. + match mem::replace(self, Self::Consumed) { Self::Recording(mut inner) => { - if let Err(e) = inner.encoder.close_if_open() { - Err(self.invalidate(e.into())) + if let Err(err) = inner.encoder.close_if_open() { + Self::Error(err.into()) } else { - *self = Self::Finished(inner); // Note: if we want to stop tracking the swapchain texture view, // this is the place to do it. - Ok(()) + Self::Finished(inner) } } - Self::Finished(_) => Err(self.invalidate(EncoderStateError::Ended.into())), - Self::Locked(_) => Err(self.invalidate(EncoderStateError::Locked.into())), - Self::Error(err) => Err(self.invalidate(err)), + Self::Consumed | Self::Finished(_) => Self::Error(EncoderStateError::Ended.into()), + Self::Locked(_) => Self::Error(EncoderStateError::Locked.into()), + st @ Self::Error(_) => st, Self::Transitioning => unreachable!(), } } @@ -374,6 +388,26 @@ impl<'a> ops::DerefMut for RecordingGuard<'a> { } } +pub(crate) struct CommandEncoder { + pub(crate) device: Arc, + + pub(crate) label: String, + + /// The mutable state of this command encoder. + pub(crate) data: Mutex, +} + +crate::impl_resource_type!(CommandEncoder); +crate::impl_labeled!(CommandEncoder); +crate::impl_parent_device!(CommandEncoder); +crate::impl_storage_item!(CommandEncoder); + +impl Drop for CommandEncoder { + fn drop(&mut self) { + resource_log!("Drop {}", self.error_ident()); + } +} + /// A raw [`CommandEncoder`][rce], and the raw [`CommandBuffer`][rcb]s built from it. /// /// Each wgpu-core [`CommandBuffer`] owns an instance of this type, which is @@ -385,16 +419,11 @@ impl<'a> ops::DerefMut for RecordingGuard<'a> { /// commands into the middle of a recorded stream. However, hal queue submission /// accepts a series of command buffers at once, so we can simply break the /// stream up into multiple buffers, and then reorder the buffers. See -/// [`CommandEncoder::close_and_swap`] for a specific example of this. -/// -/// Note that a [`CommandEncoderId`] actually refers to a [`CommandBuffer`]. -/// Methods that take a command encoder id actually look up the command buffer, -/// and then use its encoder. +/// [`InnerCommandEncoder::close_and_swap`] for a specific example of this. /// /// [rce]: hal::Api::CommandEncoder /// [rcb]: hal::Api::CommandBuffer -/// [`CommandEncoderId`]: crate::id::CommandEncoderId -pub(crate) struct CommandEncoder { +pub(crate) struct InnerCommandEncoder { /// The underlying `wgpu_hal` [`CommandEncoder`]. /// /// Successfully executed command buffers' encoders are saved in a @@ -427,10 +456,10 @@ pub(crate) struct CommandEncoder { /// [`wgpu_hal::CommandEncoder`]: hal::CommandEncoder pub(crate) is_open: bool, - pub(crate) hal_label: Option, + pub(crate) label: String, } -impl CommandEncoder { +impl InnerCommandEncoder { /// Finish the current command buffer and insert it just before /// the last element in [`self.list`][l]. /// @@ -453,7 +482,7 @@ impl CommandEncoder { /// /// - If the encoder is not open. /// - /// [l]: CommandEncoder::list + /// [l]: InnerCommandEncoder::list /// [`transition_buffers`]: hal::CommandEncoder::transition_buffers /// [`transition_textures`]: hal::CommandEncoder::transition_textures fn close_and_swap(&mut self) -> Result<(), DeviceError> { @@ -476,7 +505,7 @@ impl CommandEncoder { /// /// - If the encoder is not open. /// - /// [l]: CommandEncoder::list + /// [l]: InnerCommandEncoder::list pub(crate) fn close_and_push_front(&mut self) -> Result<(), DeviceError> { assert!(self.is_open); self.is_open = false; @@ -497,7 +526,7 @@ impl CommandEncoder { /// /// - If the encoder is not open. /// - /// [l]: CommandEncoder::list + /// [l]: InnerCommandEncoder::list pub(crate) fn close(&mut self) -> Result<(), DeviceError> { assert!(self.is_open); self.is_open = false; @@ -518,7 +547,7 @@ impl CommandEncoder { /// /// On return, the underlying hal encoder is closed. /// - /// [l]: CommandEncoder::list + /// [l]: InnerCommandEncoder::list fn close_if_open(&mut self) -> Result<(), DeviceError> { if self.is_open { self.is_open = false; @@ -536,7 +565,7 @@ impl CommandEncoder { pub(crate) fn open(&mut self) -> Result<&mut dyn hal::DynCommandEncoder, DeviceError> { if !self.is_open { self.is_open = true; - let hal_label = self.hal_label.as_deref(); + let hal_label = hal_label(Some(&self.label), self.device.instance_flags); unsafe { self.raw.begin_encoding(hal_label) } .map_err(|e| self.device.handle_hal_error(e))?; } @@ -567,7 +596,7 @@ impl CommandEncoder { } } -impl Drop for CommandEncoder { +impl Drop for InnerCommandEncoder { fn drop(&mut self) { if self.is_open { unsafe { self.raw.discard_encoding() }; @@ -584,7 +613,7 @@ impl Drop for CommandEncoder { /// Look at the documentation for [`CommandBufferMutable`] for an explanation of /// the fields in this struct. This is the "built" counterpart to that type. pub(crate) struct BakedCommands { - pub(crate) encoder: CommandEncoder, + pub(crate) encoder: InnerCommandEncoder, pub(crate) trackers: Tracker, pub(crate) temp_resources: Vec, pub(crate) indirect_draw_validation_resources: crate::indirect_validation::DrawResources, @@ -598,7 +627,7 @@ pub struct CommandBufferMutable { /// they belong to. /// /// [`wgpu_hal::Api::CommandBuffer`]: hal::Api::CommandBuffer - pub(crate) encoder: CommandEncoder, + pub(crate) encoder: InnerCommandEncoder, /// All the resources that the commands recorded so far have referred to. pub(crate) trackers: Tracker, @@ -647,25 +676,11 @@ impl CommandBufferMutable { /// A buffer of commands to be submitted to the GPU for execution. /// -/// Whereas the WebGPU API uses two separate types for command buffers and -/// encoders, this type is a fusion of the two: -/// -/// - During command recording, this holds a [`CommandEncoder`] accepting this -/// buffer's commands. In this state, the [`CommandBuffer`] type behaves like -/// a WebGPU `GPUCommandEncoder`. -/// -/// - Once command recording is finished by calling -/// [`Global::command_encoder_finish`], no further recording is allowed. The -/// internal [`CommandEncoder`] is retained solely as a storage pool for the -/// raw command buffers. In this state, the value behaves like a WebGPU -/// `GPUCommandBuffer`. -/// -/// - Once a command buffer is submitted to the queue, it is removed from the id -/// registry, and its contents are taken to construct a [`BakedCommands`], -/// whose contents eventually become the property of the submission queue. +/// Once a command buffer is submitted to the queue, its contents are taken +/// to construct a [`BakedCommands`], whose contents eventually become the +/// property of the submission queue. pub struct CommandBuffer { pub(crate) device: Arc, - support_clear_texture: bool, /// The `label` from the descriptor used to create the resource. label: String, @@ -679,25 +694,24 @@ impl Drop for CommandBuffer { } } -impl CommandBuffer { +impl CommandEncoder { pub(crate) fn new( encoder: Box, device: &Arc, label: &Label, ) -> Self { - CommandBuffer { + CommandEncoder { device: device.clone(), - support_clear_texture: device.features.contains(wgt::Features::CLEAR_TEXTURE), label: label.to_string(), data: Mutex::new( rank::COMMAND_BUFFER_DATA, CommandEncoderStatus::Recording(CommandBufferMutable { - encoder: CommandEncoder { + encoder: InnerCommandEncoder { raw: ManuallyDrop::new(encoder), list: Vec::new(), device: device.clone(), is_open: false, - hal_label: label.to_hal(device.instance_flags).map(str::to_owned), + label: label.to_string(), }, trackers: Tracker::new(), buffer_memory_init_actions: Default::default(), @@ -723,9 +737,8 @@ impl CommandBuffer { label: &Label, err: CommandEncoderError, ) -> Self { - CommandBuffer { + CommandEncoder { device: device.clone(), - support_clear_texture: device.features.contains(wgt::Features::CLEAR_TEXTURE), label: label.to_string(), data: Mutex::new(rank::COMMAND_BUFFER_DATA, CommandEncoderStatus::Error(err)), } @@ -817,10 +830,7 @@ impl CommandBuffer { ) { St::Finished(command_buffer_mutable) => Ok(command_buffer_mutable), St::Error(err) => Err(err), - St::Recording(_) | St::Locked(_) => { - Err(InvalidResourceError(self.error_ident()).into()) - } - St::Transitioning => unreachable!(), + St::Recording(_) | St::Locked(_) | St::Consumed | St::Transitioning => unreachable!(), } } } @@ -1123,22 +1133,33 @@ impl Global { pub fn command_encoder_finish( &self, encoder_id: id::CommandEncoderId, - _desc: &wgt::CommandBufferDescriptor