Clean up suspicious committed allocation workaround

wgpu-hal/Cargo.toml

@@ -76,7 +76,6 @@ dx12 = [
     "dep:range-alloc",
     "dep:windows-core",
     "gpu-allocator/d3d12",
-    "windows_rs",
     "naga/hlsl-out-if-target-windows",
     "windows/Win32_Graphics_Direct3D_Fxc",
     "windows/Win32_Graphics_Direct3D",
@@ -90,9 +89,6 @@ dx12 = [
     "windows/Win32_System_Threading",
     "windows/Win32_UI_WindowsAndMessaging",
 ]
-windows_rs = [
-    "dep:gpu-allocator",
-] # TODO: This feature is optional after the migration to windows-rs
 dxc_shader_compiler = ["dep:hassle-rs"]
 renderdoc = ["dep:libloading", "dep:renderdoc-sys"]
 fragile-send-sync-non-atomic-wasm = ["wgt/fragile-send-sync-non-atomic-wasm"]

wgpu-hal/src/dx12/device.rs

@@ -35,11 +35,7 @@ impl super::Device {
         library: &Arc<D3D12Lib>,
         dxc_container: Option<Arc<shader_compilation::DxcContainer>>,
     ) -> Result<Self, crate::DeviceError> {
-        let mem_allocator = if private_caps.suballocation_supported {
-            super::suballocation::create_allocator_wrapper(&raw, memory_hints)?
-        } else {
-            None
-        };
+        let mem_allocator = super::suballocation::create_allocator_wrapper(&raw, memory_hints)?;
 
         let idle_fence: Direct3D12::ID3D12Fence = unsafe {
             profiling::scope!("ID3D12Device::CreateFence");
@@ -383,9 +379,8 @@ impl super::Device {
 impl crate::Device for super::Device {
     type A = super::Api;
 
-    unsafe fn exit(mut self, _queue: super::Queue) {
+    unsafe fn exit(self, _queue: super::Queue) {
         self.rtv_pool.lock().free_handle(self.null_rtv_handle);
-        self.mem_allocator = None;
     }
 
     unsafe fn create_buffer(
@@ -415,7 +410,6 @@ impl crate::Device for super::Device {
             Flags: conv::map_buffer_usage_to_resource_flags(desc.usage),
         };
 
-        // TODO: Return HR as part of DeviceError straight away?
         let allocation =
             super::suballocation::create_buffer_resource(self, desc, raw_desc, &mut resource)?;
 
@@ -436,17 +430,12 @@ impl crate::Device for super::Device {
     }
 
     unsafe fn destroy_buffer(&self, mut buffer: super::Buffer) {
-        // Only happens when it's using the windows_rs feature and there's an allocation
+        // Always Some except on Intel Xe: https://github.com/gfx-rs/wgpu/issues/3552
         if let Some(alloc) = buffer.allocation.take() {
             // Resource should be dropped before free suballocation
             drop(buffer);
 
-            super::suballocation::free_buffer_allocation(
-                self,
-                alloc,
-                // SAFETY: for allocations to exist, the allocator must exist
-                unsafe { self.mem_allocator.as_ref().unwrap_unchecked() },
-            );
+            super::suballocation::free_buffer_allocation(self, alloc, &self.mem_allocator);
         }
 
         self.counters.buffers.sub(1);
@@ -536,7 +525,7 @@ impl crate::Device for super::Device {
                 self,
                 alloc,
                 // SAFETY: for allocations to exist, the allocator must exist
-                unsafe { self.mem_allocator.as_ref().unwrap_unchecked() },
+                &self.mem_allocator,
             );
         }
 
@@ -1829,15 +1818,8 @@ impl crate::Device for super::Device {
         self.counters.clone()
     }
 
-    #[cfg(feature = "windows_rs")]
     fn generate_allocator_report(&self) -> Option<wgt::AllocatorReport> {
-        let mut upstream = {
-            self.mem_allocator
-                .as_ref()?
-                .lock()
-                .allocator
-                .generate_report()
-        };
+        let mut upstream = self.mem_allocator.lock().allocator.generate_report();
 
         let allocations = upstream
             .allocations

wgpu-hal/src/dx12/mod.rs

@@ -548,7 +548,7 @@ pub struct Device {
     #[cfg(feature = "renderdoc")]
     render_doc: auxil::renderdoc::RenderDoc,
     null_rtv_handle: descriptor::Handle,
-    mem_allocator: Option<Mutex<suballocation::GpuAllocatorWrapper>>,
+    mem_allocator: Mutex<suballocation::GpuAllocatorWrapper>,
     dxc_container: Option<Arc<shader_compilation::DxcContainer>>,
     counters: wgt::HalCounters,
 }

wgpu-hal/src/dx12/suballocation.rs

@@ -1,26 +1,18 @@
+// TODO: Refactor this module to not be split between two interchangeable APIs, as both are used
+// at the same time.
 pub(crate) use allocation::{
     create_allocator_wrapper, create_buffer_resource, create_texture_resource,
     free_buffer_allocation, free_texture_allocation, AllocationWrapper, GpuAllocatorWrapper,
 };
 
-#[cfg(not(feature = "windows_rs"))]
-use committed as allocation;
-#[cfg(feature = "windows_rs")]
+/// Placed allocations are always favoured
 use placed as allocation;
 
-// TODO: windows_rs is the default now?
-// This exists to work around https://github.com/gfx-rs/wgpu/issues/3207
-// Currently this will work the older, slower way if the windows_rs feature is disabled,
-// and will use the fast path of suballocating buffers and textures using gpu_allocator if
-// the windows_rs feature is enabled.
-
 // This is the fast path using gpu_allocator to suballocate buffers and textures.
-#[cfg(feature = "windows_rs")]
 mod placed {
 
     use gpu_allocator::{d3d12::AllocationCreateDesc, MemoryLocation};
     use parking_lot::Mutex;
-    use wgt::assertions::StrictAssertUnwrapExt;
     use windows::Win32::Graphics::Direct3D12;
 
     use crate::auxil::dxgi::result::HResult as _;
@@ -38,7 +30,7 @@ mod placed {
     pub(crate) fn create_allocator_wrapper(
         raw: &Direct3D12::ID3D12Device,
         memory_hints: &wgt::MemoryHints,
-    ) -> Result<Option<Mutex<GpuAllocatorWrapper>>, crate::DeviceError> {
+    ) -> Result<Mutex<GpuAllocatorWrapper>, crate::DeviceError> {
         // TODO: the allocator's configuration should take hardware capability into
         // account.
         let mb = 1024 * 1024;
@@ -61,7 +53,7 @@ mod placed {
             debug_settings: Default::default(),
             allocation_sizes,
         }) {
-            Ok(allocator) => Ok(Some(Mutex::new(GpuAllocatorWrapper { allocator }))),
+            Ok(allocator) => Ok(Mutex::new(GpuAllocatorWrapper { allocator })),
             Err(e) => {
                 log::error!("Failed to create d3d12 allocator, error: {}", e);
                 Err(e)?
@@ -78,10 +70,10 @@ mod placed {
         let is_cpu_read = desc.usage.contains(crate::BufferUses::MAP_READ);
         let is_cpu_write = desc.usage.contains(crate::BufferUses::MAP_WRITE);
 
-        // It's a workaround for Intel Xe drivers.
+        // Workaround for Intel Xe drivers
         if !device.private_caps.suballocation_supported {
             return super::committed::create_buffer_resource(device, desc, raw_desc, resource)
-                .map(|_| None);
+                .map(|()| None);
         }
 
         let location = match (is_cpu_read, is_cpu_write) {
@@ -93,16 +85,8 @@ mod placed {
 
         let name = desc.label.unwrap_or("Unlabeled buffer");
 
-        // SAFETY: allocator exists when the windows_rs feature is enabled
-        let mut allocator = unsafe {
-            device
-                .mem_allocator
-                .as_ref()
-                .strict_unwrap_unchecked()
-                .lock()
-        };
+        let mut allocator = device.mem_allocator.lock();
 
-        // let mut allocator = unsafe { device.mem_allocator.as_ref().unwrap_unchecked().lock() };
         let allocation_desc = AllocationCreateDesc::from_d3d12_resource_desc(
             allocator.allocator.device(),
             &raw_desc,
@@ -141,24 +125,17 @@ mod placed {
         raw_desc: Direct3D12::D3D12_RESOURCE_DESC,
         resource: &mut Option<Direct3D12::ID3D12Resource>,
     ) -> Result<Option<AllocationWrapper>, crate::DeviceError> {
-        // It's a workaround for Intel Xe drivers.
+        // Workaround for Intel Xe drivers
         if !device.private_caps.suballocation_supported {
             return super::committed::create_texture_resource(device, desc, raw_desc, resource)
-                .map(|_| None);
+                .map(|()| None);
         }
 
         let location = MemoryLocation::GpuOnly;
 
         let name = desc.label.unwrap_or("Unlabeled texture");
 
-        // SAFETY: allocator exists when the windows_rs feature is enabled
-        let mut allocator = unsafe {
-            device
-                .mem_allocator
-                .as_ref()
-                .strict_unwrap_unchecked()
-                .lock()
-        };
+        let mut allocator = device.mem_allocator.lock();
         let allocation_desc = AllocationCreateDesc::from_d3d12_resource_desc(
             allocator.allocator.device(),
             &raw_desc,
@@ -261,36 +238,19 @@ mod placed {
     }
 }
 
-// This is the older, slower path where it doesn't suballocate buffers.
-// Tracking issue for when it can be removed: https://github.com/gfx-rs/wgpu/issues/3207
+// This is the older, slower path where it doesn't suballocate buffers, which appears to
+// be broken on Intel Xe: https://github.com/gfx-rs/wgpu/issues/3552
 mod committed {
-    use parking_lot::Mutex;
     use windows::Win32::Graphics::Direct3D12;
 
     use crate::auxil::dxgi::result::HResult;
 
-    // Allocator isn't needed when not suballocating with gpu_allocator
-    #[derive(Debug)]
-    pub(crate) struct GpuAllocatorWrapper {}
-
-    // Allocations aren't needed when not suballocating with gpu_allocator
-    #[derive(Debug)]
-    pub(crate) struct AllocationWrapper {}
-
-    #[allow(unused)]
-    pub(crate) fn create_allocator_wrapper(
-        _raw: &Direct3D12::ID3D12Device,
-        _memory_hints: &wgt::MemoryHints,
-    ) -> Result<Option<Mutex<GpuAllocatorWrapper>>, crate::DeviceError> {
-        Ok(None)
-    }
-
     pub(crate) fn create_buffer_resource(
         device: &crate::dx12::Device,
         desc: &crate::BufferDescriptor,
         raw_desc: Direct3D12::D3D12_RESOURCE_DESC,
         resource: &mut Option<Direct3D12::ID3D12Resource>,
-    ) -> Result<Option<AllocationWrapper>, crate::DeviceError> {
+    ) -> Result<(), crate::DeviceError> {
         let is_cpu_read = desc.usage.contains(crate::BufferUses::MAP_READ);
         let is_cpu_write = desc.usage.contains(crate::BufferUses::MAP_WRITE);
 
@@ -333,15 +293,15 @@ mod committed {
             return Err(crate::DeviceError::ResourceCreationFailed);
         }
 
-        Ok(None)
+        Ok(())
     }
 
     pub(crate) fn create_texture_resource(
         device: &crate::dx12::Device,
         _desc: &crate::TextureDescriptor,
         raw_desc: Direct3D12::D3D12_RESOURCE_DESC,
         resource: &mut Option<Direct3D12::ID3D12Resource>,
-    ) -> Result<Option<AllocationWrapper>, crate::DeviceError> {
+    ) -> Result<(), crate::DeviceError> {
         let heap_properties = Direct3D12::D3D12_HEAP_PROPERTIES {
             Type: Direct3D12::D3D12_HEAP_TYPE_CUSTOM,
             CPUPageProperty: Direct3D12::D3D12_CPU_PAGE_PROPERTY_NOT_AVAILABLE,
@@ -373,24 +333,6 @@ mod committed {
             return Err(crate::DeviceError::ResourceCreationFailed);
         }
 
-        Ok(None)
-    }
-
-    #[allow(unused)]
-    pub(crate) fn free_buffer_allocation(
-        _device: &crate::dx12::Device,
-        _allocation: AllocationWrapper,
-        _allocator: &Mutex<GpuAllocatorWrapper>,
-    ) {
-        // No-op when not using gpu-allocator
-    }
-
-    #[allow(unused)]
-    pub(crate) fn free_texture_allocation(
-        _device: &crate::dx12::Device,
-        _allocation: AllocationWrapper,
-        _allocator: &Mutex<GpuAllocatorWrapper>,
-    ) {
-        // No-op when not using gpu-allocator
+        Ok(())
     }
 }

wgpu-types/src/assertions.rs

@@ -64,29 +64,3 @@ macro_rules! strict_assert_ne {
         debug_assert_ne!( $( $arg )* )
     };
 }
-
-/// Unwrapping using strict_asserts
-pub trait StrictAssertUnwrapExt<T> {
-    /// Unchecked unwrap, with a [`strict_assert`] backed assertion of validitly.
-    ///
-    /// # Safety
-    ///
-    /// It _must_ be valid to call unwrap_unchecked on this value.
-    unsafe fn strict_unwrap_unchecked(self) -> T;
-}
-
-impl<T> StrictAssertUnwrapExt<T> for Option<T> {
-    unsafe fn strict_unwrap_unchecked(self) -> T {
-        strict_assert!(self.is_some(), "Called strict_unwrap_unchecked on None");
-        // SAFETY: Checked by above assert, or by assertion by unsafe.
-        unsafe { self.unwrap_unchecked() }
-    }
-}
-
-impl<T, E> StrictAssertUnwrapExt<T> for Result<T, E> {
-    unsafe fn strict_unwrap_unchecked(self) -> T {
-        strict_assert!(self.is_ok(), "Called strict_unwrap_unchecked on Err");
-        // SAFETY: Checked by above assert, or by assertion by unsafe.
-        unsafe { self.unwrap_unchecked() }
-    }
-}