Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jul 14, 2024

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

wgpu/src/api/surface.rs Outdated Show resolved Hide resolved
@Elabajaba
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

@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.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look straightforward now, great stuff!

@teoxoy
Copy link
Member

teoxoy commented Aug 20, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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.

@cwfitzgerald cwfitzgerald merged commit a157c3c into gfx-rs:trunk Aug 20, 2024
25 checks passed
@MarijnS95 MarijnS95 deleted the windows-rs branch August 21, 2024 10:06
Copy link
Contributor Author

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
wgpu-hal/src/auxil/dxgi/result.rs Show resolved Hide resolved
wgpu-hal/src/dx12/descriptor.rs Show resolved Hide resolved
wgpu-hal/src/dx12/descriptor.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu-hal/src/dx12/command.rs Show resolved Hide resolved
Comment on lines 55 to 59
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

wgpu-hal/src/dx12/command.rs Show resolved Hide resolved
Comment on lines +727 to +728
// TODO: Empty slice vs None?
unsafe { list.ClearRenderTargetView(*rtv, &value, Some(&[])) };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like None would be more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Aug 27, 2024

@MarijnS95 thanks for being so thorough!

@teoxoy
Copy link
Member

teoxoy commented Aug 27, 2024

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

flags,
ds.clear_value.0,
ds.clear_value.1 as u8,
&[],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

MarijnS95 commented Aug 27, 2024

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

MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 27, 2024
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.
ErichDonGubler added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024
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.
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024
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.
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024
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.
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 29, 2024
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.
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 29, 2024
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.
MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 29, 2024
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.
teoxoy pushed a commit that referenced this pull request Aug 30, 2024
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.
@teoxoy teoxoy mentioned this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ComPtr API is unsound. Migrate to Windows-rs from winapi
7 participants