[hal/vk] Rework Submission and Surface Synchronization (#5681)

Fix two major synchronization issues in `wgpu_val::vulkan`: - Properly order queue command buffer submissions. Due to Mesa bugs, two semaphores are required even though the Vulkan spec says that only one should be necessary. - Properly manage surface texture acquisition and presentation: - Acquiring a surface texture can return while the presentation engine is still displaying the texture. Applications must wait for a semaphore to be signaled before using the acquired texture. - Presenting a surface texture requires a semaphore to ensure that drawing is complete before presentation occurs. Co-authored-by: Jim Blandy <[email protected]>
gfx-rs · May 30, 2024 · c745863 · c745863
1 parent 9b7a965
commit c745863
Show file tree

Hide file tree

Showing 18 changed files with 699 additions and 304 deletions.
diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs
@@ -1499,7 +1499,7 @@ impl Global {
                     .raw
                     .as_ref()
                     .unwrap()
-                    .submit(&refs, &submit_surface_textures, Some((fence, submit_index)))
+                    .submit(&refs, &submit_surface_textures, (fence, submit_index))
                     .map_err(DeviceError::from)?;
             }
 

diff --git a/wgpu-core/src/present.rs b/wgpu-core/src/present.rs
@@ -154,17 +154,20 @@ impl Global {
                 parent_id: surface_id,
             });
         }
-        #[cfg(not(feature = "trace"))]
-        let _ = device;
+
+        let fence_guard = device.fence.read();
+        let fence = fence_guard.as_ref().unwrap();
 
         let suf = A::surface_as_hal(surface.as_ref());
         let (texture_id, status) = match unsafe {
-            suf.unwrap()
-                .acquire_texture(Some(std::time::Duration::from_millis(
-                    FRAME_TIMEOUT_MS as u64,
-                )))
+            suf.unwrap().acquire_texture(
+                Some(std::time::Duration::from_millis(FRAME_TIMEOUT_MS as u64)),
+                fence,
+            )
         } {
             Ok(Some(ast)) => {
+                drop(fence_guard);
+
                 let texture_desc = wgt::TextureDescriptor {
                     label: (),
                     size: wgt::Extent3d {

diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs
@@ -22,7 +22,6 @@ const MAX_BUNNIES: usize = 1 << 20;
 const BUNNY_SIZE: f32 = 0.15 * 256.0;
 const GRAVITY: f32 = -9.8 * 100.0;
 const MAX_VELOCITY: f32 = 750.0;
-const COMMAND_BUFFER_PER_CONTEXT: usize = 100;
 const DESIRED_MAX_LATENCY: u32 = 2;
 
 #[repr(C)]
@@ -498,7 +497,7 @@ impl<A: hal::Api> Example<A> {
             let mut fence = device.create_fence().unwrap();
             let init_cmd = cmd_encoder.end_encoding().unwrap();
             queue
-                .submit(&[&init_cmd], &[], Some((&mut fence, init_fence_value)))
+                .submit(&[&init_cmd], &[], (&mut fence, init_fence_value))
                 .unwrap();
             device.wait(&fence, init_fence_value, !0).unwrap();
             device.destroy_buffer(staging_buffer);
@@ -550,7 +549,7 @@ impl<A: hal::Api> Example<A> {
             {
                 let ctx = &mut self.contexts[self.context_index];
                 self.queue
-                    .submit(&[], &[], Some((&mut ctx.fence, ctx.fence_value)))
+                    .submit(&[], &[], (&mut ctx.fence, ctx.fence_value))
                     .unwrap();
             }
 
@@ -650,7 +649,13 @@ impl<A: hal::Api> Example<A> {
 
         let ctx = &mut self.contexts[self.context_index];
 
-        let surface_tex = unsafe { self.surface.acquire_texture(None).unwrap().unwrap().texture };
+        let surface_tex = unsafe {
+            self.surface
+                .acquire_texture(None, &ctx.fence)
+                .unwrap()
+                .unwrap()
+                .texture
+        };
 
         let target_barrier0 = hal::TextureBarrier {
             texture: surface_tex.borrow(),
@@ -718,7 +723,6 @@ impl<A: hal::Api> Example<A> {
         }
 
         ctx.frames_recorded += 1;
-        let do_fence = ctx.frames_recorded > COMMAND_BUFFER_PER_CONTEXT;
 
         let target_barrier1 = hal::TextureBarrier {
             texture: surface_tex.borrow(),
@@ -732,45 +736,42 @@ impl<A: hal::Api> Example<A> {
 
         unsafe {
             let cmd_buf = ctx.encoder.end_encoding().unwrap();
-            let fence_param = if do_fence {
-                Some((&mut ctx.fence, ctx.fence_value))
-            } else {
-                None
-            };
             self.queue
-                .submit(&[&cmd_buf], &[&surface_tex], fence_param)
+                .submit(
+                    &[&cmd_buf],
+                    &[&surface_tex],
+                    (&mut ctx.fence, ctx.fence_value),
+                )
                 .unwrap();
             self.queue.present(&self.surface, surface_tex).unwrap();
             ctx.used_cmd_bufs.push(cmd_buf);
             ctx.used_views.push(surface_tex_view);
         };
 
-        if do_fence {
-            log::debug!("Context switch from {}", self.context_index);
-            let old_fence_value = ctx.fence_value;
-            if self.contexts.len() == 1 {
-                let hal_desc = hal::CommandEncoderDescriptor {
-                    label: None,
-                    queue: &self.queue,
-                };
-                self.contexts.push(unsafe {
-                    ExecutionContext {
-                        encoder: self.device.create_command_encoder(&hal_desc).unwrap(),
-                        fence: self.device.create_fence().unwrap(),
-                        fence_value: 0,
-                        used_views: Vec::new(),
-                        used_cmd_bufs: Vec::new(),
-                        frames_recorded: 0,
-                    }
-                });
-            }
-            self.context_index = (self.context_index + 1) % self.contexts.len();
-            let next = &mut self.contexts[self.context_index];
-            unsafe {
-                next.wait_and_clear(&self.device);
-            }
-            next.fence_value = old_fence_value + 1;
+        log::debug!("Context switch from {}", self.context_index);
+        let old_fence_value = ctx.fence_value;
+        if self.contexts.len() == 1 {
+            let hal_desc = hal::CommandEncoderDescriptor {
+                label: None,
+                queue: &self.queue,
+            };
+            self.contexts.push(unsafe {
+                ExecutionContext {
+                    encoder: self.device.create_command_encoder(&hal_desc).unwrap(),
+                    fence: self.device.create_fence().unwrap(),
+                    fence_value: 0,
+                    used_views: Vec::new(),
+                    used_cmd_bufs: Vec::new(),
+                    frames_recorded: 0,
+                }
+            });
+        }
+        self.context_index = (self.context_index + 1) % self.contexts.len();
+        let next = &mut self.contexts[self.context_index];
+        unsafe {
+            next.wait_and_clear(&self.device);
         }
+        next.fence_value = old_fence_value + 1;
     }
 }
 

diff --git a/wgpu-hal/examples/raw-gles.rs b/wgpu-hal/examples/raw-gles.rs
@@ -156,6 +156,7 @@ fn fill_screen(exposed: &hal::ExposedAdapter<hal::api::Gles>, width: u32, height
             })
             .unwrap()
     };
+    let mut fence = unsafe { od.device.create_fence().unwrap() };
     let rp_desc = hal::RenderPassDescriptor {
         label: None,
         extent: wgt::Extent3d {
@@ -183,6 +184,6 @@ fn fill_screen(exposed: &hal::ExposedAdapter<hal::api::Gles>, width: u32, height
         encoder.begin_render_pass(&rp_desc);
         encoder.end_render_pass();
         let cmd_buf = encoder.end_encoding().unwrap();
-        od.queue.submit(&[&cmd_buf], &[], None).unwrap();
+        od.queue.submit(&[&cmd_buf], &[], (&mut fence, 0)).unwrap();
     }
 }
diff --git a/wgpu-hal/examples/ray-traced-triangle/main.rs b/wgpu-hal/examples/ray-traced-triangle/main.rs
@@ -13,7 +13,6 @@ use std::{
 };
 use winit::window::WindowButtons;
 
-const COMMAND_BUFFER_PER_CONTEXT: usize = 100;
 const DESIRED_MAX_LATENCY: u32 = 2;
 
 /// [D3D12_RAYTRACING_INSTANCE_DESC](https://microsoft.github.io/DirectX-Specs/d3d/Raytracing.html#d3d12_raytracing_instance_desc)
@@ -759,7 +758,7 @@ impl<A: hal::Api> Example<A> {
             let mut fence = device.create_fence().unwrap();
             let init_cmd = cmd_encoder.end_encoding().unwrap();
             queue
-                .submit(&[&init_cmd], &[], Some((&mut fence, init_fence_value)))
+                .submit(&[&init_cmd], &[], (&mut fence, init_fence_value))
                 .unwrap();
             device.wait(&fence, init_fence_value, !0).unwrap();
             cmd_encoder.reset_all(iter::once(init_cmd));
@@ -808,7 +807,13 @@ impl<A: hal::Api> Example<A> {
     fn render(&mut self) {
         let ctx = &mut self.contexts[self.context_index];
 
-        let surface_tex = unsafe { self.surface.acquire_texture(None).unwrap().unwrap().texture };
+        let surface_tex = unsafe {
+            self.surface
+                .acquire_texture(None, &ctx.fence)
+                .unwrap()
+                .unwrap()
+                .texture
+        };
 
         let target_barrier0 = hal::TextureBarrier {
             texture: surface_tex.borrow(),
@@ -909,7 +914,6 @@ impl<A: hal::Api> Example<A> {
         }
 
         ctx.frames_recorded += 1;
-        let do_fence = ctx.frames_recorded > COMMAND_BUFFER_PER_CONTEXT;
 
         let target_barrier1 = hal::TextureBarrier {
             texture: surface_tex.borrow(),
@@ -959,53 +963,50 @@ impl<A: hal::Api> Example<A> {
 
         unsafe {
             let cmd_buf = ctx.encoder.end_encoding().unwrap();
-            let fence_param = if do_fence {
-                Some((&mut ctx.fence, ctx.fence_value))
-            } else {
-                None
-            };
             self.queue
-                .submit(&[&cmd_buf], &[&surface_tex], fence_param)
+                .submit(
+                    &[&cmd_buf],
+                    &[&surface_tex],
+                    (&mut ctx.fence, ctx.fence_value),
+                )
                 .unwrap();
             self.queue.present(&self.surface, surface_tex).unwrap();
             ctx.used_cmd_bufs.push(cmd_buf);
             ctx.used_views.push(surface_tex_view);
         };
 
-        if do_fence {
-            log::info!("Context switch from {}", self.context_index);
-            let old_fence_value = ctx.fence_value;
-            if self.contexts.len() == 1 {
-                let hal_desc = hal::CommandEncoderDescriptor {
-                    label: None,
-                    queue: &self.queue,
-                };
-                self.contexts.push(unsafe {
-                    ExecutionContext {
-                        encoder: self.device.create_command_encoder(&hal_desc).unwrap(),
-                        fence: self.device.create_fence().unwrap(),
-                        fence_value: 0,
-                        used_views: Vec::new(),
-                        used_cmd_bufs: Vec::new(),
-                        frames_recorded: 0,
-                    }
-                });
-            }
-            self.context_index = (self.context_index + 1) % self.contexts.len();
-            let next = &mut self.contexts[self.context_index];
-            unsafe {
-                next.wait_and_clear(&self.device);
-            }
-            next.fence_value = old_fence_value + 1;
+        log::info!("Context switch from {}", self.context_index);
+        let old_fence_value = ctx.fence_value;
+        if self.contexts.len() == 1 {
+            let hal_desc = hal::CommandEncoderDescriptor {
+                label: None,
+                queue: &self.queue,
+            };
+            self.contexts.push(unsafe {
+                ExecutionContext {
+                    encoder: self.device.create_command_encoder(&hal_desc).unwrap(),
+                    fence: self.device.create_fence().unwrap(),
+                    fence_value: 0,
+                    used_views: Vec::new(),
+                    used_cmd_bufs: Vec::new(),
+                    frames_recorded: 0,
+                }
+            });
+        }
+        self.context_index = (self.context_index + 1) % self.contexts.len();
+        let next = &mut self.contexts[self.context_index];
+        unsafe {
+            next.wait_and_clear(&self.device);
         }
+        next.fence_value = old_fence_value + 1;
     }
 
     fn exit(mut self) {
         unsafe {
             {
                 let ctx = &mut self.contexts[self.context_index];
                 self.queue
-                    .submit(&[], &[], Some((&mut ctx.fence, ctx.fence_value)))
+                    .submit(&[], &[], (&mut ctx.fence, ctx.fence_value))
                     .unwrap();
             }
 

diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs
@@ -857,6 +857,7 @@ impl crate::Surface for Surface {
     unsafe fn acquire_texture(
         &self,
         timeout: Option<std::time::Duration>,
+        _fence: &Fence,
     ) -> Result<Option<crate::AcquiredSurfaceTexture<Api>>, crate::SurfaceError> {
         let mut swapchain = self.swap_chain.write();
         let sc = swapchain.as_mut().unwrap();
@@ -895,7 +896,7 @@ impl crate::Queue for Queue {
         &self,
         command_buffers: &[&CommandBuffer],
         _surface_textures: &[&Texture],
-        signal_fence: Option<(&mut Fence, crate::FenceValue)>,
+        (signal_fence, signal_value): (&mut Fence, crate::FenceValue),
     ) -> Result<(), crate::DeviceError> {
         let mut temp_lists = self.temp_lists.lock();
         temp_lists.clear();
@@ -908,11 +909,9 @@ impl crate::Queue for Queue {
             self.raw.execute_command_lists(&temp_lists);
         }
 
-        if let Some((fence, value)) = signal_fence {
-            self.raw
-                .signal(&fence.raw, value)
-                .into_device_result("Signal fence")?;
-        }
+        self.raw
+            .signal(&signal_fence.raw, signal_value)
+            .into_device_result("Signal fence")?;
 
         // Note the lack of synchronization here between the main Direct queue
         // and the dedicated presentation queue. This is automatically handled

diff --git a/wgpu-hal/src/empty.rs b/wgpu-hal/src/empty.rs
@@ -75,6 +75,7 @@ impl crate::Surface for Context {
     unsafe fn acquire_texture(
         &self,
         timeout: Option<std::time::Duration>,
+        fence: &Resource,
     ) -> Result<Option<crate::AcquiredSurfaceTexture<Api>>, crate::SurfaceError> {
         Ok(None)
     }
@@ -114,7 +115,7 @@ impl crate::Queue for Context {
         &self,
         command_buffers: &[&Resource],
         surface_textures: &[&Resource],
-        signal_fence: Option<(&mut Resource, crate::FenceValue)>,
+        signal_fence: (&mut Resource, crate::FenceValue),
     ) -> DeviceResult<()> {
         Ok(())
     }

diff --git a/wgpu-hal/src/gles/egl.rs b/wgpu-hal/src/gles/egl.rs
@@ -1432,6 +1432,7 @@ impl crate::Surface for Surface {
     unsafe fn acquire_texture(
         &self,
         _timeout_ms: Option<Duration>, //TODO
+        _fence: &super::Fence,
     ) -> Result<Option<crate::AcquiredSurfaceTexture<super::Api>>, crate::SurfaceError> {
         let swapchain = self.swapchain.read();
         let sc = swapchain.as_ref().unwrap();

diff --git a/wgpu-hal/src/gles/queue.rs b/wgpu-hal/src/gles/queue.rs
@@ -1740,7 +1740,7 @@ impl crate::Queue for super::Queue {
         &self,
         command_buffers: &[&super::CommandBuffer],
         _surface_textures: &[&super::Texture],
-        signal_fence: Option<(&mut super::Fence, crate::FenceValue)>,
+        (signal_fence, signal_value): (&mut super::Fence, crate::FenceValue),
     ) -> Result<(), crate::DeviceError> {
         let shared = Arc::clone(&self.shared);
         let gl = &shared.context.lock();
@@ -1774,12 +1774,10 @@ impl crate::Queue for super::Queue {
             }
         }
 
-        if let Some((fence, value)) = signal_fence {
-            fence.maintain(gl);
-            let sync = unsafe { gl.fence_sync(glow::SYNC_GPU_COMMANDS_COMPLETE, 0) }
-                .map_err(|_| crate::DeviceError::OutOfMemory)?;
-            fence.pending.push((value, sync));
-        }
+        signal_fence.maintain(gl);
+        let sync = unsafe { gl.fence_sync(glow::SYNC_GPU_COMMANDS_COMPLETE, 0) }
+            .map_err(|_| crate::DeviceError::OutOfMemory)?;
+        signal_fence.pending.push((signal_value, sync));
 
         Ok(())
     }

diff --git a/wgpu-hal/src/gles/web.rs b/wgpu-hal/src/gles/web.rs
@@ -427,6 +427,7 @@ impl crate::Surface for Surface {
     unsafe fn acquire_texture(
         &self,
         _timeout_ms: Option<std::time::Duration>, //TODO
+        _fence: &super::Fence,
     ) -> Result<Option<crate::AcquiredSurfaceTexture<super::Api>>, crate::SurfaceError> {
         let swapchain = self.swapchain.read();
         let sc = swapchain.as_ref().unwrap();

diff --git a/wgpu-hal/src/gles/wgl.rs b/wgpu-hal/src/gles/wgl.rs
@@ -798,6 +798,7 @@ impl crate::Surface for Surface {
     unsafe fn acquire_texture(
         &self,
         _timeout_ms: Option<Duration>,
+        _fence: &super::Fence,
     ) -> Result<Option<crate::AcquiredSurfaceTexture<super::Api>>, crate::SurfaceError> {
         let swapchain = self.swapchain.read();
         let sc = swapchain.as_ref().unwrap();