fix(limits): properly calculate max_bindings_per_bind_group
#3940
Conversation
ErichDonGubler
added
type: bug
ErichDonGubler
force-pushed
the
fix-max_bindings_per_bind_group
branch
4 times, most recently
from
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.
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.
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.
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. |
ErichDonGubler
force-pushed
the
fix-max_bindings_per_bind_group
branch
4 times, most recently
from
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. |
ErichDonGubler
force-pushed
the
fix-max_bindings_per_bind_group
branch
from
5a27bfd
to
a1db782
Compare
ErichDonGubler
force-pushed
the
fix-max_bindings_per_bind_group
branch
from
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-unknownif 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_groupis 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).