From 3dce2e502cc947a3120d1ff9d5d19c4b9987d834 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 21 Jul 2024 21:13:44 +0200 Subject: [PATCH] Clean up suspicious committed allocation workaround --- wgpu-hal/Cargo.toml | 4 -- wgpu-hal/src/dx12/device.rs | 30 ++-------- wgpu-hal/src/dx12/mod.rs | 2 +- wgpu-hal/src/dx12/suballocation.rs | 92 ++++++------------------------ wgpu-types/src/assertions.rs | 26 --------- 5 files changed, 24 insertions(+), 130 deletions(-) diff --git a/wgpu-hal/Cargo.toml b/wgpu-hal/Cargo.toml index 5c55842a9e7..ee2808b19d5 100644 --- a/wgpu-hal/Cargo.toml +++ b/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"] diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index 174c1b7ee28..dd681603157 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -35,11 +35,7 @@ impl super::Device { library: &Arc, dxc_container: Option>, ) -> Result { - 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 { - 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 diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs index ba724bb3695..e4b9e746378 100644 --- a/wgpu-hal/src/dx12/mod.rs +++ b/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>, + mem_allocator: Mutex, dxc_container: Option>, counters: wgt::HalCounters, } diff --git a/wgpu-hal/src/dx12/suballocation.rs b/wgpu-hal/src/dx12/suballocation.rs index 5ab483400be..ce7cfb31b1e 100644 --- a/wgpu-hal/src/dx12/suballocation.rs +++ b/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>, crate::DeviceError> { + ) -> Result, 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, ) -> Result, 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>, 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, - ) -> Result, 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,7 +293,7 @@ mod committed { return Err(crate::DeviceError::ResourceCreationFailed); } - Ok(None) + Ok(()) } pub(crate) fn create_texture_resource( @@ -341,7 +301,7 @@ mod committed { _desc: &crate::TextureDescriptor, raw_desc: Direct3D12::D3D12_RESOURCE_DESC, resource: &mut Option, - ) -> Result, 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, - ) { - // No-op when not using gpu-allocator - } - - #[allow(unused)] - pub(crate) fn free_texture_allocation( - _device: &crate::dx12::Device, - _allocation: AllocationWrapper, - _allocator: &Mutex, - ) { - // No-op when not using gpu-allocator + Ok(()) } } diff --git a/wgpu-types/src/assertions.rs b/wgpu-types/src/assertions.rs index ee10bfd56c8..a36b2e8265f 100644 --- a/wgpu-types/src/assertions.rs +++ b/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 { - /// 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 StrictAssertUnwrapExt for Option { - 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 StrictAssertUnwrapExt for Result { - 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() } - } -}