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

Dynamic buffer descriptors #1949

Closed
msiglreith opened this issue Apr 18, 2018 · 3 comments · Fixed by #3393
Closed

Dynamic buffer descriptors #1949

msiglreith opened this issue Apr 18, 2018 · 3 comments · Fixed by #3393

Comments

@msiglreith
Copy link
Contributor

msiglreith commented Apr 18, 2018

Add support for dynamic uniform buffers and dynamic storage buffers.

Implementation:

  • Vulkan: obvious
  • DX12: Descriptor + Root constant for offset maybe? Separate root constant space for all dynamic descriptor offset and updated on BindDescriptorSet. Further limits the available place in the root signature (32 root constants, 8 descriptor sets (2 slots due to sampler separation), 8 + 4 root constants for dynamic, 3 root constants for workgroup size, 1 spare for now). Other ideas?
  • Metal: No idea

Edit: Update dx12 root signature split up

@kvark kvark added the client: blocker blocker issue for a known client label Jun 4, 2018
@msiglreith msiglreith mentioned this issue Jun 5, 2018
3 tasks
bors bot added a commit that referenced this issue Jun 5, 2018
2111: Dynamic buffers r=kvark a=msiglreith

Start defining the basic API for dynamic buffer descriptors. Atm based upon vulkan, not optimal but provides the best performance. Other possbiel API design could use `(set, Option<Offset>)` for descriptor set binding, but involves more work on the portability layer and might be more typing for users.

Feedback appreciated.
More backends to follow ..

Addresses #1949
PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: msiglreith <[email protected]>
Co-authored-by: Dzmitry Malyshau <[email protected]>
@kvark kvark removed the api: hal label Jun 26, 2019
@kvark
Copy link
Member

kvark commented Sep 30, 2019

The current status is - the feature is implemented and working well on Vulkan and Metal.
DX12/11 needs this covered as well, as it's blocking rx and potentially Amethyst from using the backend. See code.

@kvark kvark pinned this issue Sep 30, 2019
@msiglreith
Copy link
Contributor Author

Note: Using root descriptors for implementing it in DX12 should be the easier approach compared to the one outlined. It should occupy the same space in the root signature.

@msiglreith
Copy link
Contributor Author

With the implementation of uniform buffers as root descriptors we end up with a root signature layout (64slots):

  • 32 root constants -> 128bytes (Vulkan minimum)
  • 8 descriptor tables for 4 descriptor sets (CBV/SRV/UAV and Sampler tables split)
  • 8 root descriptors for dynamic uniform buffers -> 2x8 slots (Vulkan Minimum)

This leafs us with 8 slots for dynamic storage buffers, #2604 and #2589

Root descriptors for SRV and UAV are different which would require us to know which resource type is used on the shader side. We don't have this information in the host side API (see #2933).

In case we assume 4 slots for the #2604 and #2589, we end up with 4 slots -> 2 root descriptors for storage buffers. As we would have to bind both SRV and UAV if we follow the approach used for descriptor tables, it would end up with 1 storage buffer max which would not fit the vulkan minimum.

As storage buffers are translated to the R/WByteAddressBuffers, we actually can make use of the descriptor table + push constant approach proposed initially. Therefore we only need one push constant for each storage buffer and we would fit in the limit. This requires a few changes to spirv-cross tho to properly offset the address buffers with the push constant value, along with injecting these in the shaders.

bors bot added a commit that referenced this issue Oct 4, 2019
3033: dx12: Implement dynamic uniform buffers r=kvark a=msiglreith

Part of #1949

Implement dynamic uniform buffers via root descriptors (2 slots in the root signature per descriptor).
This approach is suitable for uniform buffers as these are translated to typed constant buffers on the shader side.

Storage buffers need to be implemented another way as we would oversubscribe the root signature and we need to deal with read-only descriptors in a way, which is not feasible with the root descriptor approach.

Tested with an modified quad example, executing two draw calls with a different offset.

TODO: d3d12-rs changes pending

Co-authored-by: msiglreith <[email protected]>
@kvark kvark unpinned this issue Oct 21, 2019
@bors bors bot closed this as completed in #3393 Oct 6, 2020
@bors bors bot closed this as completed in ee90ac1 Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants