这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@Kangz
Copy link
Contributor

@Kangz Kangz commented May 22, 2019

PTAL!

spec/index.bs Outdated
"sampled-texture",
"storage-buffer",
"dynamic-storage-buffer"
"dynamic-storage-buffer",
Copy link
Contributor

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

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 is the same problem as discussed in #256: uniform buffers are read-only but use a different HW path compared to readonly storage buffers.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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",
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Kangz Kangz force-pushed the kangz-readonly-ssbo branch from dddea53 to 85d2038 Compare May 27, 2019 14:28
required u32 binding;
required GPUShaderStageFlags visibility;
required GPUBindingType type;
bool dynamic = false;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

@Kangz Kangz force-pushed the kangz-readonly-ssbo branch from 85d2038 to ce692ea Compare June 5, 2019 09:59
@Kangz Kangz force-pushed the kangz-readonly-ssbo branch from ce692ea to 7c51c20 Compare June 18, 2019 08:45
@Kangz
Copy link
Contributor Author

Kangz commented Jun 18, 2019

Merging as looking in the meeting notes this PR was agreed to with clarification for the validation.

@Kangz Kangz merged commit f3048e7 into master Jun 18, 2019
@beaufortfrancois beaufortfrancois deleted the kangz-readonly-ssbo branch June 18, 2019 11:20
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
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.

6 participants