Repository navigation
android: A58 — under the FPS limiter, hold a second mailbox commit for the next vblank - #31
Closed
utkarshdalal wants to merge 2 commits into
Closed
utkarshdalal wants to merge 2 commits into
utkarshdalal wants to merge 2 commits into
Conversation
…r the next vblank A mailbox client pacing itself just under the refresh rate gets two commits into one composite window about once a second, and the newer one replaces the older before the paint, so the panel repeats a frame. Under the limiter, with gn_mailbox_hold on (default on gamenative-drm), the second commit is held and latched at the next vblank; a third replaces the held one. Test build for the mailbox-at-display-rate study; see the patch header.
… gn_mailbox_hold_max_carry paints With in-game VSync on the client delivers one commit per vblank, so once a commit was held every later one found its paint taken by the carried commit and was carried in turn: a standing one-frame delay that frame rate cannot show. After gn_mailbox_hold_max_carry paints in a row (default 8, 0 = unbounded, property debug.gamescope.gn_mailbox_hold_max_carry) a fresh commit is latched at once and replaces the carried one. The report counted a held commit once per compositor pass it stayed held (twice per vblank); it now counts each deferral once and reports carried_paints, max_run and collapsed alongside replaced.
Author
|
Closing. The measurements this PR produced argue against shipping it.
So the one case this patch addresses is already served by FIFO plus the gate, and gamenative-drm#46 keeps that rule. The branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branch
gn-mailbox-hold-under-limiter(9fad070), basegamenative. Adds one patch,android/patches/gamescope/0054-A58-mailbox-hold-under-limiter.patch. The number is 0054 because 0052 belongs to #25 (A56) and #23 (A54), and 0053/A57 to the closed #30. A58 applies over the 0001-0051 series alone. CI build: run 41567798845 (9fad070), green. Binary sha2564feeadc116518bd7ff2c4f158022f054106f8607d0f451bf6d4ff36560f7bd94. H1 and H2 below ran on the first revision, 52e1b1f (47d8fe6c…, run 41564086468), which had no carry bound. The baseline is the shipped65d024fc…, which is the CI build of the same series without A58.Pairs with GameNative/gamenative-drm#46.
Problem
Under the FPS limiter, gamescope's mailbox merges a second commit into one composite window. A guest pacing itself just under the refresh rate hits this about once a second. The case here is DTD with in-game VSync off: Unity targets ~59.3 fps, the cap is 60 and the panel runs at 59.95-59.98 Hz.
handle_done_commits_xwayland/handle_done_commits_xdg).HeldCommits[BASE]. The first is released and its presentation feedback is discarded.The client cannot avoid the merge, because it cannot predict gamescope's latch point:
src/vblankmanager.cpp142-199).src/Backends/DRMBackend.cpp~4465). On gamenative-drm that includes the commit to the daemon, so it is spiky.presented/discarded, one inequality per frame.Only gamescope knows its latch point, so the hold has to live in gamescope.
Change
All of this is gamenative-only, behind the convar
gn_mailbox_hold. It applies only while the FPS limiter is engaged for the window: a target FPS is set,steamcompmgr_window_should_limit_fpsis true and VRR is off. For non-FIFO done commits:commit_queue;~commit_tthen releases its buffer and sendsdiscarded, exactly as a mailbox replace does today.Properties:
The carry is bounded (9fad070)
A held commit is shown one paint late. A client that keeps delivering exactly one commit per vblank never drains that delay:
So after
gn_mailbox_hold_max_carrypaints in a row (default 8, about 133 ms) have shown a carried commit, the next fresh commit is latched at once and replaces the carried one:0makes the carry unbounded, which is the 52e1b1f behaviour.1latches every fresh commit at once whenever its paint holds a carried one. That keeps no carry across a second paint, which makes the hold close to plain mailbox for a client near the refresh rate.Implementation:
gnMailboxLatchedForPaint(steamcompmgr_shared.hpp), records which paint the window's last non-FIFO commit was latched for. It holdsvblank_idx + 1;vblank_idxadvances after the vblank pass's done-commit handling, so every pass with the same index feeds the same paint.gn_mailbox_hold_entry()is called from both done-commit handlers, after the FIFO check.Switches
gn_mailbox_hold: -1 = auto, the default (on whenGAMENATIVE_DRM_SOCKETis set), 0 = off, 1 = on.gamescope_gn_mailbox_hold=0/1overrides it through the existing convar env mapping.debug.gamescope.gn_mailbox_hold=0/1wins when set. It is settable with an unrootedadb shell setprop.gn_mailbox_hold_max_carry(default 8, 0 = unbounded) follows the same pattern: envgamescope_gn_mailbox_hold_max_carry, propertydebug.gamescope.gn_mailbox_hold_max_carry.Logs (xwm scope)
gn_mailbox_hold=<0|1> source=<default|env|prop> max_carry=N.gn_mailbox_hold: deferred=N carried_paints=C max_run=R collapsed=X replaced=M over K vblanks.deferred: commits held for a later paint, each counted once.carried_paints: paints that showed a commit one paint late. This is the latency the hold adds.max_run: the longest run of carried paints in a row.collapsed: carries ended by the bound.replaced: held commits dropped for a newer one.The 52e1b1f counter
52e1b1f logged
held=N, counting a held commit once for every compositor pass in which it stayed held. A held commit stays held for at least two passes: the pass it completes in, and the vblank pass of the paint it was held from. It is latched by the first pass aftervblank_idxadvances.So H2's steady
held=1200 over 600 vblanksmeans every one of the ~600 commits was deferred once. That is a real one-paint delay on every frame, not a counting artefact. Likewise, H1'sheld=38-164means about 19-82 deferrals per 600 vblanks.There is no off-by-one on the marker. It is set to
vblank_idx + 1at latch, and every pass after the vblank pass computesvblank_idx + 2, so the first commit of a new paint is never mistaken for an already-latched one. It was the carried commit that took the paint.Device verification (Retroid Pocket 6)
H1 setup:
requested=immediate), cap 60, 59.95 Hz panel.b084eadand its test switchdebug.gndrm.mailbox_at_rate=1, which declares mailbox at the display's rate with the vblank-grid gate kept.debug.gamescope.gn_mailbox_hold=1.H1 result:
declared=mailbox requested=immediate source=env-at-rate cap=60 gate=vsync,declared_fifo=0.gn_mailbox_hold=1 source=prop.held=38-164 replaced=0per 600 vblanks, on the 52e1b1f counter. All 5 replacements in the run happened during loading.H2 setup: the same stack (52e1b1f), with in-game VSync on (
requested=fifo) and a 59.98 Hz panel.H2 result:
declared=mailbox requested=fifo source=env-at-rate cap=60 gate=vsync.gate_holds+600,late0-4,skipped0,slot_waits0,commit_lat_us~6.0 ms.late=0, margin p50 +3.45 ms, lead ~2.2 ms.gn_mailbox_hold=1 source=prop,replaced=0in steady state.held=1200per 600 vblanks from the fourth window on, meaning every commit was carried one paint (see above). 9fad070 bounds this.slot_waits=0.carried_paintsnear 0 after onecollapsed)The remaining shortfall from 59.95 is the game's own ~59.3 fps self-pacing, which is the same as under master's FIFO.
Buffer pressure
With the hold, a window can have one buffer on screen, one latched and one held. A client with only three swapchain images could then wait for a release now and then. In H1 the shim's
slot_waitsstayed at 0. Clients that render directly to Xwayland with a three-image swapchain have not been measured.Pairing with gamenative-drm GameNative/gamenative-drm#46
GameNative/gamenative-drm#46 makes the shim declare the container's present mode on the override route. Its one exception is a cap at the display's rate, where it declares FIFO because the shim alone cannot keep two commits out of one composite. With A58 in the payload, that exception can be dropped: gamescope and the shim ship in the same archive. Until then, GameNative/gamenative-drm#46 keeps the exception by default, and
debug.gndrm.mailbox_at_rateturns on mailbox at the display's rate for testing.@joshuatam, I'd value your read on the hold point. Is latching the held commit at the next cycle's first done-commit pass right, and should the hold also cover windows that are not the focus base plane?