Refactor some swapchain operations (#2361)

* Refactor some swapchain operations

* Better locking for is_retired

* Oops

* Update vulkano/src/swapchain/acquire_present.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Update vulkano/src/swapchain/acquire_present.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

---------

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

vulkano/src/device/mod.rs

@@ -532,6 +532,13 @@ impl Device {
         InstanceOwnedDebugWrapper::cast_slice_inner(&self.physical_devices)
     }
 
+    /// Returns a device mask containing all physical devices in this device. In other words:
+    /// every bit that corresponds to a physical device in this device is set to 1.
+    #[inline]
+    pub fn device_mask(&self) -> u32 {
+        (1 << self.physical_devices.len() as u32) - 1
+    }
+
     /// Returns the instance used to create this device.
     #[inline]
     pub fn instance(&self) -> &Arc<Instance> {

vulkano/src/swapchain/acquire_present.rs

@@ -17,8 +17,7 @@ use crate::{
         future::{AccessCheckError, AccessError, GpuFuture, SubmitAnyBuilder},
         semaphore::Semaphore,
     },
-    DeviceSize, Requires, RequiresAllOf, RequiresOneOf, Validated, ValidationError, VulkanError,
+    DeviceSize, Validated, ValidationError, VulkanError, VulkanObject,
-    VulkanObject,
 };
 use smallvec::smallvec;
 use std::{
@@ -34,6 +33,87 @@ use std::{
     time::Duration,
 };
 
+/// Parameters to acquire the next image from a swapchain.
+#[derive(Clone, Debug)]
+pub struct AcquireNextImageInfo {
+    /// If no image is immediately available, how long to wait for one to become available.
+    ///
+    /// Specify `None` to wait forever. This is only allowed if at least one image in the swapchain
+    /// has not been acquired yet.
+    ///
+    /// The default value is `None`.
+    pub timeout: Option<Duration>,
+
+    /// The semaphore to signal when an image has become available.
+    ///
+    /// `semaphore` and `fence` must not both be `None`.
+    ///
+    /// The default value is `None`.
+    pub semaphore: Option<Arc<Semaphore>>,
+
+    /// The fence to signal when an image has become available.
+    ///
+    /// `semaphore` and `fence` must not both be `None`.
+    ///
+    /// The default value is `None`.
+    pub fence: Option<Arc<Fence>>,
+
+    pub _ne: crate::NonExhaustive,
+}
+
+impl Default for AcquireNextImageInfo {
+    #[inline]
+    fn default() -> Self {
+        Self {
+            timeout: None,
+            semaphore: None,
+            fence: None,
+            _ne: crate::NonExhaustive(()),
+        }
+    }
+}
+
+impl AcquireNextImageInfo {
+    pub(crate) fn validate(&self, device: &Device) -> Result<(), Box<ValidationError>> {
+        let &Self {
+            timeout,
+            ref semaphore,
+            ref fence,
+            _ne: _,
+        } = self;
+
+        if let Some(timeout) = timeout {
+            if timeout.as_nanos() >= u64::MAX as u128 {
+                return Err(Box::new(ValidationError {
+                    context: "timeout".into(),
+                    problem: "is not less than `u64::MAX` nanoseconds".into(),
+                    ..Default::default()
+                }));
+            }
+        }
+
+        if semaphore.is_none() && fence.is_none() {
+            return Err(Box::new(ValidationError {
+                problem: "`semaphore` and `fence` are both `None`".into(),
+                vuids: &["VUID-VkAcquireNextImageInfoKHR-semaphore-01782"],
+                ..Default::default()
+            }));
+        }
+
+        if let Some(semaphore) = semaphore {
+            // VUID-VkAcquireNextImageInfoKHR-commonparent
+            assert_eq!(device, semaphore.device().as_ref());
+        }
+
+        if let Some(fence) = fence {
+            // VUID-VkAcquireNextImageInfoKHR-commonparent
+            assert_eq!(device, fence.device().as_ref());
+        }
+
+        Ok(())
+    }
+}
+
 /// Tries to take ownership of an image in order to draw on it.
 ///
 /// The function returns the index of the image in the array of images that was returned
@@ -51,38 +131,33 @@ pub fn acquire_next_image(
     timeout: Option<Duration>,
 ) -> Result<(u32, bool, SwapchainAcquireFuture), Validated<VulkanError>> {
     let semaphore = Arc::new(Semaphore::from_pool(swapchain.device.clone())?);
-    let fence = Fence::from_pool(swapchain.device.clone())?;
+    let fence = Arc::new(Fence::from_pool(swapchain.device.clone())?);
 
     let AcquiredImage {
         image_index,
-        suboptimal,
+        is_suboptimal,
-    } = {
+    } = unsafe {
-        // Check that this is not an old swapchain. From specs:
+        swapchain.acquire_next_image(&AcquireNextImageInfo {
-        // > swapchain must not have been replaced by being passed as the
+            timeout,
-        // > VkSwapchainCreateInfoKHR::oldSwapchain value to vkCreateSwapchainKHR
+            semaphore: Some(semaphore.clone()),
-        let retired = swapchain.is_retired.lock();
+            fence: Some(fence.clone()),
-        if *retired {
+            ..Default::default()
-            return Err(VulkanError::OutOfDate.into());
+        })?
-        }
-
-        let acquire_result =
-            unsafe { acquire_next_image_raw(&swapchain, timeout, Some(&semaphore), Some(&fence)) };
-
-        if matches!(
-            acquire_result,
-            Err(Validated::Error(VulkanError::FullScreenExclusiveModeLost))
-        ) {
-            swapchain
-                .full_screen_exclusive_held
-                .store(false, Ordering::SeqCst);
-        }
-
-        acquire_result?
     };
 
+    unsafe {
+        let mut state = semaphore.state();
+        state.swapchain_acquire();
+    }
+
+    unsafe {
+        let mut state = fence.state();
+        state.import_swapchain_acquire();
+    }
+
     Ok((
         image_index,
-        suboptimal,
+        is_suboptimal,
         SwapchainAcquireFuture {
             swapchain,
             semaphore: Some(semaphore),
@@ -100,6 +175,10 @@ pub fn acquire_next_image(
 /// - The semaphore and/or the fence must be kept alive until it is signaled.
 /// - The swapchain must not have been replaced by being passed as the old swapchain when creating
 ///   a new one.
+#[deprecated(
+    since = "0.35.0",
+    note = "use `Swapchain::acquire_next_image_unchecked` instead"
+)]
 pub unsafe fn acquire_next_image_raw(
     swapchain: &Swapchain,
     timeout: Option<Duration>,
@@ -129,7 +208,7 @@ pub unsafe fn acquire_next_image_raw(
         out.as_mut_ptr(),
     );
 
-    let suboptimal = match result {
+    let is_suboptimal = match result {
         ash::vk::Result::SUCCESS => false,
         ash::vk::Result::SUBOPTIMAL_KHR => true,
         ash::vk::Result::NOT_READY => return Err(VulkanError::NotReady.into()),
@@ -149,13 +228,21 @@ pub unsafe fn acquire_next_image_raw(
 
     Ok(AcquiredImage {
         image_index: out.assume_init(),
-        suboptimal,
+        is_suboptimal,
     })
 }
 
+/// An image that will be acquired from a swapchain.
+#[derive(Clone, Copy, Debug)]
 pub struct AcquiredImage {
+    /// The index of the swapchain image that will be acquired.
     pub image_index: u32,
-    pub suboptimal: bool,
+
+    /// Whether the swapchain is no longer optimally configured to present to its surface.
+    ///
+    /// If this is `true`, then the acquired image will still be usable, but the swapchain should
+    /// be recreated soon to ensure an optimal configuration.
+    pub is_suboptimal: bool,
 }
 
 /// Represents the moment when the GPU will have access to a swapchain image.
@@ -168,7 +255,7 @@ pub struct SwapchainAcquireFuture {
     semaphore: Option<Arc<Semaphore>>,
     // Fence that is signalled when the acquire is complete. Empty if the acquire has already
     // happened.
-    fence: Option<Fence>,
+    fence: Option<Arc<Fence>>,
     finished: AtomicBool,
 }
 
@@ -806,61 +893,11 @@ where
 /// Returns a bool to represent if the presentation was suboptimal. In this case the swapchain is
 /// still usable, but the swapchain should be recreated as the Surface's properties no longer match
 /// the swapchain.
+#[deprecated(since = "0.35.0", note = "use `Swapchain::wait_for_present` instead")]
 pub fn wait_for_present(
     swapchain: Arc<Swapchain>,
     present_id: u64,
     timeout: Option<Duration>,
 ) -> Result<bool, Validated<VulkanError>> {
-    if !swapchain.device.enabled_features().present_wait {
+    swapchain.wait_for_present(present_id.try_into().unwrap(), timeout)
-        return Err(Box::new(ValidationError {
-            requires_one_of: RequiresOneOf(&[RequiresAllOf(&[Requires::Feature("present_wait")])]),
-            vuids: &["VUID-vkWaitForPresentKHR-presentWait-06234"],
-            ..Default::default()
-        })
-        .into());
-    }
-
-    if present_id == 0 {
-        return Err(Box::new(ValidationError {
-            context: "present_id".into(),
-            problem: "is 0".into(),
-            ..Default::default()
-        })
-        .into());
-    }
-
-    let retired = swapchain.is_retired.lock();
-
-    // VUID-vkWaitForPresentKHR-swapchain-04997
-    if *retired {
-        return Err(VulkanError::OutOfDate.into());
-    }
-
-    let timeout_ns = timeout.map(|dur| dur.as_nanos() as u64).unwrap_or(0);
-
-    let result = unsafe {
-        (swapchain.device.fns().khr_present_wait.wait_for_present_khr)(
-            swapchain.device.handle(),
-            swapchain.handle,
-            present_id,
-            timeout_ns,
-        )
-    };
-
-    match result {
-        ash::vk::Result::SUCCESS => Ok(false),
-        ash::vk::Result::SUBOPTIMAL_KHR => Ok(true),
-        ash::vk::Result::TIMEOUT => Err(VulkanError::Timeout.into()),
-        err => {
-            let err = VulkanError::from(err);
-
-            if matches!(err, VulkanError::FullScreenExclusiveModeLost) {
-                swapchain
-                    .full_screen_exclusive_held
-                    .store(false, Ordering::SeqCst);
-            }
-
-            Err(err.into())
-        }
-    }
 }

vulkano/src/swapchain/mod.rs

@@ -346,6 +346,7 @@ use std::{
         atomic::{AtomicBool, AtomicU64, Ordering},
         Arc,
     },
+    time::Duration,
 };
 
 /// Contains the swapping system and the images that can be shown on a surface.
@@ -1359,17 +1360,6 @@ impl Swapchain {
         &self.image_sharing
     }
 
-    #[inline]
-    pub(crate) unsafe fn full_screen_exclusive_held(&self) -> &AtomicBool {
-        &self.full_screen_exclusive_held
-    }
-
-    #[inline]
-    pub(crate) unsafe fn try_claim_present_id(&self, present_id: NonZeroU64) -> bool {
-        let present_id = u64::from(present_id);
-        self.prev_present_id.fetch_max(present_id, Ordering::SeqCst) < present_id
-    }
-
     /// Returns the pre-transform that was passed when creating the swapchain.
     #[inline]
     pub fn pre_transform(&self) -> SurfaceTransform {
@@ -1418,6 +1408,234 @@ impl Swapchain {
         self.full_screen_exclusive
     }
 
+    /// Acquires temporary ownership of a swapchain image.
+    ///
+    /// The function returns the index of the image in the array of images that was returned
+    /// when creating the swapchain. The image will not be available immediately after the function
+    /// returns successfully.
+    ///
+    /// When the image becomes available, a semaphore or fence will be signaled. The image can then
+    /// be accessed by the host or device. After this, the image must be *presented* back to the
+    /// swapchain, using the [`present`] queue command.
+    ///
+    /// # Safety
+    ///
+    /// - `self` must be kept alive until either `acquire_info.semaphore` or `acquire_info.fence`
+    ///   is signaled.
+    /// - If all images from `self` are currently acquired, and have not been presented yet, then
+    ///   `acquire_info.timeout` must not be `None`.
+    ///
+    /// If `acquire_info.semaphore` is `Some`:
+    /// - The semaphore must be kept alive until it is signaled.
+    /// - When the signal operation is executed, the semaphore must be in the unsignaled state.
+    ///
+    /// If `acquire_info.fence` is `Some`:
+    /// - The fence must be kept alive until it is signaled.
+    /// - The fence must be unsignaled and must not be associated with any other command that is
+    ///   still executing.
+    ///
+    /// [`present`]: crate::device::QueueGuard::present
+    #[inline]
+    pub unsafe fn acquire_next_image(
+        &self,
+        acquire_info: &AcquireNextImageInfo,
+    ) -> Result<AcquiredImage, Validated<VulkanError>> {
+        let is_retired_lock = self.is_retired.lock();
+        self.validate_acquire_next_image(acquire_info, *is_retired_lock)?;
+
+        Ok(self.acquire_next_image_unchecked(acquire_info)?)
+    }
+
+    fn validate_acquire_next_image(
+        &self,
+        acquire_info: &AcquireNextImageInfo,
+        is_retired: bool,
+    ) -> Result<(), Box<ValidationError>> {
+        acquire_info
+            .validate(&self.device)
+            .map_err(|err| err.add_context("acquire_info"))?;
+
+        if is_retired {
+            return Err(Box::new(ValidationError {
+                problem: "this swapchain is retired".into(),
+                vuids: &["VUID-VkAcquireNextImageInfoKHR-swapchain-01675"],
+                ..Default::default()
+            }));
+        }
+
+        // unsafe
+        // VUID-vkAcquireNextImage2KHR-surface-07784
+        // VUID-VkAcquireNextImageInfoKHR-semaphore-01288
+        // VUID-VkAcquireNextImageInfoKHR-semaphore-01781
+        // VUID-VkAcquireNextImageInfoKHR-fence-01289
+
+        Ok(())
+    }
+
+    #[cfg_attr(not(feature = "document_unchecked"), doc(hidden))]
+    pub unsafe fn acquire_next_image_unchecked(
+        &self,
+        acquire_info: &AcquireNextImageInfo,
+    ) -> Result<AcquiredImage, VulkanError> {
+        let &AcquireNextImageInfo {
+            timeout,
+            ref semaphore,
+            ref fence,
+            _ne: _,
+        } = acquire_info;
+
+        let acquire_info_vk = ash::vk::AcquireNextImageInfoKHR {
+            swapchain: self.handle,
+            timeout: timeout.map_or(u64::MAX, |duration| {
+                u64::try_from(duration.as_nanos()).unwrap()
+            }),
+            semaphore: semaphore
+                .as_ref()
+                .map(VulkanObject::handle)
+                .unwrap_or_default(),
+            fence: fence.as_ref().map(VulkanObject::handle).unwrap_or_default(),
+            device_mask: self.device.device_mask(),
+            ..Default::default()
+        };
+
+        let fns = self.device.fns();
+        let mut output = MaybeUninit::uninit();
+
+        let result = if self.device.api_version() >= Version::V1_1
+            || self.device.enabled_extensions().khr_device_group
+        {
+            (fns.khr_swapchain.acquire_next_image2_khr)(
+                self.device.handle(),
+                &acquire_info_vk,
+                output.as_mut_ptr(),
+            )
+        } else {
+            debug_assert!(acquire_info_vk.p_next.is_null());
+            (fns.khr_swapchain.acquire_next_image_khr)(
+                self.device.handle(),
+                acquire_info_vk.swapchain,
+                acquire_info_vk.timeout,
+                acquire_info_vk.semaphore,
+                acquire_info_vk.fence,
+                output.as_mut_ptr(),
+            )
+        };
+
+        let is_suboptimal = match result {
+            ash::vk::Result::SUCCESS => false,
+            ash::vk::Result::SUBOPTIMAL_KHR => true,
+            ash::vk::Result::NOT_READY => return Err(VulkanError::NotReady),
+            ash::vk::Result::TIMEOUT => return Err(VulkanError::Timeout),
+            err => {
+                let err = VulkanError::from(err);
+
+                if matches!(err, VulkanError::FullScreenExclusiveModeLost) {
+                    self.full_screen_exclusive_held
+                        .store(false, Ordering::SeqCst);
+                }
+
+                return Err(err);
+            }
+        };
+
+        Ok(AcquiredImage {
+            image_index: output.assume_init(),
+            is_suboptimal,
+        })
+    }
+
+    /// Waits for a swapchain image with a specific present ID to be presented to the user.
+    ///
+    /// For this to work, you must set [`SwapchainPresentInfo::present_id`] to `Some` when
+    /// presenting. This function will then wait until the swapchain image with the specified ID is
+    /// presented.
+    ///
+    /// Returns whether the presentation was suboptimal. This has the same meaning as in
+    /// [`AcquiredImage::is_suboptimal`].
+    #[inline]
+    pub fn wait_for_present(
+        &self,
+        present_id: NonZeroU64,
+        timeout: Option<Duration>,
+    ) -> Result<bool, Validated<VulkanError>> {
+        let is_retired_lock = self.is_retired.lock();
+        self.validate_wait_for_present(present_id, timeout, *is_retired_lock)?;
+
+        unsafe { Ok(self.wait_for_present_unchecked(present_id, timeout)?) }
+    }
+
+    fn validate_wait_for_present(
+        &self,
+        _present_id: NonZeroU64,
+        timeout: Option<Duration>,
+        is_retired: bool,
+    ) -> Result<(), Box<ValidationError>> {
+        if !self.device.enabled_features().present_wait {
+            return Err(Box::new(ValidationError {
+                requires_one_of: RequiresOneOf(&[RequiresAllOf(&[Requires::Feature(
+                    "present_wait",
+                )])]),
+                vuids: &["VUID-vkWaitForPresentKHR-presentWait-06234"],
+                ..Default::default()
+            }));
+        }
+
+        if is_retired {
+            return Err(Box::new(ValidationError {
+                problem: "this swapchain is retired".into(),
+                vuids: &["VUID-vkWaitForPresentKHR-swapchain-04997"],
+                ..Default::default()
+            }));
+        }
+
+        if let Some(timeout) = timeout {
+            if timeout.as_nanos() >= u64::MAX as u128 {
+                return Err(Box::new(ValidationError {
+                    context: "timeout".into(),
+                    problem: "is not less than `u64::MAX` nanoseconds".into(),
+                    ..Default::default()
+                }));
+            }
+        }
+
+        Ok(())
+    }
+
+    #[cfg_attr(not(feature = "document_unchecked"), doc(hidden))]
+    pub unsafe fn wait_for_present_unchecked(
+        &self,
+        present_id: NonZeroU64,
+        timeout: Option<Duration>,
+    ) -> Result<bool, VulkanError> {
+        let result = {
+            let fns = self.device.fns();
+            (fns.khr_present_wait.wait_for_present_khr)(
+                self.device.handle(),
+                self.handle,
+                present_id.get(),
+                timeout.map_or(u64::MAX, |duration| {
+                    u64::try_from(duration.as_nanos()).unwrap()
+                }),
+            )
+        };
+
+        match result {
+            ash::vk::Result::SUCCESS => Ok(false),
+            ash::vk::Result::SUBOPTIMAL_KHR => Ok(true),
+            ash::vk::Result::TIMEOUT => Err(VulkanError::Timeout),
+            err => {
+                let err = VulkanError::from(err);
+
+                if matches!(err, VulkanError::FullScreenExclusiveModeLost) {
+                    self.full_screen_exclusive_held
+                        .store(false, Ordering::SeqCst);
+                }
+
+                Err(err)
+            }
+        }
+    }
+
     /// Acquires full-screen exclusivity.
     ///
     /// The swapchain must have been created with [`FullScreenExclusive::ApplicationControlled`],
@@ -1558,6 +1776,17 @@ impl Swapchain {
             false
         }
     }
+
+    #[inline]
+    pub(crate) unsafe fn full_screen_exclusive_held(&self) -> &AtomicBool {
+        &self.full_screen_exclusive_held
+    }
+
+    #[inline]
+    pub(crate) unsafe fn try_claim_present_id(&self, present_id: NonZeroU64) -> bool {
+        let present_id = u64::from(present_id);
+        self.prev_present_id.fetch_max(present_id, Ordering::SeqCst) < present_id
+    }
 }
 
 impl Drop for Swapchain {