-
Notifications
You must be signed in to change notification settings - Fork 897
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
Ensure safety of indirect dispatch #5714
base: trunk
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
018b23b
to
a5bebb0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ac3f089
to
36281af
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
b57350e
to
e0bd41a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ab03bc6
to
abeb863
Compare
This comment was marked as resolved.
This comment was marked as resolved.
d0c4e49
to
0219cfe
Compare
0219cfe
to
fa11fd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased onto trunk
to resolve conflicts.
wgpu-core/src/device/resource.rs
Outdated
) -> Arc<Buffer> { | ||
) -> Result<Arc<Buffer>, resource::CreateBufferError> { | ||
#[cfg(feature = "indirect-validation")] | ||
let raw_indirect_validation_bind_group = | ||
self.create_indirect_validation_bind_group(hal_buffer.as_ref(), desc.size, desc.usage)?; | ||
|
||
unsafe { self.raw().add_raw_buffer(&*hal_buffer) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nical, was it correct to resolve a conflict here by ordering the add_raw_buffer
counter increment after creating raw_indirect_validation_bind_group
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: I believe we shouldn't increment the counter after the only fallible operation that this PR introduces. If we fail, then we don't have a Buffer
created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks correct to me: if create_indirect_validation_bind_group
fails then the Buffer
is not added and destroy_buffer
will not be called so the counter is balanced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review is still WIP for me, sadly. HOWEVER:
- I did split some commits out, and I'm feeling confident that they're truly orthogonal (which means I feel like I'm understanding the scope of individual commits!).
- I'm partway through the
ensure safety of indirect dispatch
commit. I've got the resource validation, creation, and destruction all confirmed in my head, and I'm not concerned about the correctness of those anymore. My next step is to finish reviewing the executed commands in the middle, to make sure they all make sense to me.
praise: This looks hecka complicated, esp. the size calculation for buffers. Kudos for persevering! Hopefully your eyes didn't glaze over too much with all the algebra; I know mine would have (and probably will during review 😅).
buffer_size: u64, | ||
buffer: &dyn hal::DynBuffer, | ||
) -> Result<Box<dyn hal::DynBindGroup>, DeviceError> { | ||
let binding_size = calculate_src_buffer_binding_size(buffer_size, limits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: validate this in my head
fa11fd8
to
456638d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
456638d
to
f35c215
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f35c215
to
5ff8ef9
Compare
64d03cf
to
47c5116
Compare
by injecting a compute shader that validates the content of the indirect buffer
This removes the required barrier prior to the validation dispatch.
47c5116
to
db002de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly done with the ensure safety of indirect dispatch
commit! 😭 🏁
state | ||
.scope | ||
.buffers | ||
.merge_single(&buffer, hal::BufferUses::INDIRECT)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why do we not call merge_single
in the version of code with indirect validation, as with the other? 🤔 Is that on purpose?
wgpu-core/src/command/compute.rs
Outdated
unsafe { | ||
state.raw_encoder.transition_buffers(&[hal::BufferBarrier { | ||
buffer: params.dst_buffer, | ||
usage: hal::BufferUses::INDIRECT..hal::BufferUses::STORAGE_READ_WRITE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This range seems invalid; INDIRECT
is an element that is after STORAGE_READ_WRITE
. Was the intent to provide an empty range? If so, there's probably clearer way to do it. If not, theeeen oops!
ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); | ||
|
||
let _ = run_test(&ctx, &[0, 0, 0], true).await; | ||
|
||
let error = pollster::block_on(ctx.device.pop_error_scope()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RESET_BIND_GROUPS
test fails because we handle validation errors fatally in Queue::submit
. Because the current type signature unconditionally returns a SubmissionIndex
, I need to change wgpu_core
's internals to return it even when there's an error, so we can handle it non-fatally and use the validation error sink (as is according to spec., I think?).
Tagging @cwfitzgerald, @Wumpf, and @jimblandy for validation on this approach. CC @teoxoy for 👀.
The test that is failing in #5714 (comment) seems important, so I'm going to mark this as a draft until we can get that unblocked. I'm planning on continuing work on this during the coming week, though it will not be my highest priority until after #5320. |
Ensure safety of indirect dispatch by injecting a compute shader that validates the content of the indirect buffer.
Part of #2431.