From 59dcb9f657d9b758611ad09b4e85a3ba454da66a Mon Sep 17 00:00:00 2001 From: Jerzy Wilczek Date: Thu, 22 Aug 2024 17:07:17 +0200 Subject: [PATCH] Change the `DropGuard` public API to a callback-based one. This patch also unifies the behavior of the the `DropGuard`-ish constructs in the Vulkan-based implementation. --- CHANGELOG.md | 4 ++++ wgpu-hal/src/gles/device.rs | 12 ++++++------ wgpu-hal/src/lib.rs | 20 ++++++++++++++++++-- wgpu-hal/src/vulkan/adapter.rs | 10 +++++++--- wgpu-hal/src/vulkan/device.rs | 10 ++++++---- wgpu-hal/src/vulkan/instance.rs | 10 +++++++--- wgpu-hal/src/vulkan/mod.rs | 2 +- 7 files changed, 49 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21ec6bde0c0..dcc64024250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,10 @@ By @wumpf in [#6069](https://github.com/gfx-rs/wgpu/pull/6069), [#6099](https:// - Reduce the amount of debug and trace logs emitted by wgpu-core and wgpu-hal. By @nical in [#6065](https://github.com/gfx-rs/wgpu/issues/6065) - `Rg11b10Float` is renamed to `Rg11b10UFloat`. By @sagudev in [#6108](https://github.com/gfx-rs/wgpu/pull/6108) +#### HAL + +- Change the inconsistent `DropGuard` based API on Vulkan and GLES to a consistent, callback-based one. By @jerzywilczek in ... + ### Dependency Updates #### GLES diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index ad092307e99..4210229fb76 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -110,22 +110,22 @@ impl super::Device { /// /// - `name` must be created respecting `desc` /// - `name` must be a texture - /// - If `drop_guard` is [`None`], wgpu-hal will take ownership of the texture. If `drop_guard` is - /// [`Some`], the texture must be valid until the drop implementation + /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of the texture. If + /// `drop_callback` is [`Some`], the texture must be valid until the drop implementation /// of the drop guard is called. #[cfg(any(native, Emscripten))] pub unsafe fn texture_from_raw( &self, name: std::num::NonZeroU32, desc: &crate::TextureDescriptor, - drop_guard: Option, + drop_callback: Option, ) -> super::Texture { super::Texture { inner: super::TextureInner::Texture { raw: glow::NativeTexture(name), target: super::Texture::get_info_from_desc(desc), }, - drop_guard, + drop_guard: drop_callback.map(|callback| crate::DropGuard { callback }), mip_level_count: desc.mip_level_count, array_layer_count: desc.array_layer_count(), format: desc.format, @@ -146,13 +146,13 @@ impl super::Device { &self, name: std::num::NonZeroU32, desc: &crate::TextureDescriptor, - drop_guard: Option, + drop_callback: Option, ) -> super::Texture { super::Texture { inner: super::TextureInner::Renderbuffer { raw: glow::NativeRenderbuffer(name), }, - drop_guard, + drop_guard: drop_callback.map(|callback| crate::DropGuard { callback }), mip_level_count: desc.mip_level_count, array_layer_count: desc.array_layer_count(), format: desc.format, diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index b62a6b59620..4d3446287a6 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -303,8 +303,24 @@ pub type MemoryRange = Range; pub type FenceValue = u64; pub type AtomicFenceValue = std::sync::atomic::AtomicU64; -/// Drop guard to signal wgpu-hal is no longer using an externally created object. -pub type DropGuard = Box; +/// A callback to signal that wgpu is no longer using a resource. +pub type DropCallback = Box; + +pub struct DropGuard { + callback: DropCallback, +} + +impl Drop for DropGuard { + fn drop(&mut self) { + (self.callback)(); + } +} + +impl std::fmt::Debug for DropGuard { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("DropGuard").finish() + } +} #[derive(Clone, Debug, PartialEq, Eq, Error)] pub enum DeviceError { diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index f323456eaa0..9b8374e0aa4 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -1593,11 +1593,13 @@ impl super::Adapter { /// - `raw_device` must be created from this adapter. /// - `raw_device` must be created using `family_index`, `enabled_extensions` and `physical_device_features()` /// - `enabled_extensions` must be a superset of `required_device_extensions()`. + /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `raw_device`. If + /// `drop_callback` is [`Some`], `raw_device` must be valid until the callback is called. #[allow(clippy::too_many_arguments)] pub unsafe fn device_from_raw( &self, raw_device: ash::Device, - handle_is_owned: bool, + drop_callback: Option, enabled_extensions: &[&'static CStr], features: wgt::Features, memory_hints: &wgt::MemoryHints, @@ -1812,12 +1814,14 @@ impl super::Adapter { 0, 0, 0, 0, ]; + let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback }); + let shared = Arc::new(super::DeviceShared { raw: raw_device, family_index, queue_index, raw_queue, - handle_is_owned, + drop_guard, instance: Arc::clone(&self.instance), physical_device: self.raw, enabled_extensions: enabled_extensions.into(), @@ -2005,7 +2009,7 @@ impl crate::Adapter for super::Adapter { unsafe { self.device_from_raw( raw_device, - true, + None, &enabled_extensions, features, memory_hints, diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 70136bdfb51..397f1a39162 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -290,7 +290,7 @@ impl super::DeviceShared { for &raw in self.framebuffers.lock().values() { unsafe { self.raw.destroy_framebuffer(raw, None) }; } - if self.handle_is_owned { + if self.drop_guard.is_none() { unsafe { self.raw.destroy_device(None) }; } } @@ -648,13 +648,13 @@ impl super::Device { /// # Safety /// /// - `vk_image` must be created respecting `desc` - /// - If `drop_guard` is `Some`, the application must manually destroy the image handle. This - /// can be done inside the `Drop` impl of `drop_guard`. + /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `vk_image`. If + /// `drop_callback` is [`Some`], `vk_image` must be valid until the callback is called. /// - If the `ImageCreateFlags` does not contain `MUTABLE_FORMAT`, the `view_formats` of `desc` must be empty. pub unsafe fn texture_from_raw( vk_image: vk::Image, desc: &crate::TextureDescriptor, - drop_guard: Option, + drop_callback: Option, ) -> super::Texture { let mut raw_flags = vk::ImageCreateFlags::empty(); let mut view_formats = vec![]; @@ -673,6 +673,8 @@ impl super::Device { raw_flags |= vk::ImageCreateFlags::MUTABLE_FORMAT; } + let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback }); + super::Texture { raw: vk_image, drop_guard, diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 832c74b0300..bde6af5fada 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -310,6 +310,8 @@ impl super::Instance { /// - `extensions` must be a superset of `desired_extensions()` and must be created from the /// same entry, `instance_api_version`` and flags. /// - `android_sdk_version` is ignored and can be `0` for all platforms besides Android + /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `raw_instance`. If + /// `drop_callback` is [`Some`], `raw_instance` must be valid until the callback is called. /// /// If `debug_utils_user_data` is `Some`, then the validation layer is /// available, so create a [`vk::DebugUtilsMessengerEXT`]. @@ -323,7 +325,7 @@ impl super::Instance { extensions: Vec<&'static CStr>, flags: wgt::InstanceFlags, has_nv_optimus: bool, - drop_guard: Option, + drop_callback: Option, ) -> Result { log::debug!("Instance version: 0x{:x}", instance_api_version); @@ -364,6 +366,8 @@ impl super::Instance { None }; + let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback }); + Ok(Self { shared: Arc::new(super::InstanceShared { raw: raw_instance, @@ -555,7 +559,7 @@ impl Drop for super::InstanceShared { .destroy_debug_utils_messenger(du.messenger, None); du }); - if let Some(_drop_guard) = self.drop_guard.take() { + if self.drop_guard.is_none() { self.raw.destroy_instance(None); } } @@ -829,7 +833,7 @@ impl crate::Instance for super::Instance { extensions, desc.flags, has_nv_optimus, - Some(Box::new(())), // `Some` signals that wgpu-hal is in charge of destroying vk_instance + None, ) } } diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index 26186d5fa8e..3815dfc59d0 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -547,7 +547,7 @@ struct DeviceShared { family_index: u32, queue_index: u32, raw_queue: vk::Queue, - handle_is_owned: bool, + drop_guard: Option, instance: Arc, physical_device: vk::PhysicalDevice, enabled_extensions: Vec<&'static CStr>,