-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[dx12] Don't panic if MaxSupportedFeatureLevel wasn't set by query
#8806
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
Conversation
ErichDonGubler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor changes requested. Otherwise, thanks for taking this on! 🙂
| Direct3D::D3D_FEATURE_LEVEL_12_0 => FeatureLevel::_12_0, | ||
| Direct3D::D3D_FEATURE_LEVEL_12_1 => FeatureLevel::_12_1, | ||
| Direct3D::D3D_FEATURE_LEVEL_12_2 => FeatureLevel::_12_2, | ||
| Direct3D::D3D_FEATURE_LEVEL(0) => return None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue(non-blocking): I suspect we'll also want to report this via telemetry. That sound interesting, @teoxoy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this will be worth reporting given my comment below. Also completely out of my domain of expertise lol. Seems to be an issue with some windows versions that is easily bypassed.
I'll leave this unresolved for visibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have telemetry for this since it should count towards the "total nr of people without D3D12" counter. I can also open another PR for it.
MaxSupportedFeatureLevel wasn't set by query
|
In the OP:
This is an issue for everyone trying to use wgpu on native, Mozilla is just the first to discover it. 😉 |
We don't have 100% certainty that the case we're trying to handle is the actual behavior causing crashes here (viz., we're not sure that |
|
@ErichDonGubler I asked in the directx discord. I'm told that what we're seeing "shouldnt be possible" but also isn't a driver bug; the person is responding with single sentences and I can't figure out what they mean. Once I get feedback I'll also document that here in a comment. |
|
Inlining my notes from bug 2007754, comment 7:
ETA: For reference, that's this commit stack: https://github.com/gfx-rs/wgpu/compare/aba9161b72c028aa8a1ce15aabd92e3c3cdb2da3..6fe22a0b59f256648eedd7652247c23490162d1a |
@ErichDonGubler So, were you able to get a crash locally with the test case in #8803? Does this PR fix that crash? |
|
Also, I will update this with information from the DX12 discord server: This is a bug in Microsoft's DirectX code on older Windows versions. The fix is just as I did it here, and some sample code shared by several users had basically identical code to what we would have after this lands. I'd still like confirmation that this actually fixes the bug but I'd be very surprised if it didn't. |
Co-authored-by: Erich Gubler <erichdongubler@gmail.com>
Thanks for tracking this down! |
|
Actually, does the 0 mean that the device supports FL11 or does it mean the device doesn't support D3D12 at all? I'm asking because creating the D3D12 succeeded (which has a minimum FL requirement of 11) and the old code (before #8576) didn't run into issues. |
|
Well we set the value to zero before calling the DirectX function, so I'm actually not sure if a value of zero indicates nothing being written or a zero being written by the driver. It shouldn't really matter though. I'm pretty sure we should treat DX12 as being unsupported if it returns zero. It would return one of the feature levels that we pass it if it actually supported one of them. This is what the code from other projects that I've seen does, just throws away those devices. I'm honestly a little surprised that the old code didn't run into issues, but I don't think that means we should assume it supports a feature level that it explicitly doesn't claim to. |
|
I looked at Firefox's crash stats and if you aggregate them by "platform version" here, there are crashes even on 10.0.26200 (Windows 11 25H2). There is also another issue (crash stats) introduced by #8576 where we now always wgpu/wgpu-hal/src/dx12/adapter.rs Line 74 in bf075e7
This seemed fine to do since |
|
I'll put up a PR solving these 2 issues and adding telemetry as well. |
Connections
Closes #8803
Description
Fixes issue with device not supporting our required D3D12 feature level, and us entering unreachable code. This is an issue for mozilla.
Testing
Hasn't been, @ErichDonGubler can you verify that this should fix it?
Squash or Rebase?
Squash due to only commit being named stupid
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.