diff --git a/wgpu-core/src/command/memory_init.rs b/wgpu-core/src/command/memory_init.rs index 85e5e644c93..64b8d0065e4 100644 --- a/wgpu-core/src/command/memory_init.rs +++ b/wgpu-core/src/command/memory_init.rs @@ -268,28 +268,30 @@ impl BakedCommands { let mut ranges: Vec = Vec::new(); for texture_use in self.texture_memory_actions.drain_init_actions() { - let mut initialization_status = texture_use.texture.initialization_status.write(); - let use_range = texture_use.range; - let affected_mip_trackers = initialization_status - .mips - .iter_mut() - .enumerate() - .skip(use_range.mip_range.start as usize) - .take((use_range.mip_range.end - use_range.mip_range.start) as usize); - - match texture_use.kind { - MemoryInitKind::ImplicitlyInitialized => { - for (_, mip_tracker) in affected_mip_trackers { - mip_tracker.drain(use_range.layer_range.clone()); + { + let mut initialization_status = texture_use.texture.initialization_status.write(); + let use_range = texture_use.range; + let affected_mip_trackers = initialization_status + .mips + .iter_mut() + .enumerate() + .skip(use_range.mip_range.start as usize) + .take((use_range.mip_range.end - use_range.mip_range.start) as usize); + + match texture_use.kind { + MemoryInitKind::ImplicitlyInitialized => { + for (_, mip_tracker) in affected_mip_trackers { + mip_tracker.drain(use_range.layer_range.clone()); + } } - } - MemoryInitKind::NeedsInitializedMemory => { - for (mip_level, mip_tracker) in affected_mip_trackers { - for layer_range in mip_tracker.drain(use_range.layer_range.clone()) { - ranges.push(TextureInitRange { - mip_range: (mip_level as u32)..(mip_level as u32 + 1), - layer_range, - }); + MemoryInitKind::NeedsInitializedMemory => { + for (mip_level, mip_tracker) in affected_mip_trackers { + for layer_range in mip_tracker.drain(use_range.layer_range.clone()) { + ranges.push(TextureInitRange { + mip_range: (mip_level as u32)..(mip_level as u32 + 1), + layer_range, + }); + } } } } diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 88a99dab83e..82e075d335f 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -1903,6 +1903,9 @@ impl Global { /// /// Return `all_queue_empty` indicating whether there are more queue /// submissions still in flight. + /// + /// Upon entry, collects the set of devices to poll. Will not poll new + /// devices created while polling the initial set. fn poll_all_devices_of_api( &self, force_wait: bool, @@ -1913,9 +1916,14 @@ impl Global { let hub = &self.hub; let mut all_queue_empty = true; { - let device_guard = hub.devices.read(); + let devices = hub + .devices + .read() + .iter() + .map(|(_id, device)| device.clone()) + .collect::>(); - for (_id, device) in device_guard.iter() { + for device in devices { let poll_type = if force_wait { // TODO(#8286): Should expose timeout to poll_all. wgt::PollType::wait_indefinitely() diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index c39209a8e8d..8d83781b1c7 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -573,23 +573,76 @@ impl WebGpuError for QueueSubmitError { } } -/// A partially-assembled submission. +/// A command submission in the process of being assembled. +/// +/// Within `wgpu_core`, enqueuing commands for execution on the GPU is a +/// three-step process: +/// +/// 1) Call [`Queue::allocate_submission`] to acquire the necessary locks, +/// assign a submission index, wrap them all up as a [`PendingSubmission`], +/// and return it. +/// +/// 2) Add the command buffers to be submitted to [`executions`], and note any +/// surface textures they reference in [`surface_textures`]. Contribute to +/// [`Queue::pending_writes`] as necessary. +/// +/// 3) Call the `PendingSubmission`'s [`submit`] method, passing your mutex +/// guard on [`Queue::pending_writes`]. +/// +/// It is also acceptable to drop the `PendingSubmission` without submitting; +/// this frees its locks in the appropriate order. This may be necessary when +/// those locks are required to access the state that determines whether a +/// submission is needed at all. +/// +/// This split allows the various places in `wgpu_core` that need to submit +/// commands to the GPU to share the common initial code for locking and final +/// code for actually submitting the commands to `wgpu_hal`: +/// +/// - [`Queue::submit`] just submits user-constructed [`CommandBuffer`]s. +/// +/// - [`Queue::prepare_surface_texture_for_present`] examines the surface +/// texture being presented, and submit deferred initialization commands and +/// barriers to get it ready. +/// +/// - [`Queue::flush_writes_for_buffer`] and [`Queue::flush_pending_writes`] +/// simply submit the operations already staged in [`Queue::pending_writes`]. /// /// Returned from [`Queue::allocate_submission`] and consumed by [`submit`]. /// These are internal APIs used in `Queue::submit` and other places within /// `wgpu-core` that need to submit work. /// -/// [`submit`]: `PendingSubmission::submit` +/// [`submit`]: PendingSubmission::submit +/// [`executions`]: PendingSubmission::executions +/// [`surface_textures`]: PendingSubmission::surface_textures pub(crate) struct PendingSubmission<'a> { queue: &'a Queue, - snatch_guard: SnatchGuard<'a>, + + // These lock guards must appear in this struct in the order given. + // + // The instrumented locks in [`lock::ranked`] require that locks be acquired + // and released in a stack-like order. Since `rank::DEVICE_COMMAND_INDICES` + // follows `rank::DEVICE_SNATCHABLE_LOCK`, the lock on + // `Device::command_indices` must be released before the lock on + // `Device::snatchable_lock`. Rust drops struct members from first to last, + // so this ordering of fields ensures the order we want. + /// A guard for the lock on `Device::command_indices`. command_index_guard: RwLockWriteGuard<'a, CommandIndices>, - // Command buffers to be executed, along with trackers for the resources they use. + + /// A guard for the lock on `Device::snatchable_lock`. + snatch_guard: SnatchGuard<'a>, + + /// Command buffers to be submitted, along with trackers for the resources + /// they use. pub executions: Vec, - // Surface textures referenced by command buffers in this submission. These need to be - // passed to the HAL `submit` call. Deduplicated using a hashmap to avoid vulkan - // deadlocking from the same surface texture being submitted multiple times. + + /// Surface textures referenced by command buffers in this submission. + /// + /// These need to be passed to the [`wgpu_hal::Queue::submit`], which + /// requires that the list contains no duplicates, so we store them in a + /// `HashMap` keyed by `SurfaceTexture` address. surface_textures: FastHashMap<*const Texture, Arc>, + + /// The index this submission has been assigned. pub index: SubmissionIndex, } @@ -659,12 +712,8 @@ impl Queue { buffer_offset, ); - drop(snatch_guard); - pending_writes.consume(staging_buffer); - drop(pending_writes); - result } @@ -916,35 +965,44 @@ impl Queue { } else { destination.origin.z..destination.origin.z + size.depth_or_array_layers }; - let mut dst_initialization_status = dst.initialization_status.write(); - if dst_initialization_status.mips[destination.mip_level as usize] - .check(init_layer_range.clone()) - .is_some() - { - if has_copy_partial_init_tracker_coverage(size, &destination, &dst.desc) { - for layer_range in dst_initialization_status.mips[destination.mip_level as usize] - .drain(init_layer_range) - .collect::>>() - { - let mut trackers = self.device.trackers.lock(); - crate::command::clear_texture( - &dst, - TextureInitRange { - mip_range: destination.mip_level..(destination.mip_level + 1), - layer_range, - }, - encoder, - &mut trackers.textures, - &self.device.alignments, - self.device.zero_buffer.as_ref(), - &snatch_guard, - self.device.instance_flags, + let layer_ranges_to_clear = { + let mut dst_initialization_status = dst.initialization_status.write(); + if dst_initialization_status.mips[destination.mip_level as usize] + .check(init_layer_range.clone()) + .is_some() + { + if has_copy_partial_init_tracker_coverage(size, &destination, &dst.desc) { + Some( + dst_initialization_status.mips[destination.mip_level as usize] + .drain(init_layer_range) + .collect::>>(), ) - .map_err(QueueWriteError::from)?; + } else { + dst_initialization_status.mips[destination.mip_level as usize] + .drain(init_layer_range); + None } } else { - dst_initialization_status.mips[destination.mip_level as usize] - .drain(init_layer_range); + None + } + }; + if let Some(layer_ranges) = layer_ranges_to_clear { + let mut trackers = self.device.trackers.lock(); + for layer_range in layer_ranges { + crate::command::clear_texture( + &dst, + TextureInitRange { + mip_range: destination.mip_level..(destination.mip_level + 1), + layer_range, + }, + encoder, + &mut trackers.textures, + &self.device.alignments, + self.device.zero_buffer.as_ref(), + &snatch_guard, + self.device.instance_flags, + ) + .map_err(QueueWriteError::from)?; } } @@ -1157,6 +1215,10 @@ impl Queue { let (selector, dst_base) = extract_texture_selector(&destination, &size, &dst)?; + let snatch_guard = self.device.snatchable_lock.read(); + + let dst_raw = dst.try_raw(&snatch_guard)?; + // This must happen after parameter validation (so that errors are reported // as required by the spec), but before any side effects. if size.width == 0 || size.height == 0 || size.depth_or_array_layers == 0 { @@ -1178,41 +1240,47 @@ impl Queue { } else { destination.origin.z..destination.origin.z + size.depth_or_array_layers }; - let mut dst_initialization_status = dst.initialization_status.write(); - if dst_initialization_status.mips[destination.mip_level as usize] - .check(init_layer_range.clone()) - .is_some() - { - if has_copy_partial_init_tracker_coverage(&size, &destination, &dst.desc) { - for layer_range in dst_initialization_status.mips[destination.mip_level as usize] - .drain(init_layer_range) - .collect::>>() - { - let mut trackers = self.device.trackers.lock(); - crate::command::clear_texture( - &dst, - TextureInitRange { - mip_range: destination.mip_level..(destination.mip_level + 1), - layer_range, - }, - encoder, - &mut trackers.textures, - &self.device.alignments, - self.device.zero_buffer.as_ref(), - &self.device.snatchable_lock.read(), - self.device.instance_flags, + let layer_ranges_to_clear = { + let mut dst_initialization_status = dst.initialization_status.write(); + if dst_initialization_status.mips[destination.mip_level as usize] + .check(init_layer_range.clone()) + .is_some() + { + if has_copy_partial_init_tracker_coverage(&size, &destination, &dst.desc) { + Some( + dst_initialization_status.mips[destination.mip_level as usize] + .drain(init_layer_range) + .collect::>>(), ) - .map_err(QueueWriteError::from)?; + } else { + dst_initialization_status.mips[destination.mip_level as usize] + .drain(init_layer_range); + None } } else { - dst_initialization_status.mips[destination.mip_level as usize] - .drain(init_layer_range); + None + } + }; + if let Some(layer_ranges) = layer_ranges_to_clear { + let mut trackers = self.device.trackers.lock(); + for layer_range in layer_ranges { + crate::command::clear_texture( + &dst, + TextureInitRange { + mip_range: destination.mip_level..(destination.mip_level + 1), + layer_range, + }, + encoder, + &mut trackers.textures, + &self.device.alignments, + self.device.zero_buffer.as_ref(), + &snatch_guard, + self.device.instance_flags, + ) + .map_err(QueueWriteError::from)?; } } - let snatch_guard = self.device.snatchable_lock.read(); - let dst_raw = dst.try_raw(&snatch_guard)?; - let regions = hal::TextureCopy { src_base: hal::TextureCopyBase { mip_level: 0, @@ -1586,8 +1654,8 @@ impl Queue { let submission = PendingSubmission { queue: self, - snatch_guard, command_index_guard, + snatch_guard, executions: Vec::new(), surface_textures: FastHashMap::default(), index, diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index b5299b7247b..dfe2c19ff16 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -664,10 +664,23 @@ impl Device { self.adapter.backend() } + /// Return true if `self` is still valid. + /// + /// Note that a `false` result here does *not* guarantee that no further activity will + /// occur on the `Device`. A [`Device`] can be marked invalid at any time, even while + /// locks are held, meaning that operations begun before the `Device` was marked + /// invalid will still generally run to completion. This is simply a consequence of + /// making [`Device::valid`] an `AtomicBool`, rather than an ordinary `bool` field + /// protected by the same locks that all other operations acquire. pub fn is_valid(&self) -> bool { self.valid.load(Ordering::Acquire) } + /// Return `Err(DeviceError)` if `self` is not valid. Otherwise, return `Ok(())`. + /// + /// Note that an `Err` result here does *not* guarantee that no further activity will + /// occur on the `Device`. See the documentation for [`is_valid`][Self::is_valid] for + /// details. pub fn check_is_valid(&self) -> Result<(), DeviceError> { if self.is_valid() { Ok(()) @@ -897,14 +910,27 @@ impl Device { } }; - // Prevent new commands from being submitted as we want to act on `queue_empty`. - let command_indices = self.command_indices.read(); - // Check that the device is valid. This is combined with queue empty to decide whether - // to destroy all resources. Queue.submit blocks on command indices being writable - // and rejects if invalid so if the device in now invalid, and all submissions are - // finished, there will be no more submissions. - let device_valid = self.is_valid(); - drop(command_indices); + // When a device is marked invalid, we must destroy all its hal resources once its + // queue is empty, by calling `release_gpu_resources`. + // + // However, checking device validity for this purpose is tricky. A device can be + // marked invalid at any time, regardless of what locks are held. This means that, + // even though queue submission does check device validity at the start (in + // `Queue::allocate_submission`), the submission will proceed even if the the + // device gets marked invalid after that check. Thus, other threads can observe + // the queue becoming non-empty even after they have observed the device to be + // invalid. + // + // In this function, for `release_gpu_resources` to work as intended, we must be + // sure that no further work can be submitted. Specifically, we must be sure that + // if we see the device marked invalid, then so will any subsequent attempts to + // submit work to the queue, causing them to fail. To accomplish this, it suffices + // to hold the same lock while we call `is_valid` that `Queue::allocate_submission` + // holds while it does the submission. + let device_valid = { + let _command_indices_guard = self.command_indices.read(); + self.is_valid() + }; // Maintain all finished submissions on the queue, updating the relevant user closures and // collecting if the queue is empty. @@ -5377,17 +5403,24 @@ impl Device { // initiate movement into those buckets, and it can do that by calling // "destroy" on all the resources we know about. - // During these iterations, we discard all errors. We don't care! let trackers = self.trackers.lock(); - for buffer in trackers.buffers.used_resources() { - if let Some(buffer) = Weak::upgrade(buffer) { - buffer.destroy(); - } + let buffers = trackers + .buffers + .used_resources() + .flat_map(Weak::upgrade) + .collect::>(); + let textures = trackers + .textures + .used_resources() + .flat_map(Weak::upgrade) + .collect::>(); + drop(trackers); + + for buffer in buffers { + buffer.destroy(); } - for texture in trackers.textures.used_resources() { - if let Some(texture) = Weak::upgrade(texture) { - texture.destroy(); - } + for texture in textures { + texture.destroy(); } } diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index 7d18b6bd48d..da904fb71ca 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -1314,8 +1314,12 @@ impl crate::Queue for Queue { let mut wait_semaphores = SemaphoreList::new(SemaphoreListMode::Wait); let mut signal_semaphores = SemaphoreList::new(SemaphoreListMode::Signal); - // Double check that the same swapchain image isn't being given to us multiple times, - // as that will deadlock when we try to lock them all. + // Assert that we will not deadlock as we try to lock each `SurfaceTexture`'s + // metadata. + // + // The documentation for [`wgpu_hal::Queue::submit`] requires that + // `surface_textures` must not contain duplicates, so simply iterating over + // the slice and locking each one should be fine. debug_assert!( { let mut check = HashSet::with_capacity(surface_textures.len());