Merge #1673

1673: hal/vk: relay semaphore r=Ralith a=kvark

**Connections**
Somewhere on the Matrix, by @pythonesque

**Description**
Vulkan (somewhat confusingly) requires semaphore synchronization between `vkQueueSubmit` and `vkQueuePresent` *even if* it's done on the same queue.
This PR exercises a dumb idea of just keeping one binary semaphore as a "relay" between all queue commands, all concealed within hal/vk backend.

**Testing**
Running Halmark shows that all the things break here. I'm not seeing anything particularly wrong with the code though. My humble guess is that AMD driver (at least) isn't happy about the mix of binary and timeline semaphores used together in the same signal operation.


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

wgpu-core/src/device/mod.rs

@@ -2446,7 +2446,7 @@ impl<A: hal::Api> Device<A> {
         self.command_allocator.into_inner().dispose(&self.raw);
         unsafe {
             self.raw.destroy_fence(self.fence);
-            self.raw.exit();
+            self.raw.exit(self.queue);
         }
     }
 }

wgpu-hal/examples/halmark/main.rs

@@ -518,10 +518,9 @@ impl<A: hal::Api> Example<A> {
             self.device.destroy_pipeline_layout(self.pipeline_layout);
 
             self.surface.unconfigure(&self.device);
-            self.device.exit();
+            self.device.exit(self.queue);
             self.instance.destroy_surface(self.surface);
             drop(self.adapter);
-            drop(self.queue);
         }
     }
 

wgpu-hal/src/dx12/device.rs

@@ -592,13 +592,14 @@ impl super::Device {
 }
 
 impl crate::Device<super::Api> for super::Device {
-    unsafe fn exit(self) {
+    unsafe fn exit(self, queue: super::Queue) {
         self.rtv_pool.into_inner().destroy();
         self.dsv_pool.into_inner().destroy();
         self.srv_uav_pool.into_inner().destroy();
         self.sampler_pool.into_inner().destroy();
         self.shared.destroy();
         self.idler.destroy();
+        queue.raw.destroy();
 
         // Debug tracking alive objects
         if !thread::panicking() {

wgpu-hal/src/empty.rs

@@ -106,7 +106,7 @@ impl crate::Queue<Api> for Context {
 }
 
 impl crate::Device<Api> for Context {
-    unsafe fn exit(self) {}
+    unsafe fn exit(self, queue: Context) {}
     unsafe fn create_buffer(&self, desc: &crate::BufferDescriptor) -> DeviceResult<Resource> {
         Ok(Resource)
     }

wgpu-hal/src/gles/device.rs

@@ -289,9 +289,12 @@ impl super::Device {
 }
 
 impl crate::Device<super::Api> for super::Device {
-    unsafe fn exit(self) {
+    unsafe fn exit(self, queue: super::Queue) {
         let gl = &self.shared.context;
         gl.delete_vertex_array(self.main_vao);
+        gl.delete_framebuffer(queue.draw_fbo);
+        gl.delete_framebuffer(queue.copy_fbo);
+        gl.delete_buffer(queue.zero_buffer);
     }
 
     unsafe fn create_buffer(

wgpu-hal/src/lib.rs

@@ -204,7 +204,7 @@ pub trait Adapter<A: Api>: Send + Sync {
 
 pub trait Device<A: Api>: Send + Sync {
     /// Exit connection to this logical device.
-    unsafe fn exit(self);
+    unsafe fn exit(self, queue: A::Queue);
     /// Creates a new buffer.
     ///
     /// The initial usage is `BufferUses::empty()`.

wgpu-hal/src/metal/device.rs

@@ -161,7 +161,7 @@ impl super::Device {
 }
 
 impl crate::Device<super::Api> for super::Device {
-    unsafe fn exit(self) {}
+    unsafe fn exit(self, _queue: super::Queue) {}
 
     unsafe fn create_buffer(&self, desc: &crate::BufferDescriptor) -> DeviceResult<super::Buffer> {
         let map_read = desc.usage.contains(crate::BufferUses::MAP_READ);

wgpu-hal/src/vulkan/adapter.rs

@@ -860,6 +860,10 @@ impl crate::Adapter<super::Api> for super::Adapter {
             swapchain_fn,
             device: Arc::clone(&shared),
             family_index,
+            relay_semaphore: shared
+                .raw
+                .create_semaphore(&vk::SemaphoreCreateInfo::builder(), None)?,
+            relay_active: false,
         };
 
         let mem_allocator = {

wgpu-hal/src/vulkan/device.rs

@@ -538,9 +538,12 @@ impl super::Device {
 }
 
 impl crate::Device<super::Api> for super::Device {
-    unsafe fn exit(self) {
+    unsafe fn exit(self, queue: super::Queue) {
         self.mem_allocator.into_inner().cleanup(&*self.shared);
         self.desc_allocator.into_inner().cleanup(&*self.shared);
+        self.shared
+            .raw
+            .destroy_semaphore(queue.relay_semaphore, None);
         self.shared.free_resources();
     }
 

wgpu-hal/src/vulkan/mod.rs

@@ -240,6 +240,12 @@ pub struct Queue {
     swapchain_fn: khr::Swapchain,
     device: Arc<DeviceShared>,
     family_index: u32,
+    /// This special semaphore is used to synchronize GPU work of
+    /// everything on a queue with... itself. Yikes!
+    /// It's required by the confusing portion of the spec to be signalled
+    /// by last submission and waited by the present.
+    relay_semaphore: vk::Semaphore,
+    relay_active: bool,
 }
 
 #[derive(Debug)]
@@ -444,19 +450,18 @@ impl crate::Queue<Api> for Queue {
 
         let mut fence_raw = vk::Fence::null();
         let mut vk_timeline_info;
+        let mut semaphores = [self.relay_semaphore, vk::Semaphore::null()];
         let signal_values;
-        let signal_semaphores;
+
         if let Some((fence, value)) = signal_fence {
             fence.maintain(&self.device.raw)?;
             match *fence {
                 Fence::TimelineSemaphore(raw) => {
-                    signal_values = [value];
-                    signal_semaphores = [raw];
+                    signal_values = [!0, value];
+                    semaphores[1] = raw;
                     vk_timeline_info = vk::TimelineSemaphoreSubmitInfo::builder()
                         .signal_semaphore_values(&signal_values);
-                    vk_info = vk_info
-                        .signal_semaphores(&signal_semaphores)
-                        .push_next(&mut vk_timeline_info);
+                    vk_info = vk_info.push_next(&mut vk_timeline_info);
                 }
                 Fence::FencePool {
                     ref mut active,
@@ -465,16 +470,30 @@ impl crate::Queue<Api> for Queue {
                 } => {
                     fence_raw = match free.pop() {
                         Some(raw) => raw,
-                        None => {
-                            let vk_info = vk::FenceCreateInfo::builder().build();
-                            self.device.raw.create_fence(&vk_info, None)?
-                        }
+                        None => self
+                            .device
+                            .raw
+                            .create_fence(&vk::FenceCreateInfo::builder(), None)?,
                     };
                     active.push((value, fence_raw));
                 }
             }
         }
 
+        let wait_stage_mask = [vk::PipelineStageFlags::TOP_OF_PIPE];
+        if self.relay_active {
+            vk_info = vk_info
+                .wait_semaphores(&semaphores[..1])
+                .wait_dst_stage_mask(&wait_stage_mask);
+        }
+        self.relay_active = true;
+        let signal_count = if semaphores[1] == vk::Semaphore::null() {
+            1
+        } else {
+            2
+        };
+        vk_info = vk_info.signal_semaphores(&semaphores[..signal_count]);
+
         self.device
             .raw
             .queue_submit(self.raw, &[vk_info.build()], fence_raw)?;
@@ -490,10 +509,16 @@ impl crate::Queue<Api> for Queue {
 
         let swapchains = [ssc.raw];
         let image_indices = [texture.index];
-        let vk_info = vk::PresentInfoKHR::builder()
+        let semaphores = [self.relay_semaphore];
+        let mut vk_info = vk::PresentInfoKHR::builder()
             .swapchains(&swapchains)
             .image_indices(&image_indices);
 
+        if self.relay_active {
+            vk_info = vk_info.wait_semaphores(&semaphores);
+            self.relay_active = false;
+        }
+
         let suboptimal = self
             .swapchain_fn
             .queue_present(self.raw, &vk_info)