-
Notifications
You must be signed in to change notification settings - Fork 967
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
fix(limits): properly calculate max_bindings_per_bind_group
#3940
fix(limits): properly calculate max_bindings_per_bind_group
#3940
Conversation
3210f23
to
d2fd46f
Compare
This PR is basically ready to go, minus a |
wgpu-hal/src/gles/adapter.rs
Outdated
let max_bindings_per_bind_group = (max_sampled_textures_per_shader_stage | ||
+ max_samplers_per_shader_stage | ||
+ max_storage_buffers_per_shader_stage | ||
+ max_storage_textures_per_shader_stage | ||
+ max_uniform_buffers_per_shader_stage) | ||
* crate::auxil::MAX_SHADER_STAGES_PER_PIPELINE; | ||
|
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.
suggestion: This calculation is currently duplicated across all back ends. We should make this shared code, too.
wgpu-hal/src/dx11/adapter.rs
Outdated
+ max_storage_buffers_per_shader_stage | ||
+ max_storage_textures_per_shader_stage | ||
+ max_uniform_buffers_per_shader_stage) | ||
* MAX_SHADER_STAGES_PER_PIPELINE; |
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 don't think this is correct. maxBindingsPerBindGroup
's purpose is to limit the maximum index used for bind group bindings, because in some implementations (wgpu with the vulkan backend for example) binding descriptors end up being allocated in a dense array, the size of which is the biggest index in the group, and with some vulkan drivers that was causing some stability/safety issues with large indices.
The limit was first introduced in the spec to be 640, and it has apparently been changed to 1000 since.
So the fix should involve updating the limit to 1000 and making it work consistently over the various backends.
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.
Hmm...sounds like we need to talk about it. @nical and I resolved in Firefox team chat to discuss with the WebGPU team. I'll set this PR back as draft for now.
Status: currently blocked on pending discussion with the WebGPU standards body (see #3940 (comment)). |
No need to wait, this is superseded by #3942. There is a bit of confusion in the spec around that formula but it is the spec that needs change/clarification in this case, not wgpu. |
224348c
to
5a27bfd
Compare
I agree that this PR cannot progress for now. I don't agree that it should be closed right now, given that we still need to discuss your conclusion with the wider team and the standards committee first. |
5a27bfd
to
a1db782
Compare
a1db782
to
1f7d343
Compare
I'm going to close this for triage purposes as I don't really want unmergable PRs hanging out - please feel free to keep the branch alive, and re-open once a decision has been made upstream. |
Depends on #3942.
Checklist
cargo clippy
.Runcargo clippy --target wasm32-unknown-unknown
if applicable.Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories
Currently tracked in the Mozilla bug tracker at bug 1844230.
Description
Describe what problem this is solving, and how it's solved.
AdapterInfo::max_bindings_per_bind_group
is incorrectly calculated, according to the WebGPU spec.:Testing
Explain how this change is tested.
Tested by experimentally consuming this fix in Firefox, and demonstrating that the
webgpu:api,operation,adapter,requestDevice:limit,worse_than_default:limit="maxBindingsPerBindGroup"
case works correctly (TODO: link).