gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.41k stars 909 forks source link

QuerySet initialization tracking #3993

Open Wumpf opened 1 year ago

Wumpf commented 1 year ago

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 https://github.com/gpuweb/gpuweb/issues/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 commented 1 year ago

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 https://github.com/gfx-rs/wgpu/pull/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 commented 9 months ago

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 commented 9 months ago

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.