Add a way to notify with Queue::submit() to Vulkan's vk::Semaphore allocated outside of wgpu #6813
Conversation
wgpu-hal/src/vulkan/mod.rs
Outdated
| &self.device.raw | ||
| } | ||
|
|
||
| pub fn add_signal_semahore(&self, semaphore: vk::Semaphore) { |
There was a problem hiding this comment.
typo: s/semahore/semaphore
wgpu-core/src/resource.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// - The raw queue handle must not be manually destroyed | ||
| pub unsafe fn queue_as_hal<A: HalApi, F: FnOnce(Option<&A::Queue>) -> R, R>( |
There was a problem hiding this comment.
Nit: lets use a where here, as the expressions are complex.
There was a problem hiding this comment.
Nit on top of nit: all other as_hal() functions have the bound directly on the type definition, it would be more consistent to keep it or replace it across the code-base atomically?
| @@ -1210,6 +1211,11 @@ impl crate::Queue for Queue { | |||
| signal_values.push(!0); | |||
| } | |||
|
|
|||
| let mut pending_signal_semaphores = self.signal_semaphores.lock(); | |||
| if !pending_signal_semaphores.is_empty() { | |||
| signal_semaphores.append(&mut pending_signal_semaphores); | |||
There was a problem hiding this comment.
This also needs to push to signal_values or you'll throw a validation error as the values/semaphores won't be matched up.
There was a problem hiding this comment.
I am going to apply the comment.
There was a problem hiding this comment.
What if the caller wants to signal a timeline semaphore? Perhaps additional semaphore values should be exposed as well?
| @@ -1210,6 +1211,12 @@ impl crate::Queue for Queue { | |||
| signal_values.push(!0); | |||
| } | |||
|
|
|||
| let mut pending_signal_semaphores = self.signal_semaphores.lock(); | |||
| if !pending_signal_semaphores.is_empty() { | |||
| signal_values.append(vec![!0; pending_signal_semaphores.len()].as_mut()); | |||
There was a problem hiding this comment.
| signal_values.append(vec![!0; pending_signal_semaphores.len()].as_mut()); | |
| signal_values.extend(iter::repeat(!0).take(pending_signal_semaphores.len())); |
Something like that avoids allocation entirely.
| pub fn add_signal_semaphore(&self, semaphore: vk::Semaphore) { | ||
| let mut signal_semaphores = self.signal_semaphores.lock(); | ||
| signal_semaphores.push(semaphore); | ||
| } |
There was a problem hiding this comment.
I think it would be reasonable to have an argument that is an Option for a signal value, and automatically convert None into !0
There was a problem hiding this comment.
Since the value is ignored, initializing with 0 instead of !0 should be fine too?
There was a problem hiding this comment.
Ah if it's ignored that's fine. I thought it needed to be !0
There was a problem hiding this comment.
https://registry.khronos.org/vulkan/specs/latest/man/html/VkTimelineSemaphoreSubmitInfo.html
If the semaphore in VkSubmitInfo::pWaitSemaphores or VkSubmitInfo::pSignalSemaphores corresponding to an entry in pWaitSemaphoreValues or pSignalSemaphoreValues respectively was not created with a VkSemaphoreType of VK_SEMAPHORE_TYPE_TIMELINE, the implementation must ignore the value in the pWaitSemaphoreValues or pSignalSemaphoreValues entry.
Funnily enough that's the entire description :)
Connections
Fixes #6812
Description
The following changes are done.