diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 8bfddd5d9..6b66bfa6a 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -573,25 +573,29 @@ impl super::Device { let images = unsafe { functor.get_swapchain_images(raw) }.map_err(super::map_host_device_oom_err)?; - // NOTE: It's important that we define at least images.len() wait - // semaphores, since we prospectively need to provide the call to - // acquire the next image with an unsignaled semaphore. - let surface_semaphores = (0..=images.len()) + // NOTE: It's important that we define the same number of acquire/present semaphores + // as we will need to index into them with the image index. + let acquire_semaphores = (0..=images.len()) .map(|i| { - super::SwapchainImageSemaphores::new(&self.shared, i) + super::SwapchainAcquireSemaphore::new(&self.shared, i) .map(Mutex::new) .map(Arc::new) }) .collect::, _>>()?; + let present_semaphores = (0..=images.len()) + .map(|i| Arc::new(Mutex::new(super::SwapchainPresentSemaphores::new(i)))) + .collect::>(); + Ok(super::Swapchain { raw, functor, device: Arc::clone(&self.shared), images, config: config.clone(), - surface_semaphores, - next_semaphore_index: 0, + acquire_semaphores, + next_acquire_index: 0, + present_semaphores, next_present_time: None, }) } diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 2e82e7d6b..572ffcd3f 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -188,9 +188,18 @@ impl super::Swapchain { }; // We cannot take this by value, as the function returns `self`. - for semaphore in self.surface_semaphores.drain(..) { + for semaphore in self.acquire_semaphores.drain(..) { let arc_removed = Arc::into_inner(semaphore).expect( - "Trying to destroy a SurfaceSemaphores that is still in use by a SurfaceTexture", + "Trying to destroy a SurfaceAcquireSemaphores that is still in use by a SurfaceTexture", + ); + let mutex_removed = arc_removed.into_inner(); + + unsafe { mutex_removed.destroy(device) }; + } + + for semaphore in self.present_semaphores.drain(..) { + let arc_removed = Arc::into_inner(semaphore).expect( + "Trying to destroy a SurfacePresentSemaphores that is still in use by a SurfaceTexture", ); let mutex_removed = arc_removed.into_inner(); @@ -1074,9 +1083,9 @@ impl crate::Surface for super::Surface { timeout_ns = u64::MAX; } - let swapchain_semaphores_arc = swapchain.get_surface_semaphores(); + let acquire_semaphore_arc = swapchain.get_acquire_semaphore(); // Nothing should be using this, so we don't block, but panic if we fail to lock. - let locked_swapchain_semaphores = swapchain_semaphores_arc + let acquire_semaphore_guard = acquire_semaphore_arc .try_lock() .expect("Failed to lock a SwapchainSemaphores."); @@ -1095,7 +1104,7 @@ impl crate::Surface for super::Surface { // `vkAcquireNextImageKHR` again. swapchain.device.wait_for_fence( fence, - locked_swapchain_semaphores.previously_used_submission_index, + acquire_semaphore_guard.previously_used_submission_index, timeout_ns, )?; @@ -1105,7 +1114,7 @@ impl crate::Surface for super::Surface { swapchain.functor.acquire_next_image( swapchain.raw, timeout_ns, - locked_swapchain_semaphores.acquire, + acquire_semaphore_guard.acquire, vk::Fence::null(), ) } { @@ -1129,12 +1138,12 @@ impl crate::Surface for super::Surface { } }; - log::error!("Got swapchain image {index}"); - - drop(locked_swapchain_semaphores); + drop(acquire_semaphore_guard); // We only advance the surface semaphores if we successfully acquired an image, otherwise // we should try to re-acquire using the same semaphores. - swapchain.advance_surface_semaphores(); + swapchain.advance_acquire_semaphore(); + + let present_semaphore_arc = swapchain.get_present_semaphores(index); // special case for Intel Vulkan returning bizarre values (ugh) if swapchain.device.vendor_id == crate::auxil::db::intel::VENDOR && index > 0x100 { @@ -1158,7 +1167,8 @@ impl crate::Surface for super::Surface { }, identity, }, - surface_semaphores: swapchain_semaphores_arc, + acquire_semaphores: acquire_semaphore_arc, + present_semaphores: present_semaphore_arc, }; Ok(Some(crate::AcquiredSurfaceTexture { texture, diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index 8f9796632..6608331f4 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -178,9 +178,9 @@ pub struct Instance { shared: Arc, } -/// The semaphores needed to use one image in a swapchain. +/// Semaphore used to acquire a swapchain image. #[derive(Debug)] -struct SwapchainImageSemaphores { +struct SwapchainAcquireSemaphore { /// A semaphore that is signaled when this image is safe for us to modify. /// /// When [`vkAcquireNextImageKHR`] returns the index of the next swapchain @@ -209,10 +209,70 @@ struct SwapchainImageSemaphores { /// wait. We set this flag when this image is acquired, and clear it the /// first time it's passed to [`Queue::submit`] as a surface texture. /// - /// [`acquire`]: SwapchainImageSemaphores::acquire + /// Additionally, semaphores can only be waited on once, so we need to ensure + /// that we only actually pass this semaphore to the first submission that + /// uses that image. + /// + /// [`acquire`]: SwapchainAcquireSemaphore::acquire /// [`Queue::submit`]: crate::Queue::submit should_wait_for_acquire: bool, + /// The fence value of the last command submission that wrote to this image. + /// + /// The next time we try to acquire this image, we'll block until + /// this submission finishes, proving that [`acquire`] is ready to + /// pass to `vkAcquireNextImageKHR` again. + /// + /// [`acquire`]: SwapchainAcquireSemaphore::acquire + previously_used_submission_index: crate::FenceValue, +} + +impl SwapchainAcquireSemaphore { + fn new(device: &DeviceShared, index: usize) -> Result { + Ok(Self { + acquire: device + .new_binary_semaphore(&format!("SwapchainImageSemaphore: Index {index} acquire"))?, + should_wait_for_acquire: true, + previously_used_submission_index: 0, + }) + } + + /// Sets the fence value which the next acquire will wait for. This prevents + /// the semaphore from being used while the previous submission is still in flight. + fn set_used_fence_value(&mut self, value: crate::FenceValue) { + self.previously_used_submission_index = value; + } + + /// Return the semaphore that commands drawing to this image should wait for, if any. + /// + /// This only returns `Some` once per acquisition; see + /// [`SwapchainAcquireSemaphore::should_wait_for_acquire`] for details. + fn get_acquire_wait_semaphore(&mut self) -> Option { + if self.should_wait_for_acquire { + self.should_wait_for_acquire = false; + Some(self.acquire) + } else { + None + } + } + + /// Indicates the cpu-side usage of this semaphore has finished for the frame, + /// so reset internal state to be ready for the next frame. + fn end_semaphore_usage(&mut self) { + // Reset the acquire semaphore, so that the next time we acquire this + // image, we can wait for it again. + self.should_wait_for_acquire = true; + } + + unsafe fn destroy(&self, device: &ash::Device) { + unsafe { + device.destroy_semaphore(self.acquire, None); + } + } +} + +#[derive(Debug)] +struct SwapchainPresentSemaphores { /// A pool of semaphores for ordering presentation after drawing. /// /// The first [`present_index`] semaphores in this vector are: @@ -244,64 +304,33 @@ struct SwapchainImageSemaphores { /// by the next present call. Any semaphores beyond that index were created /// for prior presents and are simply being retained for recycling. /// - /// [`present_index`]: SwapchainImageSemaphores::present_index + /// [`present_index`]: SwapchainPresentSemaphores::present_index /// [`vkQueuePresentKHR`]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#vkQueuePresentKHR /// [`vkQueueSubmit`]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#vkQueueSubmit present: Vec, /// The number of semaphores in [`present`] to be signalled for this submission. /// - /// [`present`]: SwapchainImageSemaphores::present + /// [`present`]: SwapchainPresentSemaphores::present present_index: usize, - /// The fence value of the last command submission that wrote to this image. - /// - /// The next time we try to acquire this image, we'll block until - /// this submission finishes, proving that [`acquire`] is ready to - /// pass to `vkAcquireNextImageKHR` again. - /// - /// [`acquire`]: SwapchainImageSemaphores::acquire - previously_used_submission_index: crate::FenceValue, - /// Which image this semaphore set is used for. frame_index: usize, } -impl SwapchainImageSemaphores { - fn new(device: &DeviceShared, frame_index: usize) -> Result { - Ok(Self { - acquire: device.new_binary_semaphore(&format!( - "SwapchainImageSemaphore: Image {frame_index} acquire" - ))?, - should_wait_for_acquire: true, +impl SwapchainPresentSemaphores { + pub fn new(frame_index: usize) -> Self { + Self { present: Vec::new(), present_index: 0, - previously_used_submission_index: 0, frame_index, - }) - } - - fn set_used_fence_value(&mut self, value: crate::FenceValue) { - self.previously_used_submission_index = value; - } - - /// Return the semaphore that commands drawing to this image should wait for, if any. - /// - /// This only returns `Some` once per acquisition; see - /// [`SwapchainImageSemaphores::should_wait_for_acquire`] for details. - fn get_acquire_wait_semaphore(&mut self) -> Option { - if self.should_wait_for_acquire { - self.should_wait_for_acquire = false; - Some(self.acquire) - } else { - None } } - /// Return a semaphore that a submission that writes to this image should + /// Return the semaphore that the next submission that writes to this image should /// signal when it's done. /// - /// See [`SwapchainImageSemaphores::present`] for details. + /// See [`SwapchainPresentSemaphores::present`] for details. fn get_submit_signal_semaphore( &mut self, device: &DeviceShared, @@ -324,29 +353,29 @@ impl SwapchainImageSemaphores { Ok(sem) } + /// Indicates the cpu-side usage of this semaphore has finished for the frame, + /// so reset internal state to be ready for the next frame. + fn end_semaphore_usage(&mut self) { + // Reset the index to 0, so that the next time we get a semaphore, we + // start from the beginning of the list. + self.present_index = 0; + } + /// Return the semaphores that a presentation of this image should wait on. /// /// Return a slice of semaphores that the call to [`vkQueueSubmit`] that /// ends this image's acquisition should wait for. See - /// [`SwapchainImageSemaphores::present`] for details. + /// [`SwapchainPresentSemaphores::present`] for details. /// /// Reset `self` to be ready for the next acquisition cycle. /// /// [`vkQueueSubmit`]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#vkQueueSubmit - fn get_present_wait_semaphores(&mut self) -> &[vk::Semaphore] { - let old_index = self.present_index; - - // Since this marks the end of this acquire/draw/present cycle, take the - // opportunity to reset `self` in preparation for the next acquisition. - self.present_index = 0; - self.should_wait_for_acquire = true; - - &self.present[0..old_index] + fn get_present_wait_semaphores(&mut self) -> Vec { + self.present[0..self.present_index].to_vec() } unsafe fn destroy(&self, device: &ash::Device) { unsafe { - device.destroy_semaphore(self.acquire, None); for sem in &self.present { device.destroy_semaphore(*sem, None); } @@ -360,16 +389,44 @@ struct Swapchain { device: Arc, images: Vec, config: crate::SurfaceConfiguration, - /// One wait semaphore per swapchain image. This will be associated with the - /// surface texture, and later collected during submission. + + /// Semaphores used between image acquisition and the first submission + /// that uses that image. This is indexed using [`next_acquire_index`]. /// - /// We need this to be `Arc>` because we need to be able to pass this - /// data into the surface texture, so submit/present can use it. - surface_semaphores: Vec>>, - /// The index of the next semaphore to use. Ideally we would use the same - /// index as the image index, but we need to specify the semaphore as an argument - /// to the acquire_next_image function which is what tells us which image to use. - next_semaphore_index: usize, + /// Because we need to provide this to [`vkAcquireNextImageKHR`], we haven't + /// received the swapchain image index for the frame yet, so we cannot use + /// that to index it. + /// + /// Before we pass this to [`vkAcquireNextImageKHR`], we ensure that we wait on + /// the submission indicated by [`previously_used_submission_index`]. This enusres + /// the semaphore is no longer in use before we use it. + /// + /// [`next_acquire_index`]: Swapchain::next_acquire_index + /// [`vkAcquireNextImageKHR`]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#vkAcquireNextImageKHR + /// [`previously_used_submission_index`]: SwapchainAcquireSemaphore::previously_used_submission_index + acquire_semaphores: Vec>>, + /// The index of the next acquire semaphore to use. + /// + /// This is incremented each time we acquire a new image, and wraps around + /// to 0 when it reaches the end of [`acquire_semaphores`]. + /// + /// [`acquire_semaphores`]: Swapchain::acquire_semaphores + next_acquire_index: usize, + + /// Semaphore sets used between all submissions that write to an image and + /// the presentation of that image. + /// + /// This is indexed by the swapchain image index returned by + /// [`vkAcquireNextImageKHR`]. + /// + /// We know it is safe to use these semaphores because use them + /// _after_ the acquire semaphore. Because the acquire semaphore + /// has been signaled, the previous presentation using that image + /// is known-finished, so this semaphore is no longer in use. + /// + /// [`vkAcquireNextImageKHR`]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#vkAcquireNextImageKHR + present_semaphores: Vec>>, + /// The present timing information which will be set in the next call to [`present()`](crate::Queue::present()). /// /// # Safety @@ -380,13 +437,20 @@ struct Swapchain { } impl Swapchain { - fn advance_surface_semaphores(&mut self) { - let semaphore_count = self.surface_semaphores.len(); - self.next_semaphore_index = (self.next_semaphore_index + 1) % semaphore_count; + /// Mark the current frame finished, advancing to the next acquire semaphore. + fn advance_acquire_semaphore(&mut self) { + let semaphore_count = self.acquire_semaphores.len(); + self.next_acquire_index = (self.next_acquire_index + 1) % semaphore_count; } - fn get_surface_semaphores(&self) -> Arc> { - self.surface_semaphores[self.next_semaphore_index].clone() + /// Get the next acquire semaphore that should be used with this swapchain. + fn get_acquire_semaphore(&self) -> Arc> { + self.acquire_semaphores[self.next_acquire_index].clone() + } + + /// Get the set of present semaphores that should be used with the given image index. + fn get_present_semaphores(&self, index: u32) -> Arc> { + self.present_semaphores[index as usize].clone() } } @@ -448,7 +512,8 @@ impl Surface { pub struct SurfaceTexture { index: u32, texture: Texture, - surface_semaphores: Arc>, + acquire_semaphores: Arc>, + present_semaphores: Arc>, } impl crate::DynSurfaceTexture for SurfaceTexture {} @@ -1384,9 +1449,10 @@ impl crate::Queue for Queue { let mut check = HashSet::with_capacity(surface_textures.len()); // We compare the Arcs by pointer, as Eq isn't well defined for SurfaceSemaphores. for st in surface_textures { - check.insert(Arc::as_ptr(&st.surface_semaphores)); + check.insert(Arc::as_ptr(&st.acquire_semaphores) as usize); + check.insert(Arc::as_ptr(&st.present_semaphores) as usize); } - check.len() == surface_textures.len() + check.len() == surface_textures.len() * 2 }, "More than one surface texture is being used from the same swapchain. This will cause a deadlock in release." ); @@ -1394,26 +1460,33 @@ impl crate::Queue for Queue { let locked_swapchain_semaphores = surface_textures .iter() .map(|st| { - st.surface_semaphores + let acquire = st + .acquire_semaphores .try_lock() - .expect("Failed to lock surface semaphore.") + .expect("Failed to lock surface acquire semaphore"); + let present = st + .present_semaphores + .try_lock() + .expect("Failed to lock surface present semaphore"); + + (acquire, present) }) .collect::>(); - for mut swapchain_semaphore in locked_swapchain_semaphores { - swapchain_semaphore.set_used_fence_value(signal_value); + for (mut acquire_semaphore, mut present_semaphores) in locked_swapchain_semaphores { + acquire_semaphore.set_used_fence_value(signal_value); // If we're the first submission to operate on this image, wait on // its acquire semaphore, to make sure the presentation engine is // done with it. - if let Some(sem) = swapchain_semaphore.get_acquire_wait_semaphore() { + if let Some(sem) = acquire_semaphore.get_acquire_wait_semaphore() { wait_stage_masks.push(vk::PipelineStageFlags::TOP_OF_PIPE); wait_semaphores.push(sem); } // Get a semaphore to signal when we're done writing to this surface // image. Presentation of this image will wait for this. - let signal_semaphore = swapchain_semaphore.get_submit_signal_semaphore(&self.device)?; + let signal_semaphore = present_semaphores.get_submit_signal_semaphore(&self.device)?; signal_semaphores.push_binary(signal_semaphore); } @@ -1488,14 +1561,28 @@ impl crate::Queue for Queue { ) -> Result<(), crate::SurfaceError> { let mut swapchain = surface.swapchain.write(); let ssc = swapchain.as_mut().unwrap(); - let mut swapchain_semaphores = texture.surface_semaphores.lock(); + let mut acquire_semaphore = texture.acquire_semaphores.lock(); + let mut present_semaphores = texture.present_semaphores.lock(); + + let wait_semaphores = present_semaphores.get_present_wait_semaphores(); + + // Reset the acquire and present semaphores internal state + // to be ready for the next frame. + // + // We do this before the actual call to present to ensure that + // even if this method errors and early outs, we have reset + // the state for next frame. + acquire_semaphore.end_semaphore_usage(); + present_semaphores.end_semaphore_usage(); + + drop(acquire_semaphore); let swapchains = [ssc.raw]; let image_indices = [texture.index]; let vk_info = vk::PresentInfoKHR::default() .swapchains(&swapchains) .image_indices(&image_indices) - .wait_semaphores(swapchain_semaphores.get_present_wait_semaphores()); + .wait_semaphores(&wait_semaphores); let mut display_timing; let present_times;