Skip to content
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

Figure out the base vertex input limits #693

Closed
kvark opened this issue Apr 8, 2020 · 15 comments · Fixed by #1274
Closed

Figure out the base vertex input limits #693

kvark opened this issue Apr 8, 2020 · 15 comments · Fixed by #1274
Labels
Projects

Comments

@kvark
Copy link
Contributor

kvark commented Apr 8, 2020

We've been asked to clarify the baseline for:

  • max number of vertex buffers
  • max number of vertex attributes
  • max attribute offset
  • max stride
@kvark
Copy link
Contributor Author

kvark commented Apr 8, 2020

For the vertex attributes, it looks like it would be safe to assume 16 based on the native limits:

  • Vulkan
  • D3D12 ("For more info, see Input Slots. Valid values are between 0 and 15.")
  • Metal ("Maximum number of entries in the buffer argument table" = 31).

The caveat here is that Metal's table will be shared between vertex buffers, argument buffers, individual buffer descriptors, and any internal buffers we have to provide.

@kvark
Copy link
Contributor Author

kvark commented Apr 8, 2020

So here is an unfortunate but potentially valid scenario (numbers taken from https://gpuweb.github.io/gpuweb/#gpulimits).

Implementation on Metal has to support at least 4 bind groups. Each will take a slot in the vertex buffer table as an argument buffer, assuming they are supported. If they aren't... this gets much worse.

It has to support at least 8 dynamic uniform buffers and 4 dynamic storage buffers. How are the offsets passed? We were planning on just excluding the dynamic resources from the argument buffer and binding them separately. That would mean at least 12 more bindings taken.

Internally, it may need to bind a buffer with some extra data, say containing the size limits for out-of-bounds checking in the shader. That's one more buffer.

So what we end up is 17 buffers taken by non-vertex-attributes, and only 14 left for the vertex buffers for attributes, at most. Which means, an example implementation can't expose the base limit of 16 that is suggested above. A more complicated implementation could possibly put all the internal things together with dynamic offsets in a single buffer, but the feasibility of this path is yet to be considered in depth (and definitely needs to be discussed).

Another problem is that, if an implementation wants to report a higher limit for any of these, it can't do so for all the limits. I.e. supposing it has an extra buffer. It could either report it as a max bind groups limit of 5, or as a max dynamic storage buffer limit as 5, but not both, and hence - will not be able to run applications that it should be able to run.

I wonder if we can look at it together with #684 and say that:

an implementation may fail to request a device with multiple limits that are above the baseline

That would be totally matching this case here with vertex attributes, and it would discourage users to just blindly request all the limits.

@Kangz
Copy link
Contributor

Kangz commented Apr 8, 2020

For the maximum stride we have found that 2048 is a good value and required at least by Vulkan. For attribute offsets, Metal requires they are aligned to 4, and that they stay entirely within a stride.

The limited amount of vertex buffers on Metal is a bit of an issue, but maybe this could be worked around by only allowing 14 or even a bit less vertex buffers? I'm not sure I see a usecase where there are more than a handful of vertex buffers.

@kvark
Copy link
Contributor Author

kvark commented Apr 8, 2020

Yes, setting the base line to 14 would have worked, assuming a Metal implementation would report all the other limits to be the baseline as well, which could certainly be better. I.e. why would an app using 4 vertex buffers have to be limited in 4 bind groups?

Some context about the use cases that need many vertex buffers (namely, glTF import) - gfx-rs/wgpu#558 (comment) .

@kainino0x
Copy link
Contributor

From the editors meeting: we propose (IIRC):

  • a minimum limit on vertex buffers less than 14 (was it 8?)
  • postpone trying to expose limits that would exceed the Metal limits when summed together
  • consider adding a maxBuffersPlusVertexBuffersForVertexStage limit later as needed

@kainino0x kainino0x added this to Needs Discussion in Main Apr 14, 2020
@kainino0x kainino0x moved this from Needs Discussion to Needs Investigation/Proposal or Revision in Main Jul 17, 2020
@kainino0x kainino0x moved this from Needs Investigation/Proposal or Revision to Needs Discussion in Main Jul 17, 2020
@kainino0x
Copy link
Contributor

Resolved: take that proposal

@kainino0x kainino0x moved this from Needs Discussion to Needs Specification in Main Jul 20, 2020
@Kangz
Copy link
Contributor

Kangz commented Jul 20, 2020

(with the limit being 8)

@jespertheend
Copy link
Contributor

Would it be possible to have these limits added to GPULimits? So that these limits could be higher on platforms that support this. Or was that the idea already?

@kvark
Copy link
Contributor Author

kvark commented Dec 1, 2020

@jespertheend yes, we need to add these to GPU limits, whoever got there first. There is an expectation that adding the limits comes with the validation statements about respecting them. If you want to help with a PR, please go for it!

@jespertheend
Copy link
Contributor

@kvark alright I’ll try to have a go at it tomorrow. I have never done a PR for a spec before so bear with me.

@kainino0x
Copy link
Contributor

FYI there's a "Limits.md" where we document where the minimums come from; we should add this to that doc (and link to this issue).

@jespertheend
Copy link
Contributor

Has a limit for the max attribute offset been chosen yet or should I skip that one for now?

@kvark
Copy link
Contributor Author

kvark commented Dec 3, 2020

I think max attribute offset is not a separate limit, it can be derived from maximum stride (which is 2048)

@jespertheend
Copy link
Contributor

Ah I think that's step 3 at https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-gpuvertexbufferlayoutdescriptor then

@jespertheend
Copy link
Contributor

Alright I've added #1274, I'm not sure if the limit names are descriptive enough.
Let me know if I missed something.

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this issue Sep 6, 2022
Fixes a bunch of tests broken in gpuweb#616.

- now correctly copy 0 bytes instead of attempting to copy all remaining elements in the source array.
- fix incorrect assertion
- typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Main
Specification Done
Development

Successfully merging a pull request may close this issue.

4 participants