Skip to content

honeykrisp: keep the whole VA window when sparse is unsupported - #38

Open
joshuaswarren wants to merge 1 commit into
omacom:integration/aurorafrom
joshuaswarren:w7k/hk-sparse-va-window
Open

joshuaswarren wants to merge 1 commit into
omacom:integration/aurorafrom
joshuaswarren:w7k/hk-sparse-va-window

Conversation

@joshuaswarren

@joshuaswarren joshuaswarren commented Nov 11, 2026 •

Copy link
Copy Markdown

[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):

  • unfixed driver, heap sized past the wall: stops at 63.75 GiB held, Failed to allocate BO VMA;
  • fixed driver, same probe: PROBE holds 68.75 GiB, limited by the kernel client limit, not VA;
  • an MLX matmul through the real queue-bind path passes on the fixed driver;
  • without the heap override the 0.75x host RAM heap heuristic still caps at 47.13 GiB on that machine (separate question, untouched here).

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.

@greptile-apps

greptile-apps Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issue remains.

Summary

The PR gives ordinary allocations the full rounded address window instead of reserving half for read-only sparse mappings.

  • Allocations use the full rounded user address window.
  • Sparse capabilities appear only when the read-only alias exists.
  • Sparse buffer and image requests return feature-not-present when unsupported.

Reviews (3) · Last reviewed commit: "honeykrisp: keep the whole VA window whe..." · Reviewed by Greptile

Comment thread src/asahi/compiler/agx_compile.c Resolved
Comment thread src/asahi/vulkan/hk_cmd_dispatch.c Resolved
Comment thread src/vulkan/runtime/vk_drm_syncobj.c Resolved
@joshuaswarren
joshuaswarren force-pushed the w7k/hk-sparse-va-window branch from 2fb4b7d to c789060 Compare November 11, 2026 15:19
@joshuaswarren

Copy link
Copy Markdown
Author

[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.

Comment thread src/asahi/vulkan/hk_queue.c Resolved
@joshuaswarren

Copy link
Copy Markdown
Author

NO-GO at c789060
Cross-model review (Claude)
Model: claude-sonnet-5-5

Read the full diff and the consumers of sparse_ro_offset at this head. No build or hardware run. The three areas asked about:

  • Rounding overflow: none reachable. util_next_power_of_two64(user_start) is only undefined above 2^63, and agx_device.c:1174-1177 already rejects user_start >= vm_end. A start that rounds up past user_limit gives user_size == 0 and the new "leaves no heap" failure path, so it fails cleanly. The checked values come out right: G17P, G15G and M5 start on a power of two already, and the M2 start rounds 0x2100000000 to 0x4000000000.
  • Runtime-graph hole: with offset 0 the shadow interval equals the primary interval, and reserve_intervals merges overlaps, so agx_va_reserve_runtime_graph_hole still succeeds.
  • Smaller-memory machines: the heap only gets bigger, and the capture/replay heap is min(user_size/8, 64 GiB). No new failure path except the one below.

MUST FIX

  1. Sparse is still advertised, so the PR's premise ("Honeykrisp refuses sparse buffers", "sparse residency is not exposed") is false at this head. hk_physical_device.c:389-402 sets sparseBinding, sparseResidencyBuffer, sparseResidencyImage2D, sparseResidencyAliased and shaderResourceResidency true for every non-native chip, which includes the M1/M2 path this PR targets. hk_physical_device.c:936-946 sets them for G17P, and :2101-2109 adds VK_QUEUE_SPARSE_BINDING_BIT. Property sparseResidencyNonResidentStrict is true (:1303). Sparse buffers and images are created and bound (hk_buffer.c:198-250 via hk_bind_scratch, hk_image.c:1695, hk_queue.c:hk_flush_bind). With sparse_ro_offset == 0 the only mapping of an unbound page is the writable scratch_bo bind (hk_queue.c:681-689, hk_buffer.c:134). hk_buffer_address_sparse_ro (hk_buffer.c:504-511) now returns the read-write address. So reads of unbound sparse pages return whatever any write to an unbound page left in the shared scratch BO, not zero, which breaks the advertised sparseResidencyNonResidentStrict guarantee. The old zero-page alias was what made it hold. The M2 allocation probe (68.75 GiB) does not exercise this. Fix: in hk_get_device_features, clear every sparse feature (sparseBinding, sparseResidencyBuffer, sparseResidencyImage2D, sparseResidencyAliased, shaderResourceResidency, sparseResidencyImage3D) when dev->sparse_ro_offset == 0 (agx_device is set up before features are built), make the non-native queue flags at hk_physical_device.c:2101 follow supported_features.sparseBinding, and add a check that fails vkCreateBuffer/vkCreateImage with the sparse create flags if the feature is off. Alternatively keep the split on chips that advertise sparse and drop it only where sparse is off.

  2. Unrelated chips lose the same guarantee silently: G17P and M5 also advertise sparse and rely on the alias for shader masking (zero_base = base | sparse_ro_offset in hk_nir_lower_descriptors.c:1535, hk_cmd_meta.c:455). The same feature clearing covers them; without it the shader paths become no-ops while the features stay on.

NICE TO HAVE

  • agx_va.c:79-96: agx_va_free is an alias consumer the PR's gating list does not mention. With offset 0 every unbind sends two identical UNBIND ops for the same range in one transaction. The unbind of a hole is probably tolerated, but gate the second op on dev->sparse_ro_offset so a VM_BIND with a duplicate range is never sent.
  • The PR proves allocation capacity only. A heap that now ends near vm_end - 32 GiB on M1/M2 is addressed by shaders, texture descriptors and VDM pointers. A probe that writes and reads from the topmost allocation would show those fields cover the whole new window.
  • Update the stale comments at agx_device.c:1100-1104 and :1150-1153, which still describe the split.

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.
@joshuaswarren
joshuaswarren force-pushed the w7k/hk-sparse-va-window branch from c789060 to f5a9525 Compare November 11, 2026 15:47
@joshuaswarren

Copy link
Copy Markdown
Author

[Joshua's agent] Fixes for the NO-GO items, new head f5a9525 (same branch, retargeted again):

  • hk_get_device_features now gates sparseBinding, sparseResidencyBuffer, sparseResidencyImage2D, sparseResidencyAliased and shaderResourceResidency on dev->sparse_ro_offset != 0, for the shared non-native block and the G17P block alike. sparseResidencyImage3D stays false. sparseResidencyNonResidentStrict reads the same condition, and the non-native queue flags take VK_QUEUE_SPARSE_BINDING_BIT only when supported_features.sparseBinding is set (the native path already did).
  • vkCreateBuffer and vkCreateImage fail with VK_ERROR_FEATURE_NOT_PRESENT for sparse create flags when the feature is off, before any allocation.
  • agx_va_free sends one UNBIND instead of two identical ones while sparse_ro_offset is zero.
  • The comments that justified heap placement with the split no longer describe it as binding; the low-window start selection stays as shipped.

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.

@joshuaswarren

Copy link
Copy Markdown
Author

GREEN at f5a9525
Cross-model review (Claude)
Model: claude-sonnet-5-5

Re-review of the NO-GO at c789060 (comment 6799942278). Read the full diff against base b7b03c5 and every consumer of sparse_ro_offset and of the sparse features at this head. Source read only: no build, no hardware run.

NO-GO items, each checked

  • sparseBinding, sparseResidencyBuffer, sparseResidencyImage2D, sparseResidencyAliased, shaderResourceResidency are gated on dev->sparse_ro_offset != 0 (hk_physical_device.c:333, 392-404). The G17P block gates the same set plus sparseResidencyImage3D (:941-952). Native chips other than G17P were already false. M5 (G17G/G17S) is not in agx_chip_native_agx3, so it takes the shared block and is gated. Features are built at :2013, after agx_open_device at :1915, so the gate reads the final offset.
  • sparseResidencyNonResidentStrict follows the same condition (:1309). The M5 override at :1587 was already false.
  • The queue VK_QUEUE_SPARSE_BINDING_BIT is taken from supported_features.sparseBinding on both the shared and native paths (:2105-2117). No other site sets the bit.
  • hk_CreateBuffer and hk_CreateImage return VK_ERROR_FEATURE_NOT_PRESENT for SPARSE_BINDING/RESIDENCY/ALIASED when sparseBinding is off, before any allocation. The image check sits after the WSI swapchain early return, which is right. hk_GetPhysicalDeviceImageFormatProperties2 already returns FORMAT_NOT_SUPPORTED for sparse flags without sparseBinding, so the sparse format-properties query returns none.
  • agx_va_free now sends one UNBIND while the offset is zero (dev->sparse_ro_offset ? 2 : 1, agx_va.c:100); the first op is the rw range, so the count prefix is correct. The agx_rw_addr_to_ro call in the initializer is still safe at offset 0.

Regression check where the offset is non-zero: none exist at this head. dev->sparse_ro_offset = 0 is unconditional (agx_device.c:1257), so every if (sparse_ro_offset) branch is dead on every chip and the ro-alias code paths cannot run. The remaining zero-offset consumers are identity operations (agx_rw_addr_to_ro returns the address; & ~0 and | 0 in the descriptor/texture lowering; the runtime-graph hole's shadow interval equals the primary one and merges). Heap windows: M1/M2 start rounds 0x2100000000 to 0x4000000000 and run to the kernel start; G17P is [0x4000000000, 0x8000000000), below both the 2^39 USC field and the VDM aperture, with the 0x7000000000 runtime hole still reserved; G15G ends at the 0x7000000000 runtime reservation; M5 is [0x4000000000, 0x8000000000).

MUST FIX

None.

NICE TO HAVE

  1. Sparse is withdrawn on every chip, including G17P and M5. Base b7b03c5 advertises it there (hk_physical_device.c base :389 and :936). The PR body says sparse is "refused everywhere" and "not exposed", which was false at base. G17P and M5 did not have the 64 GiB VA wall (their windows are 256 GiB), so they lose a working feature for no VA gain. Either say so in the description, or keep the split for chips where user_size is not the limiting factor.
  2. The pair assumptions in hk_queue.c (assert((count % 2) == 0) and the stride-2 loops at :730-736 and :779-786, and the comment at :827 about "its read-only shadow") no longer hold at offset 0. They are unreachable now, since sparse buffers and images cannot be created, but a future non-sparse caller of the bind builder would hit them.
  3. test-chip-admission.c:44-58 (native_user_window) still models the split window plus alias, which agx_open_device no longer produces, so the hole-versus-window assertions test a layout nobody runs. Update it to [rounded start, limit).
  4. util_next_power_of_two64(user_start) (agx_device.c:1253) only existed so one address bit could mark the alias. On M1/M2 it discards about 124 GiB (0x2100000000 to 0x4000000000). Dropping it is probably safe, but I did not prove nothing else needs a power-of-two base.

Not verified

  • No build, no unit tests (test-chip-admission and the others were not run), no CTS, no hardware. The author's vaalias6f build and the 68.75 GiB probe on an M2 Max are theirs; I did not reproduce them. The fresh guard-queued proof row and vulkaninfo feature dump the author promised were not available to me.
  • Chip behaviour is from source reading only. I did not run G15G, G16G, G17P or M5 hardware.

@joshuaswarren

Copy link
Copy Markdown
Author

[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.

@joshuaswarren

Copy link
Copy Markdown
Author

GREEN at f5a9525
Cross-model review (MiniMax)
Model: minimax-code/MiniMax-M3.1-Flash

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 sparse_ro_offset and of the sparse features, with the in-tree unit tests read for breakage. The M2 Max proof (68.75 GiB held vs the 63.75 GiB wall) and the vulkaninfo feature dump are the author's measurements.

What I checked, and what held:

  • The window math. user_start = util_next_power_of_two64(user_start) is the same rounding the removed agx_va_split_user_window() used (agx_va_runtime_hole.c:107), so the new heap start is not a new rounding hazard; the heap is now [user_start, user_limit) and an empty range fails cleanly on the new "leaves no heap" path. The proof's heap top 0x77ffff8000 stays under the 36-bit USC texture/sampler field limit at 2^39.
  • Every alias consumer is gated on dev->sparse_ro_offset != 0, and sparse_ro_offset is now assigned 0 unconditionally (agx_device.c:1257): the per-allocation shadow bind in hk_AllocateMemory, the read-only scratch half in hk_bind_scratch, the upper read-only mapping in hk_BindBufferMemory2, the queue shadow in hk_flush_bind, and the duplicate UNBIND in agx_va_free (now one op instead of two). No consumer is left un-gated that I could find.
  • The address fixups are safe at offset 0: agx_rw_addr_to_ro is the add-offset form, so it returns the address unchanged, and the assert(!agx_addr_is_ro(dev, addr)) inside it cannot fire because addr & 0 is 0. The shader-side ~options->sparse_ro_offset mask in agx_nir_lower_texture.c becomes a no-op, and that path only runs with sparse buffer residency, which is now off.
  • Features, property and queue flag agree: sparseBinding, sparseResidencyBuffer/Image2D/Aliased, shaderResourceResidency and the sparseResidencyNonResidentStrict property all read the same condition, the G17P block gates the same set, and both the shared and the native queue-flag branches add VK_QUEUE_SPARSE_BINDING_BIT only when supported_features.sparseBinding is set (hk_physical_device.c:2106-2121). The order is right: agx_open_device at :1915 precedes hk_get_device_features at :2013, which precedes the queue flags at :2114, so the gate reads the final offset.
  • The create paths fail closed: vkCreateBuffer and vkCreateImage return VK_ERROR_FEATURE_NOT_PRESENT for sparse create flags when the feature is off, before any allocation.
  • No in-tree test breaks. The asahi unit tests never call hk_CreateImage/hk_CreateBuffer, so the new create gates do not reach them; test-hk-sparse-formats.c sets supported_features.sparseBinding = true itself, and test-hk-g17p-compression.c only feeds a flag to the pure hk_m5_depth_compression_eligible helper.

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 maralcbr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 / result isn'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 whether integration/aurora hits 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_bind maps at a fixed address and never touches main_heap (agx_device.c:281-301). Please drop "and halves the usable heap budget" from hk_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_window in test-chip-admission.c:42-57 still models the split plus alias. :272 still expects the G15G window to end at 0x6000000000, where the driver now gives 0x7000000000. 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) in agx_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-1139 and agx_va_runtime_hole.h:100-105. That last one gives the old G15G window.
  • The new agx_device.c:1236 comment should say the heap change covers GL as well, and the capture-replay sentence can go.
  • VK_ERROR_FEATURE_NOT_PRESENT isn't a listed return for vkCreateBuffer/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, which agx_open_device no longer builds. Please model [rounded base, limit) and update the G15G end at :272 to 0x7000000000.
  • src/asahi/lib/agx_va_runtime_hole.h:110: Optional: the AGX2 heap now runs to kernel_start. Current windows fit under the 2^39 USC field, but MIN2(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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants