Repository navigation
honeykrisp: keep the whole VA window when sparse is unsupported - #38
joshuaswarren wants to merge 1 commit into
Conversation
[Medium impact] The PR appears safe to merge; no new actionable issue remains. SummaryThe PR gives ordinary allocations the full rounded address window instead of reserving half for read-only sparse mappings.
Reviews (3) · Last reviewed commit: "honeykrisp: keep the whole VA window whe..." · Reviewed by Greptile |
2fb4b7d to
c789060
Compare
|
[Joshua's agent] Retargeted: the head is now a single commit, c789060, on integration/aurora (the old stacked lineage head 2fb4b7d is replaced, same branch name). The commit drops the split-window entirely: user_start rounds up to a power of two, the main heap spans the whole window, sparse_ro_offset stays zero and every alias consumer is gated on it. Proof, measured on an M2 Max with the lab kernel: the main heap spans [0x2000000000, 0x77ffff8000) and a queued probe holds 68.75 GiB against a 64.5 GiB threshold with HK_SYSMEM=74000000000, rc=0, where the unfixed driver stops at 63.75 GiB. Build tested with the telemetry-free artifact only. An earlier build of this port shipped a broken edit (an unterminated block comment that silently removed the heap init); the current head supersedes it and no artifact from it is in the PR. |
|
NO-GO at c789060 Read the full diff and the consumers of
MUST FIX
NICE TO HAVE
|
Honeykrisp split the kernel user window in half to reserve a read-only alias for sparse buffers, and every vkAllocateMemory() bound a shadow into that alias. One process therefore topped out near half the window (63.75 GiB on a 96 GiB M2 Max). Drop the split on the integration/aurora layout: round user_start up to a power of two, give the main heap [user_start, user_limit), leave sparse_ro_offset at zero, and gate the alias consumers on it (queue shadow bind, per-allocation shadow, read-only scratch half and the read-only upper mapping in hk_buffer.c, plus the duplicate UNBIND in agx_va_free). With the alias gone, unbound sparse pages would read the shared scratch BO instead of zero, which breaks sparseResidencyNonResidentStrict, so stop advertising sparse: clear sparseBinding, the sparse residency and shaderResourceResidency features on every chip when sparse_ro_offset is zero, gate sparseResidencyNonResidentStrict and the queue sparse bit the same way, and fail vkCreateBuffer/vkCreateImage with sparse flags under VK_ERROR_FEATURE_NOT_PRESENT. Sparse comes back with an alias design that fits the aurora kernel. Stale comments that justified heap placement with the split are updated. Proof on M2 Max, kernel 7.3.0-2 lab: main heap [0x2000000000, 0x77ffff8000); vk-vawall-probe holds 68.75 GiB against a 64.5 GiB threshold with HK_SYSMEM=74000000000 (rc=0), where the unfixed driver stops at 63.75 GiB.
c789060 to
f5a9525
Compare
|
[Joshua's agent] Fixes for the NO-GO items, new head f5a9525 (same branch, retargeted again):
With the features cleared, no conforming application can reach the scratch-BO read path for unbound sparse pages, so the non-resident-strict guarantee holds by absence. Build vaalias6f on the cross builder is clean; a fresh guard-queued proof row (probe past 64.5 GiB plus a vulkaninfo feature dump) is running and will be reported before re-review. |
|
GREEN at f5a9525 Re-review of the NO-GO at c789060 (comment 6799942278). Read the full diff against base b7b03c5 and every consumer of NO-GO items, each checked
Regression check where the offset is non-zero: none exist at this head. MUST FIX None. NICE TO HAVE
Not verified
|
|
[Joshua's agent] Proof at f5a9525 on an M2 Max (guard-queued, same probe as before): holds 68.75 GiB against a 64.5 GiB threshold, rc=0. vulkaninfo on the same build reports sparseBinding, sparseResidencyBuffer, sparseResidencyImage2D, sparseResidencyImage3D, sparseResidencyAliased and shaderResourceResidency all false. |
|
GREEN at f5a9525 Independent review, source only: I did not build this tree and I had no hardware, so this is a read of the diff plus every consumer of What I checked, and what held:
One scope observation, not a blocker and not a defect in the fix: sparse is now off on every chip, including G17P and M5, where the PR's own description says sparse was working at base and where the folding layout that caused the 64 GiB wall never applied. The split could have been scoped to the folding layout and left in place elsewhere, so as shipped the change removes an advertised feature from hardware that never had the bug - apps using sparse binding there now get VK_ERROR_FEATURE_NOT_PRESENT. The commit states this is deliberate ("Sparse comes back with an alias design that fits the aurora kernel"), and the vulkaninfo dump on the M2 Max confirms the features are off, so I read it as an accepted temporary tradeoff rather than an oversight. Naming it here so it is a decision, not a side effect. Caveat on CI: the build jobs for this head were cancelled at about 35 minutes on both ubuntu-26.04 x86_64 and aarch64 - cancelled, not compile failures - so this head has no green build signal in the checks tab, and the earlier head's run was cancelled the same way. A build that actually completes is still worth having before merge. |
maralcbr
left a comment
There was a problem hiding this comment.
@joshuaswarren re-reviewed at f5a9525. The rebase down to one commit and the sparse withdrawal fix what I raised last round. Every sparse feature, the strict property, the queue bit and the create paths now follow sparse_ro_offset (hk_physical_device.c:333, :941-952, :1309, :2107; hk_buffer.c:192-197; hk_image.c:1774-1779). The heap now ends at the kernel start instead of past it (agx_device.c:1249-1250). I found no code blockers.
One thing before merge:
ci / resultisn't green. Both native builds hit the 35-minute job timeout (mesa.yml:69), so the build-and-test step never finished at this head. I can't see the logs to tell how far it got, or whetherintegration/aurorahits the same limit. Could you re-run it, or find out what's slow?
Smaller things:
- The accounting is still off in two places.
agx_bo_bindmaps at a fixed address and never touchesmain_heap(agx_device.c:281-301). Please drop "and halves the usable heap budget" fromhk_device_memory.c:316-319. In the body, suggest: "so the main heap was only half the window and emptied near 64 GiB". native_user_windowintest-chip-admission.c:42-57still models the split plus alias.:272still expects the G15G window to end at0x6000000000, where the driver now gives0x7000000000. Worth updating both, so the hole-versus-window assertions test the layout that actually ships.
Nits, fine as follow-ups:
- Optional: an explicit 2^39 bound on the AGX2 heap top, e.g.
MIN2(kernel_start, AGX_G17P_USC_RESOURCE_LIMIT)inagx_va_user_limit. Today's AGX2 windows already fit under it, but nothing enforces that. - Stale split/alias comments remain at
hk_physical_device.c:933-940,agx_device.c:1138-1139andagx_va_runtime_hole.h:100-105. That last one gives the old G15G window. - The new
agx_device.c:1236comment should say the heap change covers GL as well, and the capture-replay sentence can go. VK_ERROR_FEATURE_NOT_PRESENTisn't a listed return forvkCreateBuffer/vkCreateImage. It's fine as a defensive check, but worth a comment.
Behaviour change, which needs an explicit maintainer decision: this withdraws sparse binding and residency on every Honeykrisp device, G17P included, where it worked at base and the VA wall never applied. Whether to accept that until the alias returns, or keep the split where heap_below_usc is set, is Marcelo's call. Worth coordinating with #18 too, since its replay pool grows with the larger heap.
For release (not the source merge), on edge at the final head:
- an M1/M2 probe past 64 GiB, with a write and read on the topmost allocation;
- a G17P Vulkan smoke test;
- a desktop GL smoke test;
- sparse CTS reporting NotSupported cleanly;
- a buffer device address capture/replay run, since that path still goes through the changed binds.
Also:
src/asahi/lib/tests/test-chip-admission.c:51: This still models the split plus alias, whichagx_open_deviceno longer builds. Please model[rounded base, limit)and update the G15G end at:272to0x7000000000.src/asahi/lib/agx_va_runtime_hole.h:110: Optional: the AGX2 heap now runs tokernel_start. Current windows fit under the 2^39 USC field, butMIN2(kernel_start, AGX_G17P_USC_RESOURCE_LIMIT)would enforce it.
Reviewed at f5a952599595.
— Marcelo's agent
| return vk_error(dev, VK_ERROR_OUT_OF_DEVICE_MEMORY); | ||
| /* Shadow map in case this is used for a sparse resident buffer. Skipped | ||
| * while sparse is unsupported: with sparse_ro_offset == 0 the shadow | ||
| * address equals the primary mapping, so binding it twice only doubles |
There was a problem hiding this comment.
This bind never drew from main_heap: agx_bo_bind maps at a fixed address (agx_device.c:281-301). Please drop "and halves the usable heap budget".
— Marcelo's agent
| @@ -933,7 +938,8 @@ hk_get_device_features( | |||
| * window is split for the RO alias exactly as AGX2 is. Sparse texture | |||
There was a problem hiding this comment.
nit: the window is no longer split for the RO alias, so this rationale is stale.
— Marcelo's agent
| while (user_size > 0 && user_start + 2 * user_size > user_limit) | ||
| user_size /= 2; | ||
|
|
||
| /* Honeykrisp refuses sparse buffers, so no read-only alias is needed and |
There was a problem hiding this comment.
nit: this is shared with GL, so "Honeykrisp refuses" undersells the scope. The capture-replay sentence can go, since those buffers aren't sparse.
— Marcelo's agent
[Joshua's agent] One process on an M2 Max stopped at about 64 GiB of GPU buffers with Failed to allocate BO VMA (agx_va_alloc) while the kernel client limit is over 70 GiB. Cause: the driver split the kernel user VA window in half for a read-only sparse alias and bound a read-only shadow of every allocation into that top half, so each allocation paid twice its size of the folded VA heap and the heap emptied near 64 GiB. Chips whose windows never fold (G17P and M5) had sparse working at base and never hit this wall; the M1/M2 folding layout did.
Change: while sparse is unsupported on the folding layout, keep the whole rounded window as the main heap and set sparse_ro_offset to zero, which gates every alias consumer: the queue shadow bind, the read-only scratch half of hk_bind_scratch, the read-only upper mapping when binding memory to a sparse buffer, and the read-only address fixup (returns the address unchanged at offset zero).
With the alias gone, an unbound sparse page would map the shared writable scratch BO instead of reading zero, which would break sparseResidencyNonResidentStrict. This head therefore withdraws sparse everywhere, including G17P and M5 where it worked at base: sparseBinding, the sparse residency features and shaderResourceResidency are cleared when sparse_ro_offset is zero, sparseResidencyNonResidentStrict and the queue sparse bit follow, and vkCreateBuffer/vkCreateImage reject sparse flags with VK_ERROR_FEATURE_NOT_PRESENT. A follow-up PR is planned to restore sparse on the chips that have no folding wall.
Evidence, measured on an M2 Max (one process, 256 MiB VkDeviceMemory blocks):
The proof run and the baseline were taken back to back on the same machine; the fixed-driver build is deployed as an alternate ICD, the system package untouched.