Disentangle frame callback sequence from real DRM sequence

It can currently happen that the estimated VBlank timer fires right
before a real VBlank, which can cause some sequence collisions, which
might cause frame callbacks to never be sent. To prevent this, just
track the frame callback sequence fully separately. There isn't really
any harm in this, and if we accidentally increment it more frequently
than necessary then nothing terrible will happen.

3 files changed, 43 insertions, 30 deletions

diff --git a/src/backend/tty.rs b/src/backend/tty.rs
index 63e81322..77b88cc8 100644
--- a/src/backend/tty.rs
+++ b/src/backend/tty.rs
@@ -983,15 +983,15 @@ impl Tty {
             }
         }
 
-        if let Some(last_sequence) = output_state.current_estimated_sequence {
+        if let Some(last_sequence) = output_state.last_drm_sequence {
             let delta = meta.sequence as f64 - last_sequence as f64;
             tracy_client::Client::running()
                 .unwrap()
                 .plot(surface.sequence_delta_plot_name, delta);
         }
+        output_state.last_drm_sequence = Some(meta.sequence);
 
         output_state.frame_clock.presented(presentation_time);
-        output_state.current_estimated_sequence = Some(meta.sequence);
 
         let redraw_needed = match mem::replace(&mut output_state.redraw_state, RedrawState::Idle) {
             RedrawState::Idle => unreachable!(),
@@ -1024,6 +1024,9 @@ impl Tty {
             return;
         };
 
+        // We waited for the timer, now we can send frame callbacks again.
+        output_state.frame_callback_sequence = output_state.frame_callback_sequence.wrapping_add(1);
+
         match mem::replace(&mut output_state.redraw_state, RedrawState::Idle) {
             RedrawState::Idle => unreachable!(),
             RedrawState::Queued(_) => unreachable!(),
@@ -1036,14 +1039,10 @@ impl Tty {
             }
         }
 
-        if let Some(sequence) = output_state.current_estimated_sequence.as_mut() {
-            *sequence = sequence.wrapping_add(1);
-
-            if output_state.unfinished_animations_remain {
-                niri.queue_redraw(output);
-            } else {
-                niri.send_frame_callbacks(&output);
-            }
+        if output_state.unfinished_animations_remain {
+            niri.queue_redraw(output);
+        } else {
+            niri.send_frame_callbacks(&output);
         }
     }
 
@@ -1147,12 +1146,11 @@ impl Tty {
                                 }
                             };
 
-                            // We queued this frame successfully, so now we'll be sending frame
-                            // callbacks for the next sequence.
-                            if let Some(sequence) = output_state.current_estimated_sequence.as_mut()
-                            {
-                                *sequence = sequence.wrapping_add(1);
-                            }
+                            // We queued this frame successfully, so the current client buffers were
+                            // latched. We can send frame callbacks now, since a new client commit
+                            // will no longer overwrite this frame and will wait for a VBlank.
+                            output_state.frame_callback_sequence =
+                                output_state.frame_callback_sequence.wrapping_add(1);
 
                             return RenderResult::Submitted;
                         }
diff --git a/src/backend/winit.rs b/src/backend/winit.rs
index e9413698..580802bc 100644
--- a/src/backend/winit.rs
+++ b/src/backend/winit.rs
@@ -199,6 +199,10 @@ impl Winit {
             RedrawState::WaitingForEstimatedVBlankAndQueued(_) => unreachable!(),
         }
 
+        output_state.frame_callback_sequence = output_state.frame_callback_sequence.wrapping_add(1);
+
+        // FIXME: this should wait until a frame callback from the host compositor, but it redraws
+        // right away instead.
         if output_state.unfinished_animations_remain {
             self.backend.window().request_redraw();
         }
diff --git a/src/niri.rs b/src/niri.rs
index 28aba19e..2d1c80ff 100644
--- a/src/niri.rs
+++ b/src/niri.rs
@@ -219,15 +219,28 @@ pub struct OutputState {
     pub redraw_state: RedrawState,
     // After the last redraw, some ongoing animations still remain.
     pub unfinished_animations_remain: bool,
-    /// Estimated sequence currently displayed on this output.
+    /// Last sequence received in a vblank event.
+    pub last_drm_sequence: Option<u32>,
+    /// Sequence for frame callback throttling.
     ///
-    /// When a frame is presented on this output, this becomes the real sequence from the VBlank
-    /// callback. Then, as long as there are no KMS submissions, but we keep getting commits, this
-    /// sequence increases by 1 at estimated VBlank times.
+    /// We want to send frame callbacks for each surface at most once per monitor refresh cycle.
     ///
-    /// If there are no commits, then we won't have a timer running, so the estimated sequence will
-    /// not increase.
-    pub current_estimated_sequence: Option<u32>,
+    /// Even if a surface commit resulted in empty damage to the monitor, we want to delay the next
+    /// frame callback until roughly when a VBlank would occur, had the monitor been damaged. This
+    /// is necessary to prevent clients busy-looping with frame callbacks that result in empty
+    /// damage.
+    ///
+    /// This counter wrapping-increments by 1 every time we move into the next refresh cycle, as
+    /// far as frame callback throttling is concerned. Specifically, it happens:
+    ///
+    /// 1. Upon a successful DRM frame submission. Notably, we don't wait for the VBlank here,
+    ///    because the client buffers are already "latched" at the point of submission. Even if a
+    ///    client submits a new buffer right away, we will wait for a VBlank to draw it, which
+    ///    means that busy looping is avoided.
+    /// 2. If a frame resulted in empty damage, a timer is queued to fire roughly when a VBlank
+    ///    would occur, based on the last presentation time and output refresh interval. Sequence
+    ///    is incremented in that timer, before attempting a redraw or sending frame callbacks.
+    pub frame_callback_sequence: u32,
     /// Solid color buffer for the background that we use instead of clearing to avoid damage
     /// tracking issues and make screenshots easier.
     pub background_buffer: SolidColorBuffer,
@@ -1246,7 +1259,8 @@ impl Niri {
             redraw_state: RedrawState::Idle,
             unfinished_animations_remain: false,
             frame_clock: FrameClock::new(refresh_interval),
-            current_estimated_sequence: None,
+            last_drm_sequence: None,
+            frame_callback_sequence: 0,
             background_buffer: SolidColorBuffer::new(size, CLEAR_COLOR),
             lock_render_state,
             lock_surface: None,
@@ -2368,7 +2382,7 @@ impl Niri {
         let _span = tracy_client::span!("Niri::send_frame_callbacks");
 
         let state = self.output_state.get(output).unwrap();
-        let sequence = state.current_estimated_sequence;
+        let sequence = state.frame_callback_sequence;
 
         let should_send = |surface: &WlSurface, states: &SurfaceData| {
             // Do the standard primary scanout output check. For pointer surfaces it deduplicates
@@ -2390,16 +2404,13 @@ impl Niri {
             // If we already sent a frame callback to this surface this output refresh
             // cycle, don't send one again to prevent empty-damage commit busy loops.
             if let Some((last_output, last_sequence)) = &*last_sent_at {
-                if last_output == output && Some(*last_sequence) == sequence {
+                if last_output == output && *last_sequence == sequence {
                     send = false;
                 }
             }
 
             if send {
-                if let Some(sequence) = sequence {
-                    *last_sent_at = Some((output.clone(), sequence));
-                }
-
+                *last_sent_at = Some((output.clone(), sequence));
                 Some(output.clone())
             } else {
                 None