Merge pull request #740 from mahkoh/jorth/metal-fixes

metal: sync + ctx change fixes

src/backend.rs

@@ -183,8 +183,7 @@ pub trait HardwareCursorUpdate {
     fn set_enabled(&mut self, enabled: bool);
     fn get_buffer(&self) -> Rc<dyn GfxFramebuffer>;
     fn set_position(&mut self, x: i32, y: i32);
-    fn swap_buffer(&mut self);
+    fn swap_buffer(&mut self, sync_file: Option<SyncFile>);
-    fn set_sync_file(&mut self, sync_file: Option<SyncFile>);
     fn size(&self) -> (i32, i32);
 }
 

src/backends/metal/present.rs

@@ -6,6 +6,7 @@ use {
             transaction::{DrmConnectorState, DrmPlaneState},
             video::{
                 MetalConnector, MetalCrtc, MetalHardwareCursorChange, MetalPlane, RenderBuffer,
+                RenderBufferCopy,
             },
         },
         cmm::cmm_description::ColorDescription,
@@ -71,7 +72,7 @@ pub struct PresentFb {
     fb: Rc<DrmFramebuffer>,
     tex: Rc<dyn GfxTexture>,
     direct_scanout_data: Option<DirectScanoutData>,
-    sync_file: Option<SyncFile>,
+    copy: RenderBufferCopy,
     pub locked: bool,
 }
 
@@ -103,6 +104,12 @@ pub const DEFAULT_PRE_COMMIT_MARGIN: u64 = 16_000_000; // 16ms
 pub const DEFAULT_POST_COMMIT_MARGIN: u64 = 1_500_000; // 1.5ms;
 pub const POST_COMMIT_MARGIN_DELTA: u64 = 500_000; // 500us
 
+#[derive(Copy, Clone)]
+enum PresentFbWait {
+    Render,
+    Scanout,
+}
+
 impl MetalConnector {
     pub fn schedule_present(&self) {
         self.present_trigger.trigger();
@@ -227,6 +234,13 @@ impl MetalConnector {
             direct_scanout_id = fb.direct_scanout_data.as_ref().map(|d| d.dma_buf_id);
             present_fb = Some(fb);
         }
+        self.await_present_fb(present_fb.as_mut(), PresentFbWait::Render)
+            .await;
+        // perform_screencopies should return a sync file that we wait on before
+        // presentation since during screencopy the buffer layout might be mutated which
+        // could interfere with scanout. However, perform_screencopies just uses the
+        // current PresentFb if present_fb is None, potentially mutating the fb that is
+        // currently being scanned out, which would render such a wait absurd.
         self.perform_screencopies(&present_fb, &node, &cd);
         if let Some(sync_file) = self.cursor_sync_file.take()
             && let Err(e) = self.state.ring.readable(&sync_file).await
@@ -236,7 +250,8 @@ impl MetalConnector {
                 ErrorFmt(e)
             );
         }
-        self.await_present_fb(present_fb.as_mut()).await;
+        self.await_present_fb(present_fb.as_mut(), PresentFbWait::Scanout)
+            .await;
         let mut changed_planes = ArrayVec::new();
         let mut res = self.program_connector(
             version,
@@ -259,7 +274,8 @@ impl MetalConnector {
                 false,
             )?;
             present_fb = Some(fb);
-            self.await_present_fb(present_fb.as_mut()).await;
+            self.await_present_fb(present_fb.as_mut(), PresentFbWait::Scanout)
+                .await;
             res = self.program_connector(
                 version,
                 &crtc,
@@ -330,17 +346,26 @@ impl MetalConnector {
         }
     }
 
-    async fn await_present_fb(&self, new_fb: Option<&mut PresentFb>) {
+    async fn await_present_fb(&self, new_fb: Option<&mut PresentFb>, wait: PresentFbWait) {
+        use PresentFbWait as W;
         let Some(fb) = new_fb else {
             return;
         };
-        let Some(sync_file) = fb.sync_file.take() else {
+        let field = match wait {
+            W::Render => &mut fb.copy.render_block,
+            W::Scanout => &mut fb.copy.present_block,
+        };
+        let Some(sync_file) = field.take() else {
             return;
         };
         if let Err(e) = self.state.ring.readable(&sync_file).await {
+            let name = match wait {
+                W::Render => "render",
+                W::Scanout => "scanout",
+            };
             log::error!(
-                "Could not wait for primary sync file to complete: {}",
+                "Could not wait for primary {name} sync file to complete: {}",
-                ErrorFmt(e)
+                ErrorFmt(e),
             );
         }
     }
@@ -522,24 +547,22 @@ impl MetalConnector {
         let buffer_idx = ((connector_drm_state.cursor_fb_idx + 1) % buffers.len() as u64) as usize;
         let mut c = MetalHardwareCursorChange {
             cursor_enabled: self.cursor_enabled.get(),
-            cursor_swap_buffer: false,
+            cursor_swap_buffer: None,
             cursor_x: self.cursor_x.get(),
             cursor_y: self.cursor_y.get(),
             cursor_buffer: &buffers[buffer_idx],
-            sync_file: None,
             cursor_size: (self.dev.cursor_width as _, self.dev.cursor_height as _),
         };
         self.state.present_hardware_cursor(node, &mut c);
-        if c.cursor_swap_buffer {
+        let swap_buffers = c.cursor_swap_buffer.is_some();
-            c.sync_file = c.cursor_buffer.copy_to_dev(cd, c.sync_file)?;
+        self.cursor_swap_buffer.set(swap_buffers);
-        }
+        if let Some(sf) = c.cursor_swap_buffer.take() {
-        self.cursor_swap_buffer.set(c.cursor_swap_buffer);
+            let sf = c.cursor_buffer.copy_to_dev(cd, sf)?.present_block;
-        if c.sync_file.is_some() {
+            self.cursor_sync_file.set(sf);
-            self.cursor_sync_file.set(c.sync_file);
         }
         let mut cursor_changed = false;
         cursor_changed |= self.cursor_enabled.replace(c.cursor_enabled) != c.cursor_enabled;
-        cursor_changed |= c.cursor_swap_buffer;
+        cursor_changed |= swap_buffers;
         cursor_changed |= self.cursor_x.replace(c.cursor_x) != c.cursor_x;
         cursor_changed |= self.cursor_y.replace(c.cursor_y) != c.cursor_y;
         if cursor_changed {
@@ -833,7 +856,7 @@ impl MetalConnector {
             };
             log::debug!("{} direct scanout on {}", change, self.kernel_id());
         }
-        let sync_file;
+        let copy;
         let fb;
         let tex;
         match &direct_scanout_data {
@@ -850,17 +873,18 @@ impl MetalConnector {
                         blend_cd,
                     )
                     .map_err(MetalError::RenderFrame)?;
-                sync_file = buffer.copy_to_dev(cd, sf)?;
+                copy = buffer.copy_to_dev(cd, sf)?;
                 fb = buffer.drm.clone();
                 tex = buffer.render_tex.clone();
             }
             Some(dsd) => {
-                sync_file = match &dsd.acquire_sync {
+                let sf = match &dsd.acquire_sync {
                     AcquireSync::None => None,
                     AcquireSync::Implicit => None,
                     AcquireSync::SyncFile { sync_file } => Some(sync_file.clone()),
                     AcquireSync::Unnecessary => None,
                 };
+                copy = RenderBufferCopy::for_both(sf);
                 fb = dsd.fb.clone();
                 tex = dsd.tex.clone();
             }
@@ -869,7 +893,7 @@ impl MetalConnector {
             fb,
             tex,
             direct_scanout_data,
-            sync_file,
+            copy,
             locked: latched.locked,
         })
     }

src/backends/metal/video.rs

@@ -557,12 +557,11 @@ pub struct MetalHardwareCursor {
 }
 
 pub struct MetalHardwareCursorChange<'a> {
-    pub cursor_swap_buffer: bool,
+    pub cursor_swap_buffer: Option<Option<SyncFile>>,
     pub cursor_enabled: bool,
     pub cursor_x: i32,
     pub cursor_y: i32,
     pub cursor_buffer: &'a RenderBuffer,
-    pub sync_file: Option<SyncFile>,
     pub cursor_size: (i32, i32),
 }
 
@@ -596,12 +595,8 @@ impl HardwareCursorUpdate for MetalHardwareCursorChange<'_> {
         self.cursor_y = y;
     }
 
-    fn swap_buffer(&mut self) {
+    fn swap_buffer(&mut self, sync_file: Option<SyncFile>) {
-        self.cursor_swap_buffer = true;
+        self.cursor_swap_buffer = Some(sync_file);
-    }
-
-    fn set_sync_file(&mut self, sync_file: Option<SyncFile>) {
-        self.sync_file = sync_file;
     }
 
     fn size(&self) -> (i32, i32) {
@@ -2443,7 +2438,7 @@ impl MetalBackend {
         self.default_feedback.set(fb);
         self.ctx.set(Some(ctx));
         for dev in self.device_holder.drm_devices.lock().values() {
-            self.re_init_drm_device(&dev);
+            self.init_drm_device_after_gfx_ctx_change(&dev);
             for connector in dev.connectors.lock().values() {
                 connector.send_hardware_cursor();
             }
@@ -2478,18 +2473,25 @@ impl MetalBackend {
             self.make_render_device(dev, true);
         } else {
             if let Some(dev) = self.device_holder.drm_devices.get(&dev.devnum) {
-                self.re_init_drm_device(&dev);
+                self.init_drm_device_after_gfx_ctx_change(&dev);
             }
         }
     }
 
-    fn re_init_drm_device(&self, dev: &Rc<MetalDrmDeviceData>) {
+    fn init_drm_device_after_gfx_ctx_change(&self, dev: &Rc<MetalDrmDeviceData>) {
         if let Err(e) = self.init_drm_device(dev) {
             log::error!(
                 "Could not initialize drm device {}: {}",
                 dev.dev.devnode.as_bytes().as_bstr(),
                 ErrorFmt(e),
             );
+            for connector in dev.connectors.lock().values() {
+                connector.buffers.take();
+                connector.cursor_buffers.take();
+            }
+            for plane in dev.dev.planes.values() {
+                plane.drm_state.borrow_mut().buffers.take();
+            }
         }
         for connector in dev.connectors.lock().values() {
             if connector.connected() {
@@ -2963,6 +2965,21 @@ pub struct RenderBuffer {
     pub render_fb: Option<Rc<dyn GfxFramebuffer>>,
 }
 
+#[derive(Default)]
+pub struct RenderBufferCopy {
+    pub render_block: Option<SyncFile>,
+    pub present_block: Option<SyncFile>,
+}
+
+impl RenderBufferCopy {
+    pub fn for_both(sf: Option<SyncFile>) -> Self {
+        Self {
+            render_block: sf.clone(),
+            present_block: sf,
+        }
+    }
+}
+
 impl RenderBuffer {
     pub fn render_fb(&self) -> Rc<dyn GfxFramebuffer> {
         self.render_fb
@@ -2974,9 +2991,12 @@ impl RenderBuffer {
         &self,
         cd: &Rc<ColorDescription>,
         sync_file: Option<SyncFile>,
-    ) -> Result<Option<SyncFile>, MetalError> {
+    ) -> Result<RenderBufferCopy, MetalError> {
         let Some(tex) = &self.dev_tex else {
-            return Ok(sync_file);
+            return Ok(RenderBufferCopy {
+                render_block: None,
+                present_block: sync_file,
+            });
         };
         self.dev_fb
             .copy_texture(
@@ -2992,6 +3012,7 @@ impl RenderBuffer {
                 0,
             )
             .map_err(MetalError::CopyToOutput)
+            .map(RenderBufferCopy::for_both)
     }
 
     pub fn damage_full(&self) {

src/cursor_user.rs

@@ -515,8 +515,7 @@ impl CursorUser {
             );
             match res {
                 Ok(sync_file) => {
-                    hc.set_sync_file(sync_file);
+                    hc.swap_buffer(sync_file);
-                    hc.swap_buffer();
                 }
                 Err(e) => {
                     log::error!("Could not render hardware cursor: {}", ErrorFmt(e));