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

QuerySet initialization tracking #3993

Open
Wumpf opened this issue Jul 30, 2023 · 3 comments
Open

QuerySet initialization tracking #3993

Wumpf opened this issue Jul 30, 2023 · 3 comments
Labels
area: validation Issues related to validation, diagnostics, and error handling

Comments

@Wumpf
Copy link
Member

Wumpf commented Jul 30, 2023

Calling hal's copy_query_results on queries (as it is done in command_encoder_resolve_query_set) that haven't been written to yet is undefined behavior on DX12 (and others?), potentially causing device loss (see https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12graphicscommandlist-resolvequerydata#remarks)

Furthermore, via spec we're required to set the target buffer to 0 for any unused query (see gpuweb/gpuweb#3812).

This means we're required to track the initialization-state of each query-set and merging it upon queue submit of this encoder.
Implying that only at queue submit time we know at which point in time a given query slot is actually valid.
This validity is infecting the buffer we're resolving to at query resolve time.
Therefore, we either need to split the command encoder at the time of the resolve, so that we can insert the correct resolves or instead, we make copy_query_results not fail with uninitialized queries (dx12 issues dummy queries on creation?) and patch up the buffers's initialization state to be uninitialized for
-> Maybe we need a special MemoryInitKind::FromQueryResolve(resolve_issue_index) for this that we put on dst_buffer.initialization_status.create_action?

It might be possible that it is easier to solve this issue in a backend specific fashion: It seems so far that Metal and Vulkan internally set their query heaps to zero upon reset. Further investigation is needed, but if true we might make an exception to the rule and implement this inside of the HAL layer instead!

@Wumpf Wumpf added the area: validation Issues related to validation, diagnostics, and error handling label Jul 30, 2023
@Wumpf Wumpf added this to the WebGPU Specification V1 milestone Jul 30, 2023
Wumpf added a commit to FL33TW00D/wgpu that referenced this issue Jul 30, 2023
@Wumpf Wumpf mentioned this issue Jul 30, 2023
8 tasks
@Wumpf
Copy link
Member Author

Wumpf commented Jul 30, 2023

There is some weak indication that the issue of undefined behavior for resolving uninitialized queries exists on some Vulkan implementation as well: Prior to removing uninitialized-query-resolve from #3636, CI was observed getting stuck on Linux https://github.com/gfx-rs/wgpu/actions/runs/5637665153/job/15271020249
This is not definitive since there were other changes on that branch since.

@jimblandy
Copy link
Member

Wouldn't it be practical to just initialize these buffers when we create them? That's an awful lot of complexity to avoid just pre-zeroing what are usually really small buffers.

@Wumpf
Copy link
Member Author

Wumpf commented Jan 4, 2024

if we can we should do that! The problem is that Query Sets aren't "real" buffers, we can't write data to them at least not in the way we can write to buffers.

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
Status: No status
Development

No branches or pull requests

2 participants