MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vk): keep mesh indirect argument ranges from overlapping #2719

Open

github-sync wants to merge 2 commits from github/pr/2719 into main

github-sync commented

2026-11-12 09:31:55 +00:00

Owner

@juliotec opened this pull request on GitHub · 2026-11-12 09:08 UTC · original

mesh_args_take hands out n consecutive entries of the mesh-argument ring with
next.fetch_add(n) % (kMeshArgs - n). The modulus changes with n, so two draws
taken close together with different counts could be given overlapping entries
and overwrite each other's arguments.

Without RX_VK_MESH_ARGS (the default), mesh_args_take is still called, but only
from direct mesh draws with a non-zero first task, always with n = 1. A constant
n means a constant modulus, so nothing overlaps there; this change keeps that
path working the same. The overlap needs mixed counts, which only come from the
rewritten indirect draws enabled by RX_VK_MESH_ARGS=1 (off by default). With the
flag on, it can show up as wrong, flickering or missing mesh geometry.

What changed (gpu/vk/vk_draw.cpp, mesh_args_take and its indirect caller):
the modulus is fixed at kMeshArgs / 2 and n is clamped to at most kMeshArgs / 2,
so every range fits in the ring without wrapping and ranges handed out in a row
do not overlap until the ring comes around. kMeshArgs is doubled (1 << 19, 8 MB
instead of 4 MB) so an entry is still reused 262144 draws later, as before; a
modulus of half the old size alone would have halved that distance.

Cost: the ring buffer is host-visible and is also created for direct mesh draws
with a non-zero first task (vk_draw.cpp:6959), flag or not, so the extra 4 MB
applies to everyone who reaches that path. The RX_VK_LOG_MESH_PERPROG
diagnostic prog_at vector doubles too.

tests/mesh_args_cleanup_test.cpp now requires the allocation to be exactly
kMeshArgs * 16 bytes, derived from the extracted kMeshArgs, instead of the old
4 MB bound.

How tested: release build; Zelda OoT on Radeon RX 9070 XT (Windows,
Adrenalin), both with the flag off (the default, the n = 1 direct-draw path) and
with RX_VK_MESH_ARGS=1 (the rewritten indirect draws, mixed counts); tests/test_mesh_args_cleanup.py passes its 5 cases.

Assisted-by: GitHub Copilot

> [@juliotec](https://github.com/juliotec) opened this pull request on GitHub · 2026-11-12 09:08 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2719) mesh_args_take hands out n consecutive entries of the mesh-argument ring with next.fetch_add(n) % (kMeshArgs - n). The modulus changes with n, so two draws taken close together with different counts could be given overlapping entries and overwrite each other's arguments. Without RX_VK_MESH_ARGS (the default), mesh_args_take is still called, but only from direct mesh draws with a non-zero first task, always with n = 1. A constant n means a constant modulus, so nothing overlaps there; this change keeps that path working the same. The overlap needs mixed counts, which only come from the rewritten indirect draws enabled by RX_VK_MESH_ARGS=1 (off by default). With the flag on, it can show up as wrong, flickering or missing mesh geometry. What changed (gpu/vk/vk_draw.cpp, mesh_args_take and its indirect caller): the modulus is fixed at kMeshArgs / 2 and n is clamped to at most kMeshArgs / 2, so every range fits in the ring without wrapping and ranges handed out in a row do not overlap until the ring comes around. kMeshArgs is doubled (1 << 19, 8 MB instead of 4 MB) so an entry is still reused 262144 draws later, as before; a modulus of half the old size alone would have halved that distance. Cost: the ring buffer is host-visible and is also created for direct mesh draws with a non-zero first task (vk_draw.cpp:6959), flag or not, so the extra 4 MB applies to everyone who reaches that path. The RX_VK_LOG_MESH_PERPROG diagnostic prog_at vector doubles too. tests/mesh_args_cleanup_test.cpp now requires the allocation to be exactly kMeshArgs * 16 bytes, derived from the extracted kMeshArgs, instead of the old 4 MB bound. How tested: release build; Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), both with the flag off (the default, the n = 1 direct-draw path) and with RX_VK_MESH_ARGS=1 (the rewritten indirect draws, mixed counts); tests/test_mesh_args_cleanup.py passes its 5 cases. Assisted-by: GitHub Copilot

github-sync added 1 commit

2026-11-12 09:31:55 +00:00

fix(vk): keep mesh indirect argument ranges from overlapping 153b353bab

mesh_args_take hands out n consecutive entries of the mesh-argument ring with
next.fetch_add(n) % (kMeshArgs - n). The modulus changes with n, so two draws
taken close together with different counts could be given overlapping entries
and overwrite each other's arguments.

Without RX_VK_MESH_ARGS (the default), mesh_args_take is still called, but only
from direct mesh draws with a non-zero first task, always with n = 1. A constant
n means a constant modulus, so nothing overlaps there; this change keeps that
path working the same. The overlap needs mixed counts, which only come from the
rewritten indirect draws enabled by RX_VK_MESH_ARGS=1 (off by default). With the
flag on, it can show up as wrong, flickering or missing mesh geometry.

What changed (gpu/vk/vk_draw.cpp, mesh_args_take and its indirect caller):
the modulus is fixed at kMeshArgs / 2 and n is clamped to at most kMeshArgs / 2,
so every range fits in the ring without wrapping and ranges handed out in a row
do not overlap until the ring comes around.

How tested: release build; Zelda OoT on Radeon RX 9070 XT (Windows,
Adrenalin) with the flag off (the default), which exercises the n = 1 direct-draw
path. Not run with RX_VK_MESH_ARGS=1, so the mixed-count case was not exercised
at runtime.

Assisted-by: GitHub Copilot

github-sync force-pushed github/pr/2719 from 153b353bab to 7810454be3

2026-11-12 10:34:11 +00:00

Compare

github-sync referenced this pull request

2026-11-12 14:44:37 +00:00

fix(vk): skip clears of textures kept in ASTC blocks #2718

github-sync commented

2026-11-12 14:44:39 +00:00

Author

Owner

@Joey-Magnus requested changes on GitHub · 2026-11-12 14:34 UTC · original

The fix is right. With % (kMeshArgs - n), takes of different n land at unrelated places in the ring, so neighbouring draws can share entries when RX_VK_MESH_ARGS=1. A fixed modulus of kMeshArgs/2 with n clamped to it keeps every range inside the buffer and keeps the reuse distance at about 262144 entries. A power-of-two modulus also stays continuous when the 32-bit counter wraps, which the old one didn't.

But it breaks a test. With this merged into current main, tests/test_mesh_args_cleanup.py fails in all 5 cases with "unexpected allocation size". The fixture tests/mesh_args_cleanup_test.cpp:35 accepts at most 4 MB, and kMeshArgs << 19 makes it 8 MB. The check passes on main. Please update that bound in the same PR, ideally derived from kMeshArgs rather than a new constant.

A note on cost: the buffer is host-visible and is also made for direct mesh draws with a non-zero first task (vk_draw.cpp:6959), flag or not, so the extra 4 MB applies to everyone who reaches that path. That seems fine, but say it in the description. The diagnostic prog_at vector in RX_VK_LOG_MESH_PERPROG doubles too.

> [@Joey-Magnus](https://github.com/Joey-Magnus) requested changes on GitHub · 2026-11-12 14:34 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2719#pullrequestreview-5876239365) The fix is right. With `% (kMeshArgs - n)`, takes of different n land at unrelated places in the ring, so neighbouring draws can share entries when RX_VK_MESH_ARGS=1. A fixed modulus of kMeshArgs/2 with n clamped to it keeps every range inside the buffer and keeps the reuse distance at about 262144 entries. A power-of-two modulus also stays continuous when the 32-bit counter wraps, which the old one didn't. But it breaks a test. With this merged into current main, tests/test_mesh_args_cleanup.py fails in all 5 cases with "unexpected allocation size". The fixture tests/mesh_args_cleanup_test.cpp:35 accepts at most 4 MB, and kMeshArgs << 19 makes it 8 MB. The check passes on main. Please update that bound in the same PR, ideally derived from kMeshArgs rather than a new constant. A note on cost: the buffer is host-visible and is also made for direct mesh draws with a non-zero first task (vk_draw.cpp:6959), flag or not, so the extra 4 MB applies to everyone who reaches that path. That seems fine, but say it in the description. The diagnostic prog_at vector in RX_VK_LOG_MESH_PERPROG doubles too.

github-sync referenced this pull request

2026-11-12 14:45:14 +00:00

fix(vk): retry sparse blocks whose bind failed #2743

github-sync added 1 commit

2026-11-12 17:57:25 +00:00

test(vk): mesh args fixture checks the ring size from kMeshArgs 58249dc00f

tests/mesh_args_cleanup_test.cpp accepted at most 4 MB, and the larger ring
(kMeshArgs 1<<19) is 8 MB. The fixture now requires exactly
kMeshArgs * 16 bytes, taken from the extracted source.

How tested: tests/test_mesh_args_cleanup.py, 5 cases pass.

Assisted-by: GitHub Copilot

github-sync commented

2026-11-12 17:58:47 +00:00

Author

Owner

@juliotec commented on GitHub · 2026-11-12 17:35 UTC · original

Thanks. Pushed a fixture fix: tests/mesh_args_cleanup_test.cpp now requires the allocation to be exactly kMeshArgs * 16 bytes, taken from the extracted kMeshArgs, instead of the 4 MB bound. tests/test_mesh_args_cleanup.py passes its 5 cases. The description now covers the extra 4 MB for the direct-draw path and the doubled prog_at vector.

> [@juliotec](https://github.com/juliotec) commented on GitHub · 2026-11-12 17:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2719#issuecomment-6831108040) Thanks. Pushed a fixture fix: tests/mesh_args_cleanup_test.cpp now requires the allocation to be exactly kMeshArgs * 16 bytes, taken from the extracted kMeshArgs, instead of the 4 MB bound. tests/test_mesh_args_cleanup.py passes its 5 cases. The description now covers the extra 4 MB for the direct-draw path and the doubled prog_at vector.

github-sync force-pushed github/pr/2719 from 58249dc00f to 120e6bdbc3

2026-11-12 19:00:51 +00:00

Compare

Commenting is not possible because the repository is archived.

No reviewers