Skip to content

pkg_upload_ makes the no-copy upload source once - #2342

Closed
dean0396 wants to merge 1 commit into
seedds:mainfrom
dean0396:pr-upsrc
Closed

dean0396 wants to merge 1 commit into
seedds:mainfrom
dean0396:pr-upsrc

Conversation

@dean0396

Copy link
Copy Markdown
Contributor

What and why

pkg_upload_ asks up_big_src for a texture's no-copy source buffer twice for nearly every staged texture that reads a large pool no table holds whole, or a pool no GPU address was given out in. The first call comes before the staging allocation. The second comes right after it and always runs, in case up_alloc flushed the batch that held the first buffer. up_alloc only flushes when the batch's staging would pass UP_BATCH_BYTES, so the second buffer is almost always a duplicate over the same pages in the same open batch. Each duplicate is one more residency add, and one more remove when the batch completes.

My fork's build has the same two calls but not main's big_full check, so it also made source buffers over large pools the raw table holds whole. On it, the residency site in up_big_src added 466 buffers (1,324 MB) for about 233 textures (662 MB) on the first turn into Kakariko in Ocarina of Time, and 482 buffers (1,395 MB) in Castle Town. Those counts can include textures in large pools held whole, which main leaves out, so main probably makes fewer there. How many duplicates main makes in a game is not measured.

Change:

  • pkg_upload_ keeps g_up_seq after the first call. It calls up_big_src again only when that value changed, that is, when up_alloc flushed the batch and began a new one (up_begin is the only writer of g_up_seq).
  • DM_NVN_UPSRC_DEDUPE=0 brings back the two calls, with a row in docs/env_switches.tsv.
  • The upload staging blocks line also counts the source buffers made, those made by the second call, and the second calls left out.
  • New GPU test t_upload_src with its Makefile rule (check_t_upload_src runs it with the switch on and off).
  • One sentence added to the "Memory pools nothing gave an address to stay out of the raw pool table" entry in docs/reference/memory.md.

Main already has the big_full check in up_big_src (no source buffer over a large pool that raw global memory's table holds whole), so this only touches the second call.

Checked

  • Tests (M3 Max, macOS 26.7.2, run on main 352e4a6; the branch is now rebased onto 8b55ebb, which changes only the shader build tools and docs, and t_upload_src gives the same 132 / 95 checks there; make -C dm/gfx/tests GAME=0400486024DEC000 check_<test>):
    • t_upload_src: 132 checks, 0 failed with the default; 95 checks, 0 failed with DM_NVN_UPSRC_DEDUPE=0.
      • A fresh batch makes 1 buffer and 1 residency add at up_big_src's site.
      • A batch that the texture's own staging must flush makes 2 buffers, one in each batch, and both batches complete.
      • With the switch off, a fresh batch makes 2.
      • A large pool held whole makes none, with the switch on or off.
      • Every texture reads back equal to the CPU reference.
      • Negative controls: I built the test against the change with the second call made unconditional again (main's behaviour), and 12 checks fail: two buffers in every fresh batch. With the second call never made, 16 fail: the flushed-batch and switch-off cases.
    • t_pool_chunks, t_upload_slots, t_upload_budget, t_direct_upload, t_cpu_tex, t_pool_retire, t_deswz_gpu, t_initial_uncached, t_gmem_words and t_bc_alias pass with the default and with DM_NVN_UPSRC_DEDUPE=0. The results match unchanged main.
    • The whole make -k run suite in dm/gfx/tests gives the same results on the branch as on unchanged main, apart from the new t_upload_src. One check, t_feedback's "with nothing running: a commit", fails on both trees: in main's suite run (the branch's passed), and in 5 of 6 reruns on the branch and 6 of 6 on main.
    • The Mac host (dmhost, GAME=0400486024DEC000) builds with the same warnings as main.
    • dm/tools/tests/test_docs.py passes. test_env_switches.py fails with identical output on unchanged main (four switches quoted in the docs but read nowhere, and DM_NVN_VCONV_KEEP has no row). The new switch has its row.
  • Games run: none. I didn't run Ocarina of Time on this branch or on main. The numbers above are from my fork's build, before the change; I have no after measurement from a game.
  • Not run: scripts/regress.py check (Ocarina of Time is the only game set up on this Mac, and I didn't run it), and lint_c.py (no LLVM 23 here).

Overlapping PRs (checked against main 352e4a6): 750 PRs were open, and 367 of them touched a file this PR changes (nvn_mtl.m 219, docs/env_switches.tsv 230, dm/gfx/tests/Makefile 99, docs/reference/memory.md 41, t_upload_src.m none). I dry-ran the gh pr diff of all 367 with git apply --check on main and on this branch, whole and file by file. I also merged each PR's version of each shared file (its base blob plus its diff) onto main's and this branch's version with git merge-file, and compared which functions each PR changes. Whole diffs: 83 apply on both, 282 on neither, 2 on main only. Since then, #2339 and #2340 change nvn_mtl.m (#2340 also docs/env_switches.tsv); both apply on main and on this branch.

Apply on main but not on this branch (4):

Change the same functions (up_diag, up_big_src, pkg_upload_, or the declarations and comments just before up_diag and up_big_src): 18 PRs, counting #1599, #2263 and #2270 above.

No open PR changes the docs/reference/memory.md entry this PR edits, and none adds a t_upload_src.m. All 99 dm/gfx/tests/Makefile parts give the same result on this branch as on main.

Docs

  • The finding is in its docs/reference/memory.md entry, and python3 dm/tools/docs_symptoms.py was run (the index is unchanged)
  • No game files, keys or data extracted from a game in the change or the attachments

… if its staging allocation flushed the batch

pkg_upload_ made up_big_src's no-copy source buffer twice for nearly every staged texture read from a large pool that no table holds whole, or from a pool no GPU address was given out in: two buffers over the same pages, each added to the residency set and removed again when its batch completes. The second call, right after up_alloc, ran unconditionally, in case up_alloc had flushed the batch that held the first buffer (that batch retires it with its staging), but up_alloc flushes only when the batch's staging would pass UP_BATCH_BYTES, so the second buffer was nearly always a duplicate in the same open batch. On a fork's build with the same two calls, the first turn into Ocarina of Time's Kakariko added 466 buffers, 1,324 MB, at up_big_src's residency site for about 233 textures (662 MB); Castle Town added 482 buffers, 1,395 MB. That build also lacked main's big_full check, so these counts can include buffers over large pools the raw table holds whole, which main does not make; main's count in those places is not measured.

pkg_upload_ now notes g_up_seq after the first call and calls up_big_src again only when it changed, that is, when up_alloc flushed the batch and began another (up_begin is the only writer of g_up_seq). DM_NVN_UPSRC_DEDUPE=0 calls it twice, as before. The 'upload staging blocks' line also counts the source buffers made, those made by the second call, and the second calls left out. The big_full check in up_big_src (no source buffer over a large pool raw global memory's table holds whole) is already on main and is unchanged.

Checked on an M3 Max, macOS 26.7.2, on main 352e4a6, and t_upload_src again after a rebase onto 8b55ebb (which changes only the shader build tools and docs), with the same results: the new t_upload_src passes, 132 checks with the default and 95 with DM_NVN_UPSRC_DEDUPE=0. A fresh batch makes one buffer and one residency add at up_big_src's site; a batch the texture's own staging must flush makes two, one in each batch, and both complete; the switch off makes two; a large pool held whole makes none either way; every texture reads back equal to the CPU reference. With the second call left unconditional, as on main, the default run fails 12 checks (two buffers in every fresh batch); with the second call never made, it fails 16 (the flushed-batch and switch-off cases). t_pool_chunks, t_upload_slots, t_upload_budget, t_direct_upload, t_cpu_tex, t_pool_retire, t_deswz_gpu, t_initial_uncached, t_gmem_words and t_bc_alias pass with the default and with DM_NVN_UPSRC_DEDUPE=0, the same results as on unchanged main. The whole make run suite in dm/gfx/tests gives the same results as on main apart from the new test; one check, t_feedback's 'with nothing running: a commit', fails on both trees (in main's suite run, and in 5 of 6 reruns on the branch and 6 of 6 on main). The host builds with the same warnings as main. test_docs passes; test_env_switches fails with the same output as on main (four switches quoted in the docs but read nowhere, and DM_NVN_VCONV_KEEP has no row). Not run: a game, regress.py, lint_c.py.
@github-actions github-actions Bot added nvn NVN on Metal and its tests (dm/gfx/nvn, dm/gfx/tests) docs Documentation labels Nov 11, 2026
@stan69b

stan69b commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Hey, I cleanup my PR to handle the conflicts with main : #2270

it should now merge if you need my PR in yours for testing.

also check this one, The iPhone PR is a follow up of that PR : #1971

seedds added a commit that referenced this pull request Nov 12, 2026
… buffer only when its batch changed (PR #2378)

pkg_upload_ reads a texture in a large pool (BIG_MIN), or in a pool no
address was given out in, through a zero-copy buffer of the upload batch
(up_big_src). After taking the texture's staging (up_alloc), which may
flush the batch, it made a second buffer for every texture, flush or not,
so each staged upload put its storage into the residency set twice.

It now records the batch (g_up_seq) when it makes the first buffer and
makes a second only when the batch changed.

- Switch Sports Resort's first court frame (855 textures, 908 MB, M2 Max,
  PR #2378): up_big_src buffers 2,852-2,858 -> 1,429-1,437, residency
  commits 678-692 -> 520-523 ms, the frame 263-270 -> 243-251 ms.
- Mario Kart World, 200 s regress route (M4 Pro): up_big_src buffers
  24,696 (5,429 MB) -> 12,082 (2,424 MB); 60.0 fps and frame p99
  unchanged.
- t_upload_src: one buffer per texture in one batch, one more for a
  texture whose staging flushed its batch, every texel checked under
  Metal validation; main's code fails 2 of its 41 checks.
- regress.py check: Mario Kart World and OCTOPATH TRAVELER II pass
  (Octopath does not reach up_big_src).

Same cause found earlier in PR #2342.

Co-authored-by: bymaxmarc <354601562+bymaxmarc@users.noreply.github.com>
@seedds

seedds commented Nov 12, 2026

Copy link
Copy Markdown
Owner

Thanks: you found this cause first. The fix landed on main as 0c23e0b, from #2378, with the batch recorded right after the first up_big_src call, as you did. It has no switch: going back to the duplicate buffer has no use, so DM_NVN_UPSRC_DEDUPE wasn't needed. The extra counters stayed out too, and this PR no longer applied to main. On Mario Kart World, up_big_src buffers over a 200 s run went from 24,696 (5,429 MB) to 12,082 (2,424 MB).

@seedds seedds closed this Nov 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation nvn NVN on Metal and its tests (dm/gfx/nvn, dm/gfx/tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants