D3D12 cleanup #6200

teoxoy · 2024-09-02T17:34:07Z

Connections
Follow-up to #5956.

Description
Mainly cleans-up usage of the D3D12 API.
Also fixes an issue where only high performance adapters would be enumerated if DXGI 1.6 is available.
I will have another follow-up to this that will go over and handle errors on a case-by-case basis (similar to #6119).

Testing
Existing tests. I also tested DXC locally with the changes to shader compilation.

PR doesn't need to be squashed, each commit builds by itself.

nical

Glorious.

wgpu-hal/src/dx12/mod.rs

…s instead of using out params

…rror::Unexpected` These are guaranteed to be populated if the creation methods didn't error. If they are not, the driver/runtime is at fault.

Also fixes an issue where only high performance adapters would be enumerated if DXGI 1.6 is available.

…K` & `MULTIPLE_DEVICES` tests The test harness creates the instance and adapter that a test should use. Tests should not create these with default configurations or configs from the environment.

MarijnS95 · 2024-09-03T15:57:57Z

Cargo.toml

@@ -149,7 +149,6 @@ gpu-descriptor = "0.3"
 bit-set = "0.8"
 gpu-allocator = { version = "0.27", default-features = false }
 range-alloc = "0.1"
-hassle-rs = "0.11.0"


For future reference, the only reason why hassle-rs cannot use windows-rs as bindings is because the windows-rs maintainers refuse to allow basic Linux support. DirectxShaderConverter works on Linux via the same COM bindings otherwise.

(Either way the abstraction provided by hassle-rs is far from ideal)

I found it more straightforward to just use the bindings directly since we have to deal with them for D3D12 anyway.

Yeah the abstraction by hassle-rs isn't that useful. It's mostly creating the bindings but we have those now (except not for Linux 😭) in windows-rs.

MarijnS95 · 2024-09-03T16:08:48Z

wgpu-hal/src/dx12/mod.rs

        .ok()
-        // TODO: If there's a HRESULT, error may still be non-null and
-        // contain info.
        .into_device_result("Root signature serialization")?;


The idea was to check the error blob before bailing out with into_device_result(), missed this during the previous cleanup. The TODO remains valid.

Hmm, is that something we should handle? What would be an example?

The only scenario I can think of is: the blob getting partially filled with errors after which we get an OOM but it's probably ok to error with an OOM instead of reporting the partial errors.

I wish the exact error codes were documented (like in Vulkan), now we have to guess which variants we might run into...

I was expecting a similar setup like Compile() the error doesn't need to be OOM.

For Compile(), I'm not sure which HRESULTs to check either. There is:

The result from Compile();

Non-null error blob;

Null blob;

Result from IDxcOperationResult::GetStatus().

(and any getters on IDxcOperationResult/IDxcResult can fail as well)

Left a reply in #6200 (comment).

MarijnS95 · 2024-09-03T16:09:48Z

wgpu-hal/src/dx12/mod.rs

+        let mut result__ = None;
+
+        (func)(&Direct3D12::ID3D12Debug::IID, <*mut _>::cast(&mut result__))
+            .ok()
+            .into_device_result("GetDebugInterface")?;

-        let mut result__ = core::ptr::null_mut();
-        Ok((func)(&Direct3D12::ID3D12Debug::IID, &mut result__)
-            .and_then(|| unsafe { windows_core::Type::from_abi(result__) }))
+        result__.ok_or(crate::DeviceError::Unexpected)


Note that a lot of this was intentionally verbatim-copied from windows-rs to allow easier updating.

I see, but I think it's more valuable to have those be consistent. I'm also not a fan of using the from_abi function which maps null to Error::empty().

Right yes, that is unfortunate.

Perhaps this also has to do with SAL annotations, if I remember correctly there are different annotations that allow return values to be NULL, versus them only being NULL when a non-SUCCESS RESULT is returned.

The original code now no longer relies on deref semantics, unfortunately.

MarijnS95 · 2024-09-03T16:12:21Z

wgpu-hal/src/dx12/mod.rs

        )
+        .ok()


I should report over at windows-rs that the naming choice of this function is less than ideal. I keep mistaking it for https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.ok which discards Error information by turning Result into Option which instantly triggers during review.

MarijnS95 · 2024-09-03T16:14:04Z

wgpu-hal/src/dx12/shader_compilation.rs

+use crate::auxil::dxgi::result::HResult;
 use std::ffi::CStr;
-use std::ptr;
+use std::path::PathBuf;
+use windows::{
+    core::{Interface, PCSTR, PCWSTR},
+    Win32::Graphics::Direct3D::{Dxc, Fxc},
+};

-pub(super) use dxc::{compile_dxc, get_dxc_container, DxcContainer};
-use windows::Win32::Graphics::Direct3D;


Doesn't wgpu use StdExternalCrate formatting?

It doesn't seem that we do.

MarijnS95 · 2024-09-03T16:16:09Z

wgpu-hal/src/dx12/shader_compilation.rs

+    fn new(lib_path: Option<PathBuf>, lib_name: &'static str) -> Result<Self, libloading::Error> {
+        let lib_path = if let Some(lib_path) = lib_path {
+            if lib_path.is_file() {
+                lib_path
+            } else {
+                lib_path.join(lib_name)
            }
+        } else {
+            PathBuf::from(lib_name)
        };


I wouldn't have copied this weird bit from hassle-rs though, it has caused us quite a few headaches over the years.

I can imagine, but I didn't want to change the behavior (yet) as it is documented in our docs.

MarijnS95 · 2024-09-03T16:17:34Z

wgpu-hal/src/dx12/shader_compilation.rs

+impl OPCWSTR {
+    fn new(s: &str) -> Self {
+        let mut inner: Vec<_> = s.encode_utf16().collect();
+        inner.push(0);
+        Self { inner }
    }


Didn't HSTRING work for you?

It seemed to be even more complex for this purpose (ref-counted, no easy way to create it from a &str, no easy way to get a PCWSTR from it).

MarijnS95 · 2024-09-03T17:52:41Z

Cargo.toml

@@ -149,7 +149,6 @@ gpu-descriptor = "0.3"
 bit-set = "0.8"
 gpu-allocator = { version = "0.27", default-features = false }
 range-alloc = "0.1"
-hassle-rs = "0.11.0"


Yeah the abstraction by hassle-rs isn't that useful. It's mostly creating the bindings but we have those now (except not for Linux 😭) in windows-rs.

MarijnS95 · 2024-09-03T17:56:16Z

wgpu-hal/src/dx12/mod.rs

+        let mut result__ = None;
+
+        (func)(&Direct3D12::ID3D12Debug::IID, <*mut _>::cast(&mut result__))
+            .ok()
+            .into_device_result("GetDebugInterface")?;

-        let mut result__ = core::ptr::null_mut();
-        Ok((func)(&Direct3D12::ID3D12Debug::IID, &mut result__)
-            .and_then(|| unsafe { windows_core::Type::from_abi(result__) }))
+        result__.ok_or(crate::DeviceError::Unexpected)


Right yes, that is unfortunate.

Perhaps this also has to do with SAL annotations, if I remember correctly there are different annotations that allow return values to be NULL, versus them only being NULL when a non-SUCCESS RESULT is returned.

The original code now no longer relies on deref semantics, unfortunately.

MarijnS95 · 2024-09-03T17:59:12Z

wgpu-hal/src/dx12/shader_compilation.rs

+    kind: Dxc::DXC_OUT_KIND,
+) -> Result<T, crate::DeviceError> {
+    let mut result__: Option<T> = None;
+    unsafe { res.GetOutput::<T>(kind, &mut None, <*mut _>::cast(&mut result__)) }


This could coerce because it takes a *mut Option<T>

Suggested change

unsafe { res.GetOutput::<T>(kind, &mut None, <*mut _>::cast(&mut result__)) }

unsafe { res.GetOutput::<T>(kind, &mut None, &mut result__) }

MarijnS95 · 2024-09-03T18:00:48Z

wgpu-hal/src/dx12/shader_compilation.rs

+    res: &Dxc::IDxcResult,
+    kind: Dxc::DXC_OUT_KIND,
+) -> Result<T, crate::DeviceError> {
+    let mut result__: Option<T> = None;


May just be me who prefers this short-hand:

Suggested change

let mut result__: Option<T> = None;

let mut result__ = None::<T>;

Although the type can be inferred:

Suggested change

let mut result__: Option<T> = None;

let mut result__ = None;

Not that it matters too much, this signature will change when upstream metadata fixes land.

MarijnS95 · 2024-09-03T18:03:50Z

wgpu-hal/src/dx12/shader_compilation.rs

+        type Fun = extern "system" fn(
+            rclsid: *const windows_core::GUID,
+            riid: *const windows_core::GUID,
+            ppv: *mut *mut core::ffi::c_void,
+        ) -> windows_core::HRESULT;


Any reason to not use the existing DxcCreateInstanceProc typedef? Because it's wrapped in Option<>?

I saw that it exists but wasn't 100% sure how that would behave with libloading. It's not something I've seen in their docs/examples.

MarijnS95 · 2024-09-03T18:11:19Z

wgpu-hal/src/dx12/shader_compilation.rs

+            .compiler
+            .Compile(&buffer, Some(&compile_args), None)
+    }
+    .into_device_result("Compile")?;


Like the old API, I wonder if Compile returns an error code while IDxcResult is also returned with valid errors (the root cause) in DXC_OUT_ERRORS.

Or if that's still communicated by IDxcOperationResult::GetResult() which you're not checking here?

From my testing, IDxcCompiler3::Compile seems to succeed even if there was a compilation error.

Getting the blob via IDxcResult::GetOutput(DXC_OUT_ERRORS) always succeeds. If there is a compilation error its size/length will be non 0 and will contain the error message.

The wiki seems to say that Compile will only fail if the compiler itself ran into an internal error.
https://github.com/microsoft/DirectXShaderCompiler/wiki/Advanced-Error-Handling#internal-compiler-errors

MarijnS95 · 2024-09-03T18:15:08Z

wgpu-hal/src/dx12/mod.rs

        .ok()
-        // TODO: If there's a HRESULT, error may still be non-null and
-        // contain info.
        .into_device_result("Root signature serialization")?;


I was expecting a similar setup like Compile() the error doesn't need to be OOM.

For Compile(), I'm not sure which HRESULTs to check either. There is:

The result from Compile();

Non-null error blob;

Null blob;

Result from IDxcOperationResult::GetStatus().

(and any getters on IDxcOperationResult/IDxcResult can fail as well)

teoxoy requested a review from a team as a code owner September 2, 2024 17:34

teoxoy force-pushed the d3d12-cleanup branch 4 times, most recently from e736419 to ef0d8c8 Compare September 3, 2024 11:44

nical approved these changes Sep 3, 2024

View reviewed changes

teoxoy force-pushed the d3d12-cleanup branch from ef0d8c8 to 6d9fc2e Compare September 3, 2024 11:53

teoxoy commented Sep 3, 2024

View reviewed changes

wgpu-hal/src/dx12/mod.rs Show resolved Hide resolved

teoxoy mentioned this pull request Sep 3, 2024

test #6203

Closed

teoxoy added 9 commits September 3, 2024 16:55

[d3d12] refactor buffer/texture creation functions to return resource…

74f6798

…s instead of using out params

[d3d12] treat failure to populate references to resources as `DeviceE…

707ba4c

…rror::Unexpected` These are guaranteed to be populated if the creation methods didn't error. If they are not, the driver/runtime is at fault.

move hal_usage_error from the vk backend to lib.rs

fa700ba

[d3d12] make DxgiLib and D3D12Lib methods consistent

3ac1304

[d3d12] simplify create_factory() fn

bc01df0

[d3d12] simplify enumerate_adapters() fn

91e97e9

Also fixes an issue where only high performance adapters would be enumerated if DXGI 1.6 is available.

[d3d12] use windows-rs's bindings for DXC instead of hassle-rs

33defb4

[wgpu-hal] add null terminations to symbols to avoid reallocation

d2706c8

[tests] don't create a new instance/adapter for `DEVICE_LIFETIME_CHEC…

8429f65

…K` & `MULTIPLE_DEVICES` tests The test harness creates the instance and adapter that a test should use. Tests should not create these with default configurations or configs from the environment.

teoxoy force-pushed the d3d12-cleanup branch from 6d9fc2e to 8429f65 Compare September 3, 2024 15:00

teoxoy merged commit 07becfe into gfx-rs:trunk Sep 3, 2024
25 checks passed

teoxoy deleted the d3d12-cleanup branch September 3, 2024 15:15

teoxoy mentioned this pull request Sep 3, 2024

[d3d12] Make sure WARP device creation works properly #6205

Open

MarijnS95 reviewed Sep 3, 2024

View reviewed changes

teoxoy mentioned this pull request Sep 9, 2024

[d3d12] handle known error variants returned by D3D12CreateDevice #6241

Merged

teoxoy mentioned this pull request Dec 2, 2024

Avoid using FXC as fallback #6643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

D3D12 cleanup #6200

D3D12 cleanup #6200

teoxoy commented Sep 2, 2024

nical left a comment

MarijnS95 Sep 3, 2024 •

edited

Loading

teoxoy Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 3, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

teoxoy Sep 4, 2024

MarijnS95 Sep 3, 2024

	unsafe { res.GetOutput::<T>(kind, &mut None, <*mut _>::cast(&mut result__)) }
	unsafe { res.GetOutput::<T>(kind, &mut None, &mut result__) }

	let mut result__: Option<T> = None;
	let mut result__ = None::<T>;

D3D12 cleanup #6200

D3D12 cleanup #6200

Conversation

teoxoy commented Sep 2, 2024

nical left a comment

Choose a reason for hiding this comment

MarijnS95 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 Sep 3, 2024 •

edited

Loading