shim: no vblank-grid gate under the cap-rate FIFO declaration; a late start is released at the next grid instant

docs/10-fps-limit.md

@@ -119,7 +119,7 @@ divisor cap reads its target exactly.
   |---|---|---|---|
   | mailbox (default) | none, guest asked for FIFO (in-game VSync on) | fifo (`source=guest`) | the display (FIFO) |
   | mailbox (default) | none, guest asked for mailbox or immediate | mailbox | nobody |
-  | mailbox | at the display's rate | fifo (`source=cap-rate`) | the shim's acquire gate, every vblank; gamescope latches one FIFO commit per vblank |
+  | mailbox | at the display's rate | fifo (`source=cap-rate`) | gamescope's limiter (divisor 1), one FIFO commit per vblank; no gate, as on master |
   | mailbox | below the display's rate | mailbox | the shim's acquire gate, on the display's vblank grid |
   | fifo | none | fifo | the display (FIFO) |
   | fifo | set | fifo | gamescope's divisor limiter |
@@ -166,9 +166,14 @@ divisor cap reads its target exactly.
   gamescope's mailbox; a one-commit-per-window queue in the shim at a guessed
   2.5 ms boundary gave 59.0 with ~0.8 a second still replaced; FIFO on master
   gives 59.22 on the same scene. Under FIFO gamescope latches one commit per
-  vblank at its own wakeup (`steamcompmgr.cpp` 8028-8049), its limiter's divisor
-  at this cap is 1, and the gate keeps the guest from getting ahead, so the FIFO
-  queue stays about one frame deep. Below the display's rate the gate's releases
+  vblank at its own wakeup (`steamcompmgr.cpp` 8028-8049) and its limiter's
+  divisor at this cap is 1, which is master's behaviour in this cell. The
+  vblank-grid gate is not kept on top of it: FIFO's back-pressure already holds
+  the guest, and the two stacked turned a guest whose acquire-to-commit grew
+  past a period into a 40 fps session on a 60 Hz panel (DTD on a 1280x960
+  device after a live 120 to 60 Hz switch: `gate_holds` 0, `gate_late` 400 and
+  `skipped` 200 per 10 s, commit latency 24 ms, 400/600 publishes). Below the
+  display's rate the gate's releases
   are at least two vblanks apart and two frames in one composite are rare, so
   mailbox stays (FIFO with gamescope's limiter quantized a 32 fps game to 30 at a
   60 cap on 120 Hz; mailbox with the gate did not).
@@ -182,8 +187,9 @@ divisor cap reads its target exactly.
   other cap below the rate, the vblank nearest each instant of an ideal
   `1e9 / cap` timeline, which averages the cap on vblank boundaries (45 on 60
   releases 3 of 4 vblanks); a cap above the rate, the cap's own clock. A frame
-  that arrives after its release point starts at once, and the next release is
-  the next point of the schedule, so a game below the cap keeps its own rate. A
+  that arrives after its release point starts at once, and the release after it
+  is never more than one grid instant after it, so a game below the cap keeps its
+  own rate. A
   gate on the shim's own `1e9 / N` clock, as before 7bdcbae, beat against the
   panel: 57-59 fps on `SurfaceFlinger` at a 60 cap on a 59.97 Hz panel.
 
@@ -204,7 +210,7 @@ divisor cap reads its target exactly.
   `present mode: declared=… requested=… source=env|guest|cap-rate|cap-compat
   route=gamescope cap=… gate=vsync|clock|off lsfg=…` (`source=guest`: the guest's
   own mode, no env or a FIFO request with no cap; `source=cap-rate`: declared FIFO
-  because the cap is the display's rate, with `gate=vsync`; `source=cap-compat`:
+  because the cap is the display's rate, with `gate=off`; `source=cap-compat`:
   declared FIFO for want of the stamp lead, with `gate=off`; `gate=clock` is frame
   generation's source rate or a cap above the display's rate). With
   `GNDRM_LOG_VKSHIM=1` the same line repeats every 10 s with the counters a device

docs/15-fps-limiter-pacing-and-vsync.md

@@ -22,10 +22,10 @@ fifo, or no env and a FIFO guest), gamescope's limiter passes every
 and holds the guest's commits to it. Declared mailbox, immediate or FIFO-relaxed (mailbox
 is the container default), the limiter does not hold them and the shim's acquire gate
 releases the guest's frames on the display's vblank grid. A container in one of those
-modes under a cap at the display's rate is declared FIFO with the gate kept
-(`source=cap-rate`): gamescope latches one commit per vblank at its own wakeup, which the
-shim cannot see under mailbox (`docs/10`), and the gate keeps the FIFO queue about one
-frame deep. Under frame generation the app
+modes under a cap at the display's rate is declared FIFO with no gate
+(`source=cap-rate`), as on master: gamescope latches one commit per vblank at its own
+wakeup, which the shim cannot see under mailbox, and its limiter (divisor 1) paces the
+guest (`docs/10`). Under frame generation the app
 disarms gamescope's limiter and the shim's time gate holds the source rate instead
 (`docs/17`).
 
@@ -128,8 +128,8 @@ that rate is the display's.
   previous buffer back with `wl_buffer.release`, which is what frees the guest's next
   acquire; a cap that is not the display's rate over a whole number runs at gamescope's
   next divisor rate above it. Declared FIFO because the cap is the display's rate
-  (`source=cap-rate`), the gate below also runs, so the guest starts at most one frame
-  per vblank and the FIFO list stays about one frame deep. Declared anything else,
+  (`source=cap-rate`), no gate runs either: gamescope's FIFO latch alone paces it.
+  Declared anything else,
   gamescope takes the newest commit at each composite and the shim's acquire gate is
   the cap (`icd_shim/src/vsync_gate.rs`).
   Its release points are the instants the flip events are stamped at: the daemon's
@@ -140,7 +140,8 @@ that rate is the display's.
   the next composite. A cap at the display's rate releases every vblank, a divisor cap
   every `refresh / cap`-th, any other cap below the rate the vblank nearest each instant
   of an ideal `1e9 / cap` timeline; a frame late for its point starts at once and the
-  schedule goes on, so a guest below the cap is never quantized to a divisor; a phase
+  release after it is never more than one grid instant later, so a guest below the cap
+  is never quantized to a divisor; a phase
   push never lets two releases into one vblank. The gate this replaces ran on the shim's
   own clock (exactly 1e9 / N ns) and beat against the display's 59.97-59.99 Hz
   (57-59 fps at a 60 cap). A cap above the rate runs on the cap's own clock
@@ -167,7 +168,7 @@ that rate is the display's.
   the mailbox default runs, with no cap, FIFO for a guest that asked for FIFO (its
   own VSync, `source=guest`) and mailbox for any other guest, and mailbox capped by the shim's
   vblank-grid gate under a cap below the display's rate. Under a cap at the display's
-  rate it is declared FIFO with the gate kept (`source=cap-rate`), because the shim
+  rate it is declared FIFO with no gate (`source=cap-rate`), because the shim
   cannot keep two mailbox commits out of one gamescope composite (`docs/10`). One set
   to fifo runs FIFO under gamescope's limiter.
 - **A game below the cap keeps its own rate under mailbox** (caps below the display's
@@ -227,8 +228,9 @@ measures — the latch grid to the presenter, the vsync timeline in the flip eve
 Retroid Pocket 6 (59.97-59.98 Hz), DTD title scene, cap 60, against master's FIFO
 declaration on the same scene (59.22 fps, 1.3 % doubles):
 
-- **A cap at the display's rate, declared FIFO with the vblank-grid gate**
-  (`source=cap-rate gate=vsync`): in-game VSync off, 59.199 fps on `SurfaceFlinger`
+- **A cap at the display's rate, declared FIFO** (measured with the vblank-grid gate
+  kept, `source=cap-rate gate=vsync`, before the gate was dropped from this cell;
+  without it the cell is master's FIFO path): in-game VSync off, 59.199 fps on `SurfaceFlinger`
   and 1.3 % doubles over 60 s, shim commits equal to the daemon's publishes within
   0-2 per 10 s (the guest paces itself at ~59.3); VSync on, 59.983 fps and no
   doubles.

icd_shim/src/proxy.rs

@@ -2007,21 +2007,26 @@ unsafe extern "C" fn gn_vkCreateSwapchainKHR(
     // is declared FIFO (`live_present_mode`), so the guest's own request
     // decides it, as it did before the container's mode was honoured there. A
     // lead that arrives later changes the declaration on the next present but
-    // not the count, which holds until the guest recreates the swapchain. A
-    // cap at the display's rate declares FIFO but keeps this count: the acquire
-    // gate holds the guest to one frame per vblank, so the spare image does not
-    // deepen gamescope's FIFO queue. With no cap a guest that asked for FIFO
-    // over the container's default is declared FIFO (`choose_live_mode`), so
-    // its own request decides the count too, as on master.
-    let uncapped_vsync_request = mode_source == ModeSource::Env
-        && crate::scanout::cap_fps_note() == gndrm_uapi::wire::FPS_UNCAPPED
-        && matches!(
-            guest_present_mode(ci),
-            VK_PRESENT_MODE_FIFO_KHR | VK_PRESENT_MODE_FIFO_RELAXED_KHR
-        );
+    // not the count, which holds until the guest recreates the swapchain.
+    // Whenever the swapchain is declared FIFO at creation for another reason
+    // than its own mode (no stamp lead yet, a cap at the display's rate, a
+    // guest's FIFO request with no cap), the guest's own request decides the
+    // count too, as on master: a spare image would only deepen gamescope's FIFO
+    // queue.
+    let declared_at_creation = choose_live_mode(
+        mode,
+        mode_source,
+        guest_present_mode(ci),
+        crate::lsfg::live().0,
+        crate::scanout::cap_fps_note(),
+        crate::vsync_gate::current_grid(),
+    )
+    .1;
     let slot_mode = if gamescope_route
-        && (crate::vsync_gate::current_grid().is_none() || uncapped_vsync_request)
-    {
+        && matches!(
+            declared_at_creation,
+            Declared::CapCompat | Declared::CapRate | Declared::GuestVsync
+        ) {
         guest_present_mode(ci)
     } else {
         mode
@@ -6347,13 +6352,11 @@ mod tests {
             gamescope_route_pacing((f, Declared::CapCompat), None, 60, None),
             Pacing::Off
         );
-        // FIFO at the display's rate: the gate stays, one release per vblank.
+        // FIFO at the display's rate: no gate either; gamescope's limiter
+        // (divisor 1) paces it, as on master.
         assert_eq!(
             gamescope_route_pacing((f, Declared::CapRate), None, 60, grid),
-            Pacing::Grid {
-                grid: g,
-                interval_ns: 16_675_004
-            }
+            Pacing::Off
         );
         // Mailbox, immediate, FIFO-relaxed under a cap: the gate, on the grid.
         for mode in [
@@ -6430,10 +6433,10 @@ mod tests {
         let off = Pacing::Off;
         // Capped, env mailbox, whatever the guest asked for:
         for guest in [f, m, i] {
-            // 60 on 59.97 Hz: FIFO, the gate every vblank.
+            // 60 on 59.97 Hz: FIFO, no gate (gamescope's limiter).
             assert_eq!(
                 row(Some("mailbox"), guest, 60, None, g60),
-                (f, "cap-rate", every(g60, 1))
+                (f, "cap-rate", off)
             );
             // 60 on 120 Hz, 30 on 60: mailbox, the gate every second vblank.
             assert_eq!(
@@ -9504,16 +9507,18 @@ enum Declared {
 ///   vblank-grid gate is not run without the lead: released on the latch
 ///   points themselves, frames straddle gamescope's composite and are
 ///   replaced in its mailbox. The census reads `source=cap-compat`.
-/// - a cap at the display's rate and a mode other than FIFO: FIFO, with the
-///   acquire gate kept on the vblank grid (`source=cap-rate`). At that cap one
+/// - a cap at the display's rate and a mode other than FIFO: FIFO, with no
+///   acquire gate (`source=cap-rate`), as on master. At that cap one
 ///   frame per vblank is all the panel shows under either mode, and under
 ///   mailbox the shim would have to keep two commits out of one gamescope
 ///   composite without knowing when gamescope latches: its wakeup moves with
 ///   its rolling draw time (vblankmanager.cpp). A guest that paces itself just
 ///   below the display's rate had ~1 frame a second replaced in gamescope's
 ///   mailbox that way. Under FIFO gamescope latches one commit per vblank at
-///   its own wakeup, its limiter's divisor is 1, and the gate keeps the guest
-///   on the grid so the FIFO queue stays about one frame deep.
+///   its own wakeup and its limiter (divisor 1) paces the guest. The gate is
+///   not kept on top: FIFO's back-pressure already holds the guest, and the two
+///   stacked turned a guest whose acquire-to-commit grew past a period into a
+///   40 fps session on a 60 Hz panel.
 /// - no cap, the container's env naming a mode other than FIFO and a guest
 ///   that asked for FIFO or FIFO-relaxed: the guest's mode (`source=guest`);
 ///   see [`choose_live_mode`].
@@ -9613,29 +9618,25 @@ unsafe fn acquire_pacing(sc: *mut GnSwapchain) -> crate::vsync_gate::Pacing {
 /// - frame generation sets the source rate: the gate holds it on the cap's own
 ///   clock (the app disarms gamescope's limiter then; generation paces the
 ///   output on the latch grid itself);
-/// - a FIFO swapchain, or FIFO for want of the stamp lead: no gate.
-///   gamescope's limiter holds FIFO commits, one per limited vblank, and is
-///   the cap;
-/// - FIFO because the cap is the display's rate: the gate, on the vblank grid,
-///   one release per vblank, so the guest never gets ahead of gamescope's FIFO
-///   latch and its queue stays about one frame deep;
+/// - any FIFO declaration (a FIFO swapchain, FIFO for want of the stamp lead,
+///   FIFO because the cap is the display's rate): no gate. gamescope's limiter
+///   holds FIFO commits, one per limited vblank, and is the cap;
 /// - mailbox, immediate, FIFO-relaxed: gamescope's limiter does not hold them
 ///   (`wlserver_surface_is_fifo` is FIFO only), so the gate is the cap, on the
 ///   display's vblank grid (`crate::vsync_gate`).
 fn gamescope_route_pacing(
-    (live, why): (VkPresentModeKHR, Declared),
+    (live, _): (VkPresentModeKHR, Declared),
     source_cap: Option<u32>,
     cap_fps: u32,
     grid: Option<crate::vsync_gate::Grid>,
 ) -> crate::vsync_gate::Pacing {
     if let Some(cap) = source_cap {
         return crate::vsync_gate::Pacing::on_clock(cap);
     }
-    match why {
-        Declared::CapCompat => crate::vsync_gate::Pacing::Off,
-        Declared::Mode if live == VK_PRESENT_MODE_FIFO_KHR => crate::vsync_gate::Pacing::Off,
-        _ => crate::vsync_gate::pacing(cap_fps, grid),
+    if live == VK_PRESENT_MODE_FIFO_KHR {
+        return crate::vsync_gate::Pacing::Off;
     }
+    crate::vsync_gate::pacing(cap_fps, grid)
 }
 
 /// The mode a commit on `sc` declares to gamescope, counted for the census.

icd_shim/src/vsync_gate.rs

@@ -245,14 +245,19 @@ impl Schedule {
     /// read on the grid as it is now: the daemon re-pushes the phase as it
     /// moves, and a point booked on the old phase must not let two releases
     /// into one period of the new one. Starts later than [`ON_TIME_NS`] after
-    /// their point are counted (`gate_late`), with the points passed (`skipped`).
+    /// their point are counted (`gate_late`), with the points passed (`skipped`),
+    /// and the release after a late start is never more than one grid instant
+    /// after it.
     pub(crate) fn book(&mut self, now_ns: u64, grid: Grid, interval_ns: u64) -> u64 {
         if grid.period_ns == 0 {
             return self.book_clock(now_ns, interval_ns);
         }
         let at = grid.at_or_before(now_ns);
         let due = (self.next_ns != 0).then(|| grid.nearest(self.next_ns));
-        if let Some(due) = due.filter(|&d| now_ns >= d.saturating_add(ON_TIME_NS)) {
+        // Late against the instant the acquire was released for, which after a
+        // clock booking is off the grid.
+        let late = due.filter(|_| now_ns >= self.next_ns.saturating_add(ON_TIME_NS));
+        if let Some(due) = late {
             LATE_N.fetch_add(1, Ordering::Relaxed);
             SKIPPED_N.fetch_add(at.saturating_sub(due) / grid.period_ns, Ordering::Relaxed);
         }
@@ -262,9 +267,17 @@ impl Schedule {
         };
         let ideal_next = ideal.saturating_add(interval_ns);
         // Never two releases on one grid instant.
-        let next = grid
+        let mut next = grid
             .nearest(ideal_next)
             .max(slot.saturating_add(grid.period_ns));
+        let mut ideal_next = ideal_next;
+        if late.is_some() && next > at.saturating_add(grid.period_ns) {
+            // A late start already cost the guest time: the release after it
+            // is the next grid instant after it, never a whole slot further,
+            // and the timeline goes on from there.
+            next = at.saturating_add(grid.period_ns);
+            ideal_next = next;
+        }
         self.ideal_ns = ideal_next;
         self.next_ns = next;
         next
@@ -510,15 +523,44 @@ mod tests {
     }
 
     #[test]
-    fn a_late_frame_on_a_divisor_cap_waits_for_no_more_than_the_schedule() {
-        // 30 on 60, the guest late by most of a vblank: it starts at once, and
-        // the one after it is due at the slot after the one it was given.
+    fn a_late_frame_on_a_divisor_cap_waits_for_no_more_than_the_next_grid_instant() {
+        // 30 on 60, the guest late by half a vblank: it starts at once, and the
+        // release after it is the next grid instant, not a whole slot later.
         let g = grid(P60);
         let iv = interval(pacing(30, Some(g)));
         let mut s = Schedule::default();
         let due = s.book(g.anchor_ns, g, iv);
         assert_eq!(due, g.anchor_ns + 2 * P60);
-        assert_eq!(s.book(due + P60 / 2, g, iv), due + 2 * P60);
+        assert_eq!(s.book(due + P60 / 2, g, iv), due + P60);
+        // An on-time guest is still held to every second vblank.
+        let next = s.next_ns;
+        assert_eq!(s.book(next + 50_000, g, iv), next + 2 * P60);
+    }
+
+    #[test]
+    fn a_guest_with_16_ms_of_work_is_never_skipped() {
+        // 16 ms from acquire to the next acquire on a 59.97 Hz grid, and every
+        // seventh frame 1.5 ms slower (late for its point): the late frames
+        // start at once, the release after each is the next grid instant, and
+        // every vblank gets a start.
+        let g = grid(P60);
+        let iv = interval(pacing(60, Some(g)));
+        let mut s = Schedule::default();
+        let mut t = g.anchor_ns;
+        let mut starts = Vec::new();
+        for n in 0..600 {
+            if s.next_ns != 0 && t < s.next_ns {
+                t = s.next_ns + 50_000;
+            }
+            starts.push(t);
+            s.book(t, g, iv);
+            t += if n % 7 == 6 { 17_500_000 } else { 16_000_000 };
+        }
+        let v = vblanks(g, &starts);
+        assert!(
+            v.windows(2).all(|w| w[1] == w[0] + 1),
+            "a vblank was skipped"
+        );
     }
 
     /// One test for the process-global grid, so no other test races it.