Sitelet https://github.com/gpuweb/gpuweb/pull/93
Skip to content

Fixed pipeline layout in compute/render passes - #93

Closed
kvark wants to merge 3 commits into
gpuweb:masterfrom
kvark:layout
Closed

kvark wants to merge 3 commits into
gpuweb:masterfrom
kvark:layout

Conversation

@kvark

@kvark kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

This PR includes #92, which includes #91 (we need to go deeper!), so only the last commit needs to be taken into consideration. It's sort of a proposal/RFC, open for discussion.

Basically, since our render/compute passes encapsulate the resource binding scope, it seems natural to require all those resources to belong to the same pipeline layout. Changing the pipeline layout in practice in low-level APIs like D3D12/Vulkan requires re-binding of some of the resources, based on the layout compatibility rules, and is not generally something that is done often. Chances are that if you are going to use a completely different pipeline layout, you'd also render into different render targets, so this is going to be happening in a different pass.

An alternative would be to allow (automatic?) pipeline layout changes through a pass. That would require us to document precisely when the change is possible, how it is achieved, and likely also to provide the pipeline layout argument for binding the resource groups.

@kvark
kvark requested a review from Kangz October 3, 2018 01:27
@magcius

magcius commented Oct 3, 2018

Copy link
Copy Markdown

Changing the pipeline layout in practice in low-level APIs like D3D12/Vulkan requires re-binding of some of the resources, based on the layout compatibility rules, and is not generally something that is done often. Chances are that if you are going to use a completely different pipeline layout, you'd also render into different render targets, so this is going to be happening in a different pass.

Hm, are you sure? I would think switching between different "kinds" of object renders (e.g. particles and meshes) would have different binding group layouts, but both would want to render to the same render target. Are there any low-level APIs that require the same binding group in a pass?

@Kangz

Kangz commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

What @magcius said: the idea with having multiple bind group layouts is that you could have pipeline layouts have the following shape:

  • Bind group layout 0: frame-constant data (skybox, camera uniforms, ...)
  • Bind group layout 1: Model constant data (textures, skinning params, samplers)
  • Bind group layout 2: per instance data (uniforms, maybe additional textures?)

The point is that bind group layout 0 would always be the same during a pass, but that bind group layout 1 and 2 could easily depend on what is done, like particles or meshes (like what @magcius said) and fixing the whole pipeline layout during the pass would be too constraining.

What prompted you to do this change? Does it make the implementation easier in some way?

bors Bot added a commit to gfx-rs/wgpu that referenced this pull request Oct 3, 2018
18: Compute resource binding r=grovesNL a=kvark

Again, this isn't complete/usable yet, blocked by gpuweb/gpuweb#93
But we can merge and move forward.

Co-authored-by: Dzmitry Malyshau <kvark@mozilla.com>
@kvark

kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

@Kangz @magcius as I said, this is just an opening to discussion. I'm not yet convinced that we need to fix the layout per pass, but I'm going to argue for that point in the name of a healthy debate.

What prompted you to do this change? Does it make the implementation easier in some way?

Having the pipeline layout fixed makes a clear context for the resource binding in a pass: each bind group is expected to be of a particular layout, so whenever a group is bound, we can instantly check that it fits. If the pipeline layout is changing, then we'll need to add some new rules. e.g.:

  • when setPipeline is called, and the pipeline is of a different layout, all the resource bindings are reset
  • when setBindGroup is called, it is expected to match the pipeline layout of the last bound pipeline

These aren't most elegant rules to put in place, and I could see them prone to accidental user errors.

the idea with having multiple bind group layouts

Users can just have different bind group layouts reserved for different things inside the same pipeline layout:

  • 0: frame-constant data (skybox, camera uniforms, ...)
  • 1: textures data
  • 2: skinning data
  • 3: something else
  • 4: per-instance-type-A data
  • 5: etc

Then a particular pipeline isn't going to use all of them, and neither it has to.

It would certainly be helpful to check with ISVs on how their render passes and pipeline layouts relate. IIRC, some of the apps I saw use a single pipeline layout for everything.

@kvark

kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

@magcius

Are there any low-level APIs that require the same binding group in a pass?

Vulkan doesn't scope resource bindings to a pass, D3D12 doesn't have passes, and Metal doesn't have bind groups. So there isn't a clear precedent in the native APIs to follow.

switching between different "kinds" of object renders (e.g. particles and meshes) would have different binding group layouts

To repeat what I said above in different words, different objects are perfectly allowed to use different bind group layouts, they just don't necessarily have to be occupying conflicting slots in the pipeline layout.

@kvark

kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

Speaking of ISVs, maybe @zeux could provide a data point? (thanks in advance!)

@magcius

magcius commented Oct 3, 2018 •

Copy link
Copy Markdown

That kind of refactor (moving to unique BindGroup indexes for the whole render pass) can be tricky for an engine not designed for it. They also might have to change shaders to point to a different BindGroup index. It seems overkill for a minor implementation cost and API design cleanup.

@Kangz

Kangz commented Oct 3, 2018 •

Copy link
Copy Markdown
Contributor

@kvark well understood this is exploratory, thanks for putting this up. I think the validation rules for bind groups can be made more user friendly in the case we don't fix them per pass if we have the following validation / state tracking rules:

  • Passes start with no bind group bound
  • When pipelines are changed, bind groups are kept using the vulkan descriptor set inheritance rules
  • On draw/dispatch calls, the bind group layouts set in the current pipeline layout must match the currently bound bind groups at their respective location.

This is basically vulkan inheritance rules + lazy validation that bind group layouts match on draw calls / dispatches.

I should add that we implemented this in Dawn and the validation is looks very simple and can be cached super efficiently. We do have deduplication of bind group layouts though so that comparing them by pointers is the same as comparing them by value.

@zeux

zeux commented Oct 3, 2018

Copy link
Copy Markdown

We definitely use more than one pipeline layout per render and compute pass. It doesn't seem that restricting to just one is meaningful.

For example, we have shaders that have just one UBO that they read from, and we have shaders that have 4 - they render in the same pass (geometry pass). We use dynamic UBO offsets; this means that on AMD hardware, we are consuming user data registers proportionally to the number of UBOs we use. Forcing all shaders to use a pipeline with 4 may penalize the shaders that only use 1.

For textures, while texture descriptors wouldn't go to user data, we need to update the descriptors when textures change; having to fill in dummy descriptors would also not be optimal (Vulkan doesn't force you to fill statically unused descriptors but gpuweb might be different?)

Obviously this means that we need to switch the layouts in some cases - but the cost of the switch is generally speaking mostly CPU side and not that substantial (there might be a small GPU impact in the command processor). Compared to that, the cost of, say, descriptors no longer fitting into user data is large - driver will spill the user data registers to memory and GPU will have to pay an extra indirection cost. While there may be some renderers that use worst-case pipeline layouts, this is really an anti-pattern in many cases.

@kvark

kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

@magcius

They also might have to change shaders to point to a different BindGroup index

Either that, or just just end a pass and start a new one with a different pipeline layout.

@zeux thanks for quick feedback!

We use dynamic UBO offsets

we don't have those yet :)

this means that on AMD hardware, we are consuming user data registers proportionally to the number of UBOs we use. Forcing all shaders to use a pipeline with 4 may penalize the shaders that only use 1.

You don't need to have the bindings to 4 dynamic UBOs in shaders that don't use them, so I don't see why using the same pipeline layout would penalize those.

(Vulkan doesn't force you to fill statically unused descriptors but gpuweb might be different?)

With a fixed pipeline layout, it would totally make sense for WebGPU implementation to do the same and not require bind groups for pipelines that don't use them (statically, which we have to validate anyway).

@Kangz following Vulkan descriptor inheritance is certainly a (tempting) option, but as I mentioned it's not always clear from looking at the code which bind groups would be invalidated, and this harms API usability/complexity. Lazy validation is also probably required anyway but rubs me in the wrong way :)
How do @RafaelCintron and @litherum feel about Vulkan descriptor inheritance?

@zeux

zeux commented Oct 3, 2018

Copy link
Copy Markdown

You don't need to have the bindings to 4 dynamic UBOs in shaders that don't use them, so I don't see why using the same pipeline layout would penalize those.

Pipeline layout establishes a contract between shaders and command processor; if the pipeline layout specifies that you have 4 UBOs and some number of push constants, and this doesn't fit into user data, then CP (& driver) will prepare the user data according to the pipeline layout, with some registers spilled into memory, and shader will read the descriptors from user data and/or spilled memory depending on which descriptor is needed.

If the shader doesn't use a descriptor, it doesn't change the layout of descriptor data in memory at all. This is why you want the layout to be reasonably optimal for the shader that uses it.

@kvark

kvark commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

@zeux that's great input, thanks for details! This is convincing enough for me to close the PR. Any follow-up feedback is still appreciated, and we should touch on the topic during the next call regardless.

@kvark kvark closed this Oct 3, 2018
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* Add prototype tests for minimum resource limits

* test (V|F|C) + each of V,F,C; silence some typescript complaints

* generalize max-of-resource-type test

* Validation should happen for pipeline layout but NOT bind group layout, and generalize more

* address comments, rename format_info

* nits

Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
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.

4 participants