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

Clear or validate resources to be initialized #563

Closed
kvark opened this issue Apr 8, 2020 · 6 comments
Closed

Clear or validate resources to be initialized #563

kvark opened this issue Apr 8, 2020 · 6 comments
Labels
area: validation Issues related to validation, diagnostics, and error handling

Comments

@kvark
Copy link
Member

kvark commented Apr 8, 2020

The upstream spec requires us to zero-initialize resources: buffers and textures. I'm not a fan of that personally, but we need to validate the initialization regardless. Currently we aren't validating that. Example complaint:

[2020-04-08T16:09:02Z TRACE wgpu_core::command::render] Encoding render pass begin in command buffer (0, 1, Vulkan)
[2020-04-08T16:09:02Z WARN gfx_backend_vulkan]
VALIDATION [UNASSIGNED-CoreValidation-DrawState-InvalidRenderpass (0)] : Render pass has an attachment with loadOp == VK_ATTACHMENT_LOAD_OP_LOAD and initialLayout == VK_IMAGE_LAYOUT_UNDEFINED. This is probably not what you intended. Consider using VK_ATTACHMENT_LOAD_OP_DONT_CARE instead if the image truely is undefined at the start of the render pass.

The desired behavior here is to lazy-initialize resources that the user doesn't properly initialize, and issue a warning otherwise.

@kvark kvark added the area: validation Issues related to validation, diagnostics, and error handling label Apr 8, 2020
@Wumpf
Copy link
Member

Wumpf commented Jan 10, 2021

I might be interested in solving this :)

The desired behavior here is to lazy-initialize resources that the user doesn't properly initialize, and issue a warning otherwise.

So the idea is that the zero initialize is explicitly discouraged and logs a warning? I was actually hoping to rely on in it for a few things - I have some awkward "if this is first frame then read zero instead" things so far since there's no general clearbuffer/texture exposed :/

Besides this sentiment, there's a couple of cases where I'd argue it's too hard to avoid this warning since I believe there is only two ways to transparently clear a texture/buffer:

  • fill it entirely on creation
  • clear on start of a renderpass

(binding for write doesn't help because shader may not actually write any data)

This is a bit problematic for a wide range of usecases. E.g. a large voxel volume that is to be filled with mesh data at runtime has no way of clearing to zero other than creating a large zero sized array and "uploading" that on creation. This is unfortunate since inside the wgpu implementation we could call clear_image/fill_buffer which are much cheaper & easier but sadly not exposed in WebGPU.

@kvark
Copy link
Member Author

kvark commented Jan 10, 2021

Upstream requires this to be the desired behavior, so if you are interested in implementing it this way, without any warnings, we'd be totally cool, and much appreciated! This is one of the core bits of the API that we haven't progressed with, and it's getting needed more and more.

I have a slight concern about users actually relying on this behavior, which I'll try to explain here, but I'm not going to push it. If somebody relies on the blank state at start, they would eventually want to "reset" a resource to this state, to rely on that again. And then they'll demand clear_image and fill_buffer on the API level (as you mentioned). But if we have those exposed, then users will get all the tools they need to avoid uninitialized resources, so the case needs to be made about why we should be doing this work for them. When reading the code, I'd like to see more explicitly what's going on. An implicit operation that mutates resource contents goes across that.

@Wumpf
Copy link
Member

Wumpf commented Jan 10, 2021

Makes sense but my line of argumentation is that users will need/want clear_image/fill_buffer more so if we issue a warning (so they can avoid it).

I actually thought the doing work for them part is entirely motivated by security guarantees for the web in order to avoid reading out uninitialized memory. That would hold true no matter what is exposed.

@kvark
Copy link
Member Author

kvark commented Jan 10, 2021

Yes, I agree, the warnings are not sound. We need to either silently provide this (like upstream), or make it an error entirely. Both approaches are secure.

@Wumpf Wumpf mentioned this issue Jan 19, 2021
5 tasks
bors bot added a commit that referenced this issue Feb 1, 2021
1159: Zero initialize buffers r=kvark a=Wumpf

**Connections**
First half of  #563, focusing solely on buffers and ignoring same issue for textures

**Description**
Tracks for each buffer which parts are initialized (i.e. set to zero). Identified three interaction points for this:
* buffer mapping: Zeroing out ad-hoc and marking as initialized
* queue write to buffer: Marks target buffer regions as initialized (i.e. optimizes away buffer init down the line)
* use in binding or copy operation in a command buffer:
   * fine grained tracking of regions that may require init (or will be initialized implicitly) on each command buffer
   * set in motion on queue submit, init is exclusively with `fill_buffer`

Todo list for Ready-to-Review

- [x] memory barriers for `fill_buffer` calls
- [x] better data structure for `memory_init_tracker`
- [x] coarse filtering on command-buffer buffer init requirements (the list should almost always be empty whereas now it pushes any buffer use)
- [x] improve naming of things
- [x] at least pretend this is adequately tested

Todo list beyond this PR

* make data structures usable for textures
  * and.. well.. implement all this for textures!
* explore reusing barrier tracker for memory init tracking?


**Testing**

* Some basic testing by doing some changes to wgpu-rs samples and watching them in in the debugger.
* Added a ron test file for the player (love those!) to poke the feature a little bit
* MemoryInitTracker comes with simple unit tests

Overall this is a bit shallow but as so often in this area accurate testing is hard because the outcomes are quite indirect



Co-authored-by: Andreas Reich <r_andreas2@web.de>
@kvark
Copy link
Member Author

kvark commented Feb 7, 2021

The buffer initialization has landed in #1159. The textures can be implemented with clear_image calls internally, but there is also gpuweb/gpuweb#1422 that could be blocking this.

@kvark kvark changed the title Validate resources to be initialized Clear or validate resources to be initialized Feb 7, 2021
@kvark
Copy link
Member Author

kvark commented May 21, 2021

#1102 is left for textures

@kvark kvark closed this as completed May 21, 2021
@kvark kvark added this to the WebGPU MVP milestone May 21, 2021
kvark added a commit to kvark/wgpu that referenced this issue Jun 3, 2021
563: Document bind buffer alignment r=kvark a=kvark

Fixes  gfx-rs#561
Also updates wgpu dependency to latest, and our exampels are now fully validated for the shader interface 🎉 .

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling
Projects
None yet
Development

No branches or pull requests

2 participants