Skip to content

Commit

Permalink
[wgpu-hal] #5956 windows-rs migration followups and cleanups
Browse files Browse the repository at this point in the history 
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 microsoft/win32metadata#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.
  • Loading branch information
@MarijnS95
MarijnS95 committed Aug 29, 2024
1 parent bbdbafd commit be2ec15
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 71 deletions.
35 changes: 6 additions & 29 deletions wgpu-hal/src/auxil/dxgi/result.rs
View file
Original file line number Diff line number Diff line change
@@ -1,56 +1,33 @@
use std::borrow::Cow;

use windows::Win32::{Foundation, Graphics::Dxgi};

pub(crate) trait HResult<O> {
fn into_result(self) -> Result<O, Cow<'static, str>>;
fn into_device_result(self, description: &str) -> Result<O, crate::DeviceError>;
}
impl<T> HResult<T> for windows::core::Result<T> {
fn into_result(self) -> Result<T, Cow<'static, str>> {
// 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<T, crate::DeviceError> {
#![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
})
}
}
33 changes: 23 additions & 10 deletions wgpu-hal/src/dx12/command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -725,7 +727,7 @@ impl crate::CommandEncoder for super::CommandEncoder {
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 {
Expand Down Expand Up @@ -754,12 +756,23 @@ impl crate::CommandEncoder for super::CommandEncoder {
if let Some(ds_view) = ds_view {
if flags != Direct3D12::D3D12_CLEAR_FLAGS::default() {
unsafe {
list.ClearDepthStencilView(
ds_view,
// 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),
mem::transmute(ds_view),
flags,
ds.clear_value.0,
ds.clear_value.1 as u8,
&[],
0,
std::ptr::null(),
)
}
}
Expand Down Expand Up @@ -796,7 +809,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 {
Expand All @@ -813,7 +826,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 {
Expand Down
22 changes: 13 additions & 9 deletions wgpu-hal/src/dx12/descriptor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
})
}
Expand Down Expand Up @@ -268,12 +270,14 @@ impl CpuHeap {
let raw = unsafe { device.CreateDescriptorHeap::<Direct3D12::ID3D12DescriptorHeap>(&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,
})
Expand All @@ -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],
Expand Down
4 changes: 2 additions & 2 deletions wgpu-hal/src/dx12/device.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions wgpu-hal/src/dx12/instance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,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,
}
}
Expand Down
30 changes: 12 additions & 18 deletions wgpu-hal/src/dx12/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1092,38 +1091,33 @@ 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",
));
}
}
}

match swap_chain1.cast::<Dxgi::IDXGISwapChain3>() {
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::<Dxgi::IDXGISwapChain3>().map_err(|err| {
log::error!("Unable to cast swapchain: {}", err);
crate::SurfaceError::Other("swapchain cast to version 3")
})?
}
};

Expand Down
2 changes: 1 addition & 1 deletion wgpu-hal/src/dx12/shader_compilation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub(super) fn compile_fxc(
)
};

match hr.into_result() {
match hr {
Ok(()) => {
let shader_data = shader_data.unwrap();
(
Expand Down

0 comments on commit be2ec15

Please sign in to comment.