[wgpu-hal] Migrate d3d12 backend over to `windows-rs` #5956

MarijnS95 · 2024-07-14T21:54:00Z

Caution

This is a draft that is still a work-in-progress! See this as a reminder that work has actively been performed to get the migration done, and to familiarize yourself (as reader) with windows-rs!

I am still heavily self-reviewing, cleaning and refactoring this PR - reviewing will be a waste of time until this is taken out of draft!

Connections
Closes #3207 (once wgl.rs is also over)
Fixes #5813

Description
winapi is barely typed, doesn't care about object lifetime, and is unmaintaned above all. Microsoft officially provides up-to-date and completely typed bindings that make it much more natural, convenient, and safe to interface any API on Windows systems, including DXGI and Direct3D12! By migrating we make it much easier to write, review and maintain the d3d12 backend, and have improved interoperability with support crates like gpu-allocator that already migrated long ago.

One thing you'll also see is the windows crate embracing Result and #[must_use] - there are still a hanful of clippy errors pointing out that the original code forgot to check the returned HRESULT, leading to potential UB.

Testing
Still in progress - a qick showdown was done using:

WGPU_BACKEND=d3d12 cargo run --bin wgpu-examples shadow

Checklist

Run cargo fmt.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

TODO

Clean up unused workspace imports?
Migrate straight to windows 0.67 (with e.g. Change Windows HANDLE types back to *mut c_void **again** 😩? rust-windowing/raw-window-handle#171);
Release gpu-allocator;
Investigate (probably broken) lifetime and ownership of the various enum Surface.. holders...

wgpu-hal/src/dx12/suballocation.rs

wgpu/src/api/surface.rs

Elabajaba · 2024-07-27T02:44:40Z

Tested some of the wgpu examples and it seems to work fine on windows 11 + amd, apart from the todo! panic on dropping window handles (in the impl Drop for SurfaceTarget block).

ErichDonGubler · 2024-07-27T18:42:52Z

Oh, don't you worry about testing. We'll put this through the entire WebGPU CTS on Firefox for you, just to make sure this safe to merge. 🤪

I'm quite excited.

MarijnS95 · 2024-07-29T20:24:27Z

@ErichDonGubler that would be very nice, but not before we answered some basic questions above about the validity of SurfaceTargetUnsafe. This is quite tricky because its values are stored inside Surface, requiring the caller to keep it alive, or us to extend the refcount. I'd love to receive some feedback on how wgpu expects callers to utilize this, and hence what we need to do on the backend.

ErichDonGubler · 2024-08-12T07:36:48Z

@MarijnS95: Ah, of course. The Surface APIs are actually not used by Firefox, but we can still test wgpu-core-centric functionality. I think we can safely decouple Firefox test runs from figuring out questions with those APIs, though OFC merging a PR to mainline history would still be blocked on keeping things safe. 🙂

MarijnS95 · 2024-08-12T08:53:09Z

@ErichDonGubler correct, I don't think we can land this without settling, and documenting it. If my mind serves me correctly the original backend implementation does not assume ownership and does not drop the pointers, which we could replicate by wrapping the COM objects in ManuallyDrop.

cwfitzgerald

This is really amazing stuff! I have a few really small comments, but none that I think should block this landing. Let me make sure moz has everything they need to land this PR, and we can get rolling on this.

wgpu-hal/Cargo.toml

wgpu-hal/src/auxil/dxgi/factory.rs

wgpu-hal/src/dx12/adapter.rs

wgpu-hal/src/dx12/mod.rs

jimblandy

This looks fantastic. Assuming that this depends on windows 0.58, it should be fine for Firefox.

teoxoy

Things look straightforward now, great stuff!

teoxoy · 2024-08-20T12:22:32Z

One thing I noticed is that we still use libloading. Is that necessary given that libloading itself depends on windows_targets which does the linking in the windows crate as well? I think we could instead call the functions exposed by windows.

MarijnS95 · 2024-08-20T13:36:13Z

One thing I noticed is that we still use libloading. Is that necessary given that libloading itself depends on windows_targets which does the linking in the windows crate as well? I think we could instead call the functions exposed by windows.

It should only do the linking when those symbols are actually used/called, which they are currently not. I assumed libloading was preferred since extern functions would have been easy to use with winapi already, but this way makes the relevant DLLs optional to support (much?) older Windows?

If there's no particular reason to use runtime loading I'd be very happy to throw the extra code all right out, but it's a bit of a pity since it has already been written (ported from the linked functions inside the windows crate) in the first place.

cwfitzgerald · 2024-08-20T15:47:27Z

Yeah the idea is to allow running on windows 7-8.1, as the d3d12 functions don't exist there. How valueable that is is debatable, but that's what we've been doing.

MarijnS95

I didn't actually finish self-reviewing my PR. Here are some notes taken while very lightly scrubbing the changes, that I'll perhaps PR changes for later (or someone else can look into them).

wgpu-hal/src/auxil/dxgi/result.rs

wgpu-hal/src/dx12/descriptor.rs

wgpu-hal/src/dx12/shader_compilation.rs

wgpu-hal/src/dx12/command.rs

MarijnS95 · 2024-08-23T22:29:01Z

wgpu-hal/src/dx12/command.rs

    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);


wgpu-hal/src/dx12/command.rs

MarijnS95 · 2024-08-23T22:29:14Z

wgpu-hal/src/dx12/command.rs

+                    // TODO: Empty slice vs None?
+                    unsafe { list.ClearRenderTargetView(*rtv, &value, Some(&[])) };


I feel like None would be more obvious.

Looks like the old clear_render_target_view() in d3d12/src/command_list.rs used to map an empty slice to ptr::null(), so I'm changing it to None too. If anything this is one of the few strange semantic differences to perhaps have an effect on #6162?

wgpu-hal/src/dx12/mod.rs

teoxoy · 2024-08-27T17:43:21Z

@MarijnS95 thanks for being so thorough!

teoxoy · 2024-08-27T17:44:41Z

I left some 👍 on the things I think would be good to address in a follow-up; if you'd be up for that :)

MarijnS95 · 2024-08-27T19:57:17Z

wgpu-hal/src/dx12/command.rs

+                            flags,
+                            ds.clear_value.0,
+                            ds.clear_value.1 as u8,
+                            &[],


This one used to be ptr::null() too, but there's no None mapping in Direct3D12 here because someone only reported this issue (and hence fixed it) for clearRenderTargetView() 🤦🤦.

microsoft/win32metadata#1971

MarijnS95 · 2024-08-27T19:59:42Z

Thanks @teoxoy: I've prepared a branch locally to batch some of these together, and will open a PR soon.

PR gfx-rs#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.

…eanups

PR gfx-rs#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.

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.

MarijnS95 mentioned this pull request Jul 14, 2024

Migrate to Windows-rs from winapi #3207

Closed

MarijnS95 force-pushed the windows-rs branch from cf9033f to 9753c1f Compare July 14, 2024 21:55

MarijnS95 mentioned this pull request Jul 15, 2024

Switch winapi dependencies to windows/windows-sys servo/servo#30640

Open

MarijnS95 force-pushed the windows-rs branch 2 times, most recently from 05cb537 to b6c5854 Compare July 21, 2024 18:53

MarijnS95 commented Jul 21, 2024

View reviewed changes

wgpu-hal/src/dx12/suballocation.rs Outdated Show resolved Hide resolved

MarijnS95 commented Jul 21, 2024

View reviewed changes

wgpu/src/api/surface.rs Outdated Show resolved Hide resolved

MarijnS95 force-pushed the windows-rs branch from b6c5854 to 75abb72 Compare July 21, 2024 19:20

MarijnS95 mentioned this pull request Jul 21, 2024

[wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs #6006

Merged

6 tasks

MarijnS95 force-pushed the windows-rs branch from 75abb72 to 2f119a0 Compare July 21, 2024 20:25

MarijnS95 marked this pull request as ready for review July 21, 2024 20:25

MarijnS95 requested a review from a team as a code owner July 21, 2024 20:25

MarijnS95 force-pushed the windows-rs branch 5 times, most recently from 94cce3b to 662998b Compare July 25, 2024 19:47

MarijnS95 force-pushed the windows-rs branch from 4bb47c3 to 383881c Compare July 29, 2024 20:18

MarijnS95 force-pushed the windows-rs branch from 383881c to 5a20bd8 Compare August 14, 2024 20:50

cwfitzgerald approved these changes Aug 16, 2024

View reviewed changes

wgpu-hal/Cargo.toml Show resolved Hide resolved

wgpu-hal/src/auxil/dxgi/factory.rs Outdated Show resolved Hide resolved

wgpu-hal/src/dx12/adapter.rs Outdated Show resolved Hide resolved

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

MarijnS95 force-pushed the windows-rs branch from 5a20bd8 to 6c73e58 Compare August 19, 2024 21:25

MarijnS95 added 3 commits August 19, 2024 23:30

windows-rs 0.58

ff118b4

Clean up suspicious committed allocation workaround

4a7977c

dx12: Flatten suballocation module

74c2767

MarijnS95 force-pushed the windows-rs branch from 6c73e58 to 74c2767 Compare August 19, 2024 21:31

jimblandy approved these changes Aug 20, 2024

View reviewed changes

teoxoy approved these changes Aug 20, 2024

View reviewed changes

cwfitzgerald merged commit a157c3c into gfx-rs:trunk Aug 20, 2024
25 checks passed

MarijnS95 deleted the windows-rs branch August 21, 2024 10:06

MarijnS95 commented Aug 23, 2024

View reviewed changes

LastExceed mentioned this pull request Aug 24, 2024

The repository URL of the d3d12 crate on crates.io is 404 #6154

Closed

xiaopengli89 mentioned this pull request Aug 26, 2024

DX12 backend no longer works after #5956 #6162

Closed

MarijnS95 commented Aug 27, 2024

View reviewed changes

MarijnS95 mentioned this pull request Aug 27, 2024

[wgpu-hal] #5956 windows-rs migration followups and cleanups #6173

Merged

6 tasks

ErichDonGubler added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024

fixup! [wgpu-hal] gfx-rs#5956 windows-rs migration followups and cl…

4da2ca9

…eanups

teoxoy mentioned this pull request Sep 2, 2024

D3D12 cleanup #6200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wgpu-hal] Migrate d3d12 backend over to `windows-rs` #5956

[wgpu-hal] Migrate d3d12 backend over to `windows-rs` #5956

MarijnS95 commented Jul 14, 2024 •

edited

Loading

Elabajaba commented Jul 27, 2024

ErichDonGubler commented Jul 27, 2024

MarijnS95 commented Jul 29, 2024

ErichDonGubler commented Aug 12, 2024

MarijnS95 commented Aug 12, 2024

cwfitzgerald left a comment

jimblandy left a comment

teoxoy left a comment

teoxoy commented Aug 20, 2024

MarijnS95 commented Aug 20, 2024

cwfitzgerald commented Aug 20, 2024

MarijnS95 left a comment

MarijnS95 Aug 23, 2024

MarijnS95 Aug 23, 2024

teoxoy Aug 27, 2024

MarijnS95 Aug 27, 2024

teoxoy commented Aug 27, 2024

teoxoy commented Aug 27, 2024

MarijnS95 Aug 27, 2024

MarijnS95 commented Aug 27, 2024 •

edited

Loading

		// TODO: Empty slice vs None?
		unsafe { list.ClearRenderTargetView(*rtv, &value, Some(&[])) };

[wgpu-hal] Migrate d3d12 backend over to windows-rs #5956

[wgpu-hal] Migrate d3d12 backend over to windows-rs #5956

Conversation

MarijnS95 commented Jul 14, 2024 • edited Loading

Elabajaba commented Jul 27, 2024

ErichDonGubler commented Jul 27, 2024

MarijnS95 commented Jul 29, 2024

ErichDonGubler commented Aug 12, 2024

MarijnS95 commented Aug 12, 2024

cwfitzgerald left a comment

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

teoxoy left a comment

Choose a reason for hiding this comment

teoxoy commented Aug 20, 2024

MarijnS95 commented Aug 20, 2024

cwfitzgerald commented Aug 20, 2024

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 Aug 23, 2024

Choose a reason for hiding this comment

MarijnS95 Aug 23, 2024

Choose a reason for hiding this comment

teoxoy Aug 27, 2024

Choose a reason for hiding this comment

MarijnS95 Aug 27, 2024

Choose a reason for hiding this comment

teoxoy commented Aug 27, 2024

teoxoy commented Aug 27, 2024

MarijnS95 Aug 27, 2024

Choose a reason for hiding this comment

MarijnS95 commented Aug 27, 2024 • edited Loading

[wgpu-hal] Migrate d3d12 backend over to `windows-rs` #5956

[wgpu-hal] Migrate d3d12 backend over to `windows-rs` #5956

MarijnS95 commented Jul 14, 2024 •

edited

Loading

MarijnS95 commented Aug 27, 2024 •

edited

Loading