-
Notifications
You must be signed in to change notification settings - Fork 345
Add read-only storage buffer bindings #303
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
spec/index.bs
Outdated
| "sampled-texture", | ||
| "storage-buffer", | ||
| "dynamic-storage-buffer" | ||
| "dynamic-storage-buffer", |
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.
Perhaps we could find a better single-word alternative to "readonly-storage"? For example, "read-buffer" and possibly the corresponding "read-texture" instead of "sampled-texture".
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.
This is the same problem as discussed in #256: uniform buffers are read-only but use a different HW path compared to readonly storage buffers.
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'm not suggesting to unify "uniform" and "read-storage" usages, here I'm only talking about the name for the latter.
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.
So uniform-buffer and read-buffer? It seems more confusing imho because it isn't clear for experienced GPU developers what the difference is between the two. Maybe read-storage-buffer?
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 just feel like "read-storage-buffer" is both unnecessary long and inconsistent with the texture one ("sampled-texture"). Experienced GPU developers are likely to recognize "uniform" and be able to distinguish it from other usages.
spec/index.bs
Outdated
| "dynamic-storage-buffer" | ||
| "dynamic-storage-buffer", | ||
| "readonly-storage-buffer", | ||
| "dynamic-readonly-storage-buffer", |
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 make sense to move the "dynamic" flag as another boolean field near the GPUBindingType instead of combinatorically exploding the binding types (think about what happens if/when we add texel buffer views...)
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.
Done.
dddea53 to
85d2038
Compare
| required u32 binding; | ||
| required GPUShaderStageFlags visibility; | ||
| required GPUBindingType type; | ||
| bool dynamic = false; |
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.
The title of the change does not mention anything about this flag. What does 'dynamic' mean in this context? Please add comments in the IDL or a section in the document that explains.
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.
Done. This was changed to address @kvark's concern that we have a combinatorial explosion of binding types.
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.
Thank you for the clarification, @Kangz . Why does the implementation need to know whether a binding is dynamic or not? Will it be a validation error if you set "dynamic = false" and use a dynamic offset anyways?
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.
Implementation needs to know if it expects that extra offset at the binding time. For example, in Vulkan it would use a different VkDescriptorType. The validation would need to make sure that exactly as many offsets are provided as there are dynamic buffers in the bind group.
85d2038 to
ce692ea
Compare
ce692ea to
7c51c20
Compare
|
Merging as looking in the meeting notes this PR was agreed to with clarification for the validation. |
PTAL!