From 4454cbfaab544c9e273d69673e754848006f78a0 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 30 Aug 2024 11:02:38 +0200 Subject: [PATCH] [wgpu-hal] #5956 `windows-rs` migration followups and cleanups (#6173) PR #5956 wasn't fully complete and still had some outstanding minor issues and cleanups to be done, as well as hidden semantic changes. This addresses a bunch of them: - Remove unnecessary `Error` mapping to `String` as `windows-rs`'s `Error` has a more complete `Display` representation by itself. - Remove `into_result()` as every call could have formatted the `windows-rs` `Error` in a log call directly. - Pass `None` instead of a pointer to an empty slice wherever possible (waiting for https://github.com/microsoft/win32metadata/pull/1971 to trickle down into `windows-rs`). - Remove `.clone()` on COM objects (temporarily increasing the refcount) when it can be avoided by inverting the order of operations on `raw` variables. --- CHANGELOG.md | 2 +- wgpu-hal/src/auxil/dxgi/result.rs | 34 ++++--------------------- wgpu-hal/src/dx12/command.rs | 32 +++++++++++++++-------- wgpu-hal/src/dx12/descriptor.rs | 22 +++++++++------- wgpu-hal/src/dx12/device.rs | 4 +-- wgpu-hal/src/dx12/instance.rs | 9 +++---- wgpu-hal/src/dx12/mod.rs | 30 +++++++++------------- wgpu-hal/src/dx12/shader_compilation.rs | 4 +-- 8 files changed, 59 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdca81a71..548145448 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,7 +119,7 @@ By @teoxoy [#6134](https://github.com/gfx-rs/wgpu/pull/6134). #### DX12 -- Replace `winapi` code to use the `windows` crate. By @MarijnS95 in [#5956](https://github.com/gfx-rs/wgpu/pull/5956) +- Replace `winapi` code to use the `windows` crate. By @MarijnS95 in [#5956](https://github.com/gfx-rs/wgpu/pull/5956) and [#6173](https://github.com/gfx-rs/wgpu/pull/6173) ## 22.0.0 (2024-07-17) diff --git a/wgpu-hal/src/auxil/dxgi/result.rs b/wgpu-hal/src/auxil/dxgi/result.rs index 3bb88b5bf..61e3c6acf 100644 --- a/wgpu-hal/src/auxil/dxgi/result.rs +++ b/wgpu-hal/src/auxil/dxgi/result.rs @@ -1,56 +1,32 @@ -use std::borrow::Cow; - use windows::Win32::{Foundation, Graphics::Dxgi}; pub(crate) trait HResult { - fn into_result(self) -> Result>; fn into_device_result(self, description: &str) -> Result; } impl HResult for windows::core::Result { - fn into_result(self) -> Result> { - // TODO: use windows-rs built-in error formatting? - let description = match self { - Ok(t) => return Ok(t), - Err(e) if e.code() == Foundation::E_UNEXPECTED => "unexpected", - Err(e) if e.code() == Foundation::E_NOTIMPL => "not implemented", - Err(e) if e.code() == Foundation::E_OUTOFMEMORY => "out of memory", - Err(e) if e.code() == Foundation::E_INVALIDARG => "invalid argument", - Err(e) => return Err(Cow::Owned(format!("{e:?}"))), - }; - Err(Cow::Borrowed(description)) - } fn into_device_result(self, description: &str) -> Result { #![allow(unreachable_code)] - let err_code = if let Err(err) = &self { - Some(err.code()) - } else { - None - }; - self.into_result().map_err(|err| { + self.map_err(|err| { log::error!("{} failed: {}", description, err); - let Some(err_code) = err_code else { - unreachable!() - }; - - match err_code { + match err.code() { Foundation::E_OUTOFMEMORY => { #[cfg(feature = "oom_panic")] panic!("{description} failed: Out of memory"); - return crate::DeviceError::OutOfMemory; + crate::DeviceError::OutOfMemory } Dxgi::DXGI_ERROR_DEVICE_RESET | Dxgi::DXGI_ERROR_DEVICE_REMOVED => { #[cfg(feature = "device_lost_panic")] panic!("{description} failed: Device lost ({err})"); + crate::DeviceError::Lost } _ => { #[cfg(feature = "internal_error_panic")] panic!("{description} failed: {err}"); + crate::DeviceError::Unexpected } } - - crate::DeviceError::Lost }) } } diff --git a/wgpu-hal/src/dx12/command.rs b/wgpu-hal/src/dx12/command.rs index 5f32480fd..00f56cdb5 100644 --- a/wgpu-hal/src/dx12/command.rs +++ b/wgpu-hal/src/dx12/command.rs @@ -53,7 +53,6 @@ impl crate::BufferTextureCopy { impl super::Temp { fn prepare_marker(&mut self, marker: &str) -> (&[u16], u32) { - // TODO: Store in HSTRING self.marker.clear(); self.marker.extend(marker.encode_utf16()); self.marker.push(0); @@ -153,7 +152,7 @@ impl super::CommandEncoder { self.update_root_elements(); } - //Note: we have to call this lazily before draw calls. Otherwise, D3D complains + // Note: we have to call this lazily before draw calls. Otherwise, D3D complains // about the root parameters being incompatible with root signature. fn update_root_elements(&mut self) { use super::{BufferViewKind as Bvk, PassKind as Pk}; @@ -265,7 +264,8 @@ impl crate::CommandEncoder for super::CommandEncoder { unsafe fn begin_encoding(&mut self, label: crate::Label) -> Result<(), crate::DeviceError> { let list = loop { if let Some(list) = self.free_lists.pop() { - let reset_result = unsafe { list.Reset(&self.allocator, None) }.into_result(); + // TODO: Is an error expected here and should we print it? + let reset_result = unsafe { list.Reset(&self.allocator, None) }; if reset_result.is_ok() { break Some(list); } @@ -314,7 +314,9 @@ impl crate::CommandEncoder for super::CommandEncoder { for cmd_buf in command_buffers { self.free_lists.push(cmd_buf.raw); } - let _todo_handle_error = unsafe { self.allocator.Reset() }; + if let Err(e) = unsafe { self.allocator.Reset() } { + log::error!("ID3D12CommandAllocator::Reset() failed with {e}"); + } } unsafe fn transition_buffers<'a, T>(&mut self, barriers: T) @@ -724,8 +726,7 @@ impl crate::CommandEncoder for super::CommandEncoder { cat.clear_value.b as f32, cat.clear_value.a as f32, ]; - // TODO: Empty slice vs None? - unsafe { list.ClearRenderTargetView(*rtv, &value, Some(&[])) }; + unsafe { list.ClearRenderTargetView(*rtv, &value, None) }; } if let Some(ref target) = cat.resolve_target { self.pass.resolves.push(super::PassResolve { @@ -754,12 +755,23 @@ impl crate::CommandEncoder for super::CommandEncoder { if let Some(ds_view) = ds_view { if flags != Direct3D12::D3D12_CLEAR_FLAGS::default() { unsafe { - list.ClearDepthStencilView( + // list.ClearDepthStencilView( + // ds_view, + // flags, + // ds.clear_value.0, + // ds.clear_value.1 as u8, + // None, + // ) + // TODO: Replace with the above in the next breaking windows-rs release, + // when https://github.com/microsoft/win32metadata/pull/1971 is in. + (windows_core::Interface::vtable(list).ClearDepthStencilView)( + windows_core::Interface::as_raw(list), ds_view, flags, ds.clear_value.0, ds.clear_value.1 as u8, - &[], + 0, + std::ptr::null(), ) } } @@ -796,7 +808,7 @@ impl crate::CommandEncoder for super::CommandEncoder { Type: Direct3D12::D3D12_RESOURCE_BARRIER_TYPE_TRANSITION, Flags: Direct3D12::D3D12_RESOURCE_BARRIER_FLAG_NONE, Anonymous: Direct3D12::D3D12_RESOURCE_BARRIER_0 { - //Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`. + // Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`. // If it's not the case, we can include the `TextureUses` in `PassResove`. Transition: mem::ManuallyDrop::new( Direct3D12::D3D12_RESOURCE_TRANSITION_BARRIER { @@ -813,7 +825,7 @@ impl crate::CommandEncoder for super::CommandEncoder { Type: Direct3D12::D3D12_RESOURCE_BARRIER_TYPE_TRANSITION, Flags: Direct3D12::D3D12_RESOURCE_BARRIER_FLAG_NONE, Anonymous: Direct3D12::D3D12_RESOURCE_BARRIER_0 { - //Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`. + // Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`. // If it's not the case, we can include the `TextureUses` in `PassResolve`. Transition: mem::ManuallyDrop::new( Direct3D12::D3D12_RESOURCE_TRANSITION_BARRIER { diff --git a/wgpu-hal/src/dx12/descriptor.rs b/wgpu-hal/src/dx12/descriptor.rs index ebb42ddcd..f3b7f26f2 100644 --- a/wgpu-hal/src/dx12/descriptor.rs +++ b/wgpu-hal/src/dx12/descriptor.rs @@ -56,16 +56,18 @@ impl GeneralHeap { .into_device_result("Descriptor heap creation")? }; + let start = DualHandle { + cpu: unsafe { raw.GetCPUDescriptorHandleForHeapStart() }, + gpu: unsafe { raw.GetGPUDescriptorHandleForHeapStart() }, + count: 0, + }; + Ok(Self { - raw: raw.clone(), + raw, ty, handle_size: unsafe { device.GetDescriptorHandleIncrementSize(ty) } as u64, total_handles, - start: DualHandle { - cpu: unsafe { raw.GetCPUDescriptorHandleForHeapStart() }, - gpu: unsafe { raw.GetGPUDescriptorHandleForHeapStart() }, - count: 0, - }, + start, ranges: Mutex::new(RangeAllocator::new(0..total_handles)), }) } @@ -268,12 +270,14 @@ impl CpuHeap { let raw = unsafe { device.CreateDescriptorHeap::(&desc) } .into_device_result("CPU descriptor heap creation")?; + let start = unsafe { raw.GetCPUDescriptorHandleForHeapStart() }; + Ok(Self { inner: Mutex::new(CpuHeapInner { - _raw: raw.clone(), + _raw: raw, stage: Vec::new(), }), - start: unsafe { raw.GetCPUDescriptorHandleForHeapStart() }, + start, handle_size, total, }) @@ -297,7 +301,7 @@ impl fmt::Debug for CpuHeap { } pub(super) unsafe fn upload( - device: Direct3D12::ID3D12Device, + device: &Direct3D12::ID3D12Device, src: &CpuHeapInner, dst: &GeneralHeap, dummy_copy_counts: &[u32], diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index dd6816031..04a919047 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -1295,7 +1295,7 @@ impl crate::Device for super::Device { Some(inner) => { let dual = unsafe { descriptor::upload( - self.raw.clone(), + &self.raw, &inner, &self.shared.heap_views, &desc.layout.copy_counts, @@ -1309,7 +1309,7 @@ impl crate::Device for super::Device { Some(inner) => { let dual = unsafe { descriptor::upload( - self.raw.clone(), + &self.raw, &inner, &self.shared.heap_samplers, &desc.layout.copy_counts, diff --git a/wgpu-hal/src/dx12/instance.rs b/wgpu-hal/src/dx12/instance.rs index 036561619..7dc7713a0 100644 --- a/wgpu-hal/src/dx12/instance.rs +++ b/wgpu-hal/src/dx12/instance.rs @@ -10,10 +10,7 @@ use windows::{ }; use super::SurfaceTarget; -use crate::{ - auxil::{self, dxgi::result::HResult as _}, - dx12::D3D12Lib, -}; +use crate::{auxil, dx12::D3D12Lib}; impl Drop for super::Instance { fn drop(&mut self) { @@ -98,8 +95,8 @@ impl crate::Instance for super::Instance { ) }; - match hr.into_result() { - Err(err) => log::warn!("Unable to check for tearing support: {}", err), + match hr { + Err(err) => log::warn!("Unable to check for tearing support: {err}"), Ok(()) => supports_allow_tearing = true, } } diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs index e4b9e7463..16bb88600 100644 --- a/wgpu-hal/src/dx12/mod.rs +++ b/wgpu-hal/src/dx12/mod.rs @@ -49,14 +49,13 @@ use std::{ffi, fmt, mem, num::NonZeroU32, ops::Deref, sync::Arc}; use arrayvec::ArrayVec; use parking_lot::{Mutex, RwLock}; use windows::{ - core::{Interface, Param as _}, + core::{Free, Interface, Param as _}, Win32::{ Foundation, Graphics::{Direct3D, Direct3D12, DirectComposition, Dxgi}, System::Threading, }, }; -use windows_core::Free; use crate::auxil::{ self, @@ -1026,8 +1025,8 @@ impl crate::Surface for Surface { flags, ) }; - if let Err(err) = result.into_result() { - log::error!("ResizeBuffers failed: {}", err); + if let Err(err) = result { + log::error!("ResizeBuffers failed: {err}"); return Err(crate::SurfaceError::Other("window is in use")); } raw @@ -1092,24 +1091,22 @@ impl crate::Surface for Surface { let swap_chain1 = swap_chain1.map_err(|err| { log::error!("SwapChain creation error: {}", err); - crate::SurfaceError::Other("swap chain creation") + crate::SurfaceError::Other("swapchain creation") })?; match &self.target { SurfaceTarget::WndHandle(_) | SurfaceTarget::SurfaceHandle(_) => {} SurfaceTarget::Visual(visual) => { - if let Err(err) = unsafe { visual.SetContent(&swap_chain1) }.into_result() { - log::error!("Unable to SetContent: {}", err); + if let Err(err) = unsafe { visual.SetContent(&swap_chain1) } { + log::error!("Unable to SetContent: {err}"); return Err(crate::SurfaceError::Other( "IDCompositionVisual::SetContent", )); } } SurfaceTarget::SwapChainPanel(swap_chain_panel) => { - if let Err(err) = - unsafe { swap_chain_panel.SetSwapChain(&swap_chain1) }.into_result() - { - log::error!("Unable to SetSwapChain: {}", err); + if let Err(err) = unsafe { swap_chain_panel.SetSwapChain(&swap_chain1) } { + log::error!("Unable to SetSwapChain: {err}"); return Err(crate::SurfaceError::Other( "ISwapChainPanelNative::SetSwapChain", )); @@ -1117,13 +1114,10 @@ impl crate::Surface for Surface { } } - match swap_chain1.cast::() { - Ok(swap_chain3) => swap_chain3, - Err(err) => { - log::error!("Unable to cast swap chain: {}", err); - return Err(crate::SurfaceError::Other("swap chain cast to 3")); - } - } + swap_chain1.cast::().map_err(|err| { + log::error!("Unable to cast swapchain: {err}"); + crate::SurfaceError::Other("swapchain cast to version 3") + })? } }; diff --git a/wgpu-hal/src/dx12/shader_compilation.rs b/wgpu-hal/src/dx12/shader_compilation.rs index 8385082e3..615d34cfc 100644 --- a/wgpu-hal/src/dx12/shader_compilation.rs +++ b/wgpu-hal/src/dx12/shader_compilation.rs @@ -4,8 +4,6 @@ use std::ptr; pub(super) use dxc::{compile_dxc, get_dxc_container, DxcContainer}; use windows::Win32::Graphics::Direct3D; -use crate::auxil::dxgi::result::HResult; - // This exists so that users who don't want to use dxc can disable the dxc_shader_compiler feature // and not have to compile hassle_rs. // Currently this will use Dxc if it is chosen as the dx12 compiler at `Instance` creation time, and will @@ -57,7 +55,7 @@ pub(super) fn compile_fxc( ) }; - match hr.into_result() { + match hr { Ok(()) => { let shader_data = shader_data.unwrap(); (