hal/vulkan: use different indexes for acquire and present

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
+        // NOTE: It's important that we define the same number of acquire/present semaphores
-        // semaphores, since we prospectively need to provide the call to
+        // as we will need to index into them with the image index.
-        // acquire the next image with an unsignaled semaphore.
+        let acquire_semaphores = (0..=images.len())
-        let surface_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::<Result<Vec<_>, _>>()?;
 
+        let present_semaphores = (0..=images.len())
+            .map(|i| Arc::new(Mutex::new(super::SwapchainPresentSemaphores::new(i))))
+            .collect::<Vec<_>>();
+
         Ok(super::Swapchain {
             raw,
             functor,
             device: Arc::clone(&self.shared),
             images,
             config: config.clone(),
-            surface_semaphores,
+            acquire_semaphores,
-            next_semaphore_index: 0,
+            next_acquire_index: 0,
+            present_semaphores,
             next_present_time: None,
         })
     }

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(acquire_semaphore_guard);
-
-        drop(locked_swapchain_semaphores);
         // 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,

wgpu-hal/src/vulkan/mod.rs

@@ -178,9 +178,9 @@ pub struct Instance {
     shared: Arc<InstanceShared>,
 }
 
-/// 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<Self, crate::DeviceError> {
+        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<vk::Semaphore> {
+        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<vk::Semaphore>,
 
     /// 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 {
+impl SwapchainPresentSemaphores {
-    fn new(device: &DeviceShared, frame_index: usize) -> Result<Self, crate::DeviceError> {
+    pub fn new(frame_index: usize) -> Self {
-        Ok(Self {
+        Self {
-            acquire: device.new_binary_semaphore(&format!(
-                "SwapchainImageSemaphore: Image {frame_index} acquire"
-            ))?,
-            should_wait_for_acquire: true,
             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<vk::Semaphore> {
-        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] {
+    fn get_present_wait_semaphores(&mut self) -> Vec<vk::Semaphore> {
-        let old_index = self.present_index;
+        self.present[0..self.present_index].to_vec()
-
-        // 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]
     }
 
     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<DeviceShared>,
     images: Vec<vk::Image>,
     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<Mutex<>>` because we need to be able to pass this
+    /// Because we need to provide this to [`vkAcquireNextImageKHR`], we haven't
-    /// data into the surface texture, so submit/present can use it.
+    /// received the swapchain image index for the frame yet, so we cannot use
-    surface_semaphores: Vec<Arc<Mutex<SwapchainImageSemaphores>>>,
+    /// that to index it.
-    /// 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
+    /// Before we pass this to [`vkAcquireNextImageKHR`], we ensure that we wait on
-    /// to the acquire_next_image function which is what tells us which image to use.
+    /// the submission indicated by [`previously_used_submission_index`]. This enusres
-    next_semaphore_index: usize,
+    /// 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<Arc<Mutex<SwapchainAcquireSemaphore>>>,
+    /// 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<Arc<Mutex<SwapchainPresentSemaphores>>>,
+
     /// 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) {
+    /// Mark the current frame finished, advancing to the next acquire semaphore.
-        let semaphore_count = self.surface_semaphores.len();
+    fn advance_acquire_semaphore(&mut self) {
-        self.next_semaphore_index = (self.next_semaphore_index + 1) % semaphore_count;
+        let semaphore_count = self.acquire_semaphores.len();
+        self.next_acquire_index = (self.next_acquire_index + 1) % semaphore_count;
     }
 
-    fn get_surface_semaphores(&self) -> Arc<Mutex<SwapchainImageSemaphores>> {
+    /// Get the next acquire semaphore that should be used with this swapchain.
-        self.surface_semaphores[self.next_semaphore_index].clone()
+    fn get_acquire_semaphore(&self) -> Arc<Mutex<SwapchainAcquireSemaphore>> {
+        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<Mutex<SwapchainPresentSemaphores>> {
+        self.present_semaphores[index as usize].clone()
     }
 }
 
@@ -448,7 +512,8 @@ impl Surface {
 pub struct SurfaceTexture {
     index: u32,
     texture: Texture,
-    surface_semaphores: Arc<Mutex<SwapchainImageSemaphores>>,
+    acquire_semaphores: Arc<Mutex<SwapchainAcquireSemaphore>>,
+    present_semaphores: Arc<Mutex<SwapchainPresentSemaphores>>,
 }
 
 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::<Vec<_>>();
 
-        for mut swapchain_semaphore in locked_swapchain_semaphores {
+        for (mut acquire_semaphore, mut present_semaphores) in locked_swapchain_semaphores {
-            swapchain_semaphore.set_used_fence_value(signal_value);
+            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;