Implement indirect draw validation #7140
Conversation
teoxoy
force-pushed
the
indirect-draw-validation
branch
2 times, most recently
from
d12fe5a to
1373962
Compare
There was a problem hiding this comment.
I think my only overarching comment would be - I wonder if we can somehow internalize the cfgs in some way so that we don't need to have them all over the codebase.
I also worry about the non-indirect buffer pass starting to fail to compile and us not noticing WebGL2 compiles without it.
| hal::PipelineError::Device(error) => { | ||
| CreateComputePipelineError::Device(DeviceError::from_hal(error)) | ||
| } | ||
| hal::PipelineError::Linkage(_stages, msg) => CreateComputePipelineError::Internal(msg), | ||
| hal::PipelineError::EntryPoint(_stage) => CreateComputePipelineError::Internal( | ||
| crate::device::ENTRYPOINT_FAILURE_ERROR.to_string(), | ||
| ), | ||
| hal::PipelineError::PipelineConstants(_, error) => { | ||
| CreateComputePipelineError::PipelineConstants(error) | ||
| } |
There was a problem hiding this comment.
Thought: could these be in some kind of From implementation?
There was a problem hiding this comment.
I don't think we should, device errors in hal::PipelineError::Device need to go through device.handle_hal_error() unless there is no device yet (in this case). A From impl would be easy to misuse.
I think at some point we should coalesce the Linkage and EntryPoint variants though.
teoxoy
force-pushed
the
indirect-draw-validation
branch
3 times, most recently
from
a3b3a0f to
b17487d
Compare
|
I ended up removing the |
teoxoy
force-pushed
the
indirect-draw-validation
branch
from
c5bbf04 to
f7b7d51
Compare
teoxoy
force-pushed
the
indirect-draw-validation
branch
2 times, most recently
from
30b2a00 to
329b538
Compare
teoxoy
force-pushed
the
indirect-draw-validation
branch
4 times, most recently
from
5917833 to
50a78af
Compare
| for index in batches | ||
| .values() | ||
| .map(|batch| batch.metadata_resource_index) | ||
| .filter(|index| buffer_index_set.insert(*index)) |
There was a problem hiding this comment.
Could we do these as early-continues in the for loop - as we're iterating already, I think it would read more clearly
There was a problem hiding this comment.
Related: I was worried about those manual .clear() calls. I added 2 new data structures to get rid of the possibility of forgetting to do things in the right order.
Let me know what you think!
| let vertex_or_index_count = src[src_base_offset + 0]; | ||
|
|
||
| { | ||
| let can_overflow = ((metadata.dst_offset >> 30) & 1u) == 1u; |
There was a problem hiding this comment.
this would be a bit clearer using extractBits - if you don't want to use it for performance reasons, maybe a free function?
There was a problem hiding this comment.
The backends add some validation code for extractBits to cap the offset and count. I'll add a function.
teoxoy
force-pushed
the
indirect-draw-validation
branch
from
50a78af to
017a071
Compare
teoxoy
force-pushed
the
indirect-draw-validation
branch
from
fe4bbbf to
d5960bf
Compare
teoxoy
force-pushed
the
indirect-draw-validation
branch
from
d5960bf to
74dc697
Compare
…TION_INDIRECT_CALL`. With the only caveat that device creation will now panic if the `wgsl` feature is not enabled, `InstanceFlags::VALIDATION_INDIRECT_CALL` is set and the device supports `DownlevelFlags::INDIRECT_EXECUTION`.
…IRECT_EXECUTION` is enabled
rename items appropriately internalize indirect buffer bind groups
Connections
Closes #2431.
Description
Implements indirect draw validation by batching draw calls and injecting a compute pass prior to the render pass that will validate the indirect args.
NOTES
vertex_indexandinstance_indexbuilt-in inputs for indirect draw calls #2471notcovered (see Implement indirect draw validation #7140 (comment)).Testing
Added new tests, probably not enough, there are lots of possible permutations. I looked at branch coverage and most branches are tested at least.
The commits are self contained and so the PR doesn't need to be squashed.