Skip to content

hk: give G17P host-read and aliased-memory flushes memory scope - #20

Merged
maralcbr merged 6 commits into
integration/aurorafrom
aurora/g17p-memory-scope-flush
Oct 31, 2026
Merged

maralcbr merged 6 commits into
integration/aurorafrom
aurora/g17p-memory-scope-flush

Conversation

@DjDeveloperr

@DjDeveloperr DjDeveloperr commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Mirror of aurora-silicon#98 (merged into aurora-wip as deed89d) and #99 (merged as e567d3d), cherry-picked onto integration/aurora without conflicts. The three commits are byte-identical patches of upstream c9a17ac, eee5753 and c22e446 (same git patch-id). On this branch it builds with -Dwerror=true (gcc; clang stops on a pre-existing -Winitializer-overrides in hk_cmd_meta.c, which this PR does not touch) and passes the asahi unit suite. Every hardware result below, including the CTS table, was measured on the #98 branch (fix/g17p-memory-scope-flush, on aurora-wip), not on this branch. The hardware run of this branch is the one in scottjones's review.

On G17P (A18 Pro / Neo), a barrier made written data visible only to later reads through the same GPU virtual address. This PR gives the barrier's flush memory scope where a second address or the host can read the data. Both failures were found by the new gpubench stress suite (aurora-silicon/gpubench#4).

What broke

  • sparse.churn: a command buffer fills pages through a sparse buffer, records a transfer barrier, then copies the same pool pages through a plain buffer. The plain copy read stale memory: 494 of 526 pages, in every run. The sparse copy read the right data.
  • queue.depth: 32 compute and 32 render submissions ran on one queue. Compute results the host read back after the timeline signalled were stale 1–3 times in each 4-second run. A re-read 0.1–1.5 ms later found the right values. With the render ring on a second queue, three runs had no stale results in ~130,000 completions each.

Root cause

The G17P GPU caches are tagged by virtual address. The strong CDM barrier hk records for a dependency (flags 5, word 0x60001560) makes writes visible only through the same mapping. It does not write the data back to memory.

  • A read of the same memory through another mapping in the same command returns stale memory. That mapping can be a sparse binding next to a plain buffer, or a sparse buffer's read-only shadow.
  • The host also reads memory. hk records that same strong flush for HOST_READ availability, so the data stays in the GPU caches until the end of the command writes it back. That usually happens before the kernel reports completion, but not when render work shares the queue.

This is not the kernel's 50 ms stamp fallback. Tracepoints on four stale runs showed each stale compute submission retired from its normal completion stamp 35–240 µs before the host read it. The GPU had finished it 180–700 µs before the read.

Evidence

A diagnostic override of hk's strong-flush word was tested on the Neo with sparse.churn, 20 iterations, 526 pages:

Barrier word Stale pages
0x60001560 (flags 5, today) 494
0x60001570 (+ bit 4) 0
0x6000157c (+ bits 4:2, M5's memory-scope record) 0
0x600215fe (the G15/G16G wide-visibility word) 0
0x60001564 / 0x60001568 (bit 2 / bit 3 alone) 494 / 495
  • Bit 4 also fixed the reverse direction (write through the plain buffer, read through the sparse one) and shader stores. It also fixed reads of pages the GPU had already cached through the second mapping.
  • With the memory-scope word, queue.depth at 32 compute + 32 render on one queue went from failing 3 of 3 runs to passing 3 of 3, at ~135,000 completions per run.
  • Splitting the command stream into two kernel commands in the same submit did not help. Separate submits with a host wait between them did.
  • M5 already sets the same bits (flush_memory, HAL300) on every strong flush, and the existing packing test notes that ordering flags alone do not make writes visible through another mapping.

Fix

  • asahi: the CDM barrier codec accepts the memory-scope record (AGX3_CDM_BARRIER_FLUSH_MEMORY, bits 4:2) on G17P as well. Each bit is documented. G17P keeps the three-bit form that the HAL300 and G15 words share.
  • hk: hk_cdm_visibility_flush() records the strong flush with memory scope on request. A trailing strong record without it is strengthened in place, the same way a weak record already is. Inline stream concatenation keeps the scope of the record it folds into.
  • Host-read availability always asks for memory scope on G17P.
  • Compute and transfer dependencies ask for it only while the device holds a resource with a GPU address of its own: a sparse buffer or image, or a capture/replay buffer. This covers pipeline barriers, the deferred-clear barrier, the transfer-before-graphics ordering and vkCmdSetEvent2. Devices that never create such a resource record exactly the barriers they did before.
  • Other chips are unchanged.

The gate reads device state when a barrier is recorded. Any resource the command buffer references exists by then. It is counted from creation, so a later vkQueueBindSparse that remaps it is covered. The memory-scope flush writes back every dirty line, but only when it is recorded: it needs an open compute stream holding work. A dependency recorded with no such stream gets no flush, whether it opens a command buffer or follows a timestamp, query copy or other point that closed the stream, so writes from before it are not covered. The follow-up #23 handles that case.

Not covered: an aliasing resource created after the command buffer was recorded and reached only through a device address the command buffer reads indirectly. This is legal Vulkan but unusual. Closing it would mean gating on the enabled features (sparseBinding, bufferDeviceAddressCaptureReplay) instead. DXVK and vkd3d-proton enable those whenever they are offered, so that gate would put the flush cost on every Proton game. I chose not to do that.

Also not covered as this lands: when the memory-scope flush goes into an open stream holding work unrelated to the source, a graphics destination is still ordered only after the source's stream, so it can start before the flush. Compute and transfer readers follow the flush on the CDM subqueue. #23 orders the graphics destination after the flush stream in that case.

Qualification

Neo (A18 Pro), stock kernel kit (integration/aurora + #563), the aurora-silicon#98 branch (fix/g17p-memory-scope-flush: commits c9a17ac and eee5753, identical patches to the first two commits here) built with -Dwerror=true:

  • Host: the asahi unit tests pass 75/75, including the updated agx3 barrier-packing test.
  • gpubench-stress, unmodified PR honeykrisp: partial-subgroup coopmat, narrow-pointer addressing, cache key and sin fixes (stacked on #3) #4 binary: sparse.churn passed 6/6 runs at about 1.18M checks each. Baseline fails at iteration 0.
  • queue.depth with the #98 branch:
    • 7 runs at 64–128 compute + 64 render on one queue, 90k–156k checks each, had no stale result. The baseline has stale results within 0.2 s.
    • These deep runs only finish with the kernel's work-state limit in place. Without it, depth 128 + 64 still loses the device to a separate kernel stall, fixed in a separate aurora-silicon/linux PR.
    • The 3 clean runs at 32 + 32 in the evidence above used the diagnostic flush word on every strong flush, not the #98 branch.
  • CTS: run with /var/tmp/cts/run.py, 6 workers.
Group Pass Fail
sparse_resources 9903 0
memory_model 3796 0
query_pool 18160 0
synchronization (rerun) 28647 0
synchronization2 36516 0
api.copy_and_blit 101867 0
memory 4945 0
compute 10743 0
ssbo 12162 0

The first synchronization pass had two rc=-9 crashes in signal_order.timeline_semaphore.write_image_*, after a GPU timeout under 6-worker load. Both pass 3/3 in isolation on the #98 branch and on the baseline. The baseline shows the same crash class in its own full pass, and a full rerun on the #98 branch was clean.

  • Performance:
    • For applications without host-read barriers and without alias resources, the recorded command streams are byte-identical to the baseline. gpubench records neither, and 20 light and heavy rows over 3 alternated rounds stayed within run-to-run spread.
    • Each host-read barrier gets one memory-scope flush.
    • With a live sparse buffer on the device, 64 dispatch + barrier pairs per command buffer cost +20% when each dispatch writes 256 KB, and nothing measurable at small writes.

Review follow-ups

The strong CDM barrier (flags 5) makes compute and transfer writes visible
only to later reads through the same virtual address, because the G17P
caches are tagged by virtual address. A read of the same memory through a
second mapping in the same command returns stale memory. That second
mapping can be a sparse binding next to a plain buffer over the same pages,
or a sparse buffer's read-only shadow.

Bit 4 of the barrier word adds memory scope. On a Neo (A18 Pro), one
command buffer fills pages through a sparse buffer, records a transfer
barrier, then copies the same pool pages through a plain buffer. Pages read
back stale, by barrier word:
- 0x60001560 (flags 5): 494 of 526
- 0x60001570 (bit 4): 0 of 526
- 0x6000157c (bits 4:2, the HAL300 memory record): 0 of 526
- 0x600215fe (the G15 wide-visibility word): 0 of 526
- 0x60001564 or 0x60001568 (bit 2 or bit 3 alone): 494 and 495 of 526

Bit 4 also fixes the reverse direction, shader stores, and reads of pages
the GPU had already cached through the second mapping.

Accept the HAL300 record (bits 4:2) on G17P as well, and name the bits.
On G17P a strong CDM flush leaves written data in the GPU caches, which are
tagged by virtual address. Two cases depended on more than that.

Host availability: a barrier to HOST_READ records hk's strong
cache-maintenance flush, which never reached memory on G17P. The end of the
command usually wrote the data back before completion was reported, but
not when render work shared the queue.

gpubench-stress queue.depth runs 32 compute and 32 render submissions on
one queue. Compute results were read back stale 1-3 times per 4-second run,
in every run. Tracepoints showed each stale submission had finished on the
GPU 180-700 us before the host read it, and the kernel had retired it from
its completion stamp 35-240 us before the read. The data reached memory
0.1-1.5 ms later. With the render ring on a second queue, or with a
memory-scope host flush, three runs each saw no stale result in ~135000
completions.

Aliased memory: sparse buffers and images, and capture/replay buffers, map
their memory at an address of their own as well. In sparse.churn, a copy
through a plain buffer over the pool read stale data for 494 of 526 pages
written through the sparse buffer. That happens after a barrier in the same
command buffer, whichever direction or kind of write is involved. Splitting
the stream into two kernel commands of the same submission did not help.

So:
- hk_cdm_visibility_flush() records the strong flush with memory scope
  (AGX3_CDM_BARRIER_FLUSH_MEMORY) on request. It strengthens a trailing
  strong record in place, as a weak one already is. Inline stream
  concatenation keeps the scope of the record it folds into.
- Host-read availability always asks for memory scope on G17P.
- Compute and transfer dependencies ask for it only while the device holds
  a resource with its own GPU address (hk_cdm_alias_scope()). This covers
  pipeline barriers, the deferred-clear barrier, the transfer-before-graphics
  ordering, and vkCmdSetEvent2. Devices that never create such a resource
  record the same barriers as before.

M5 already gives every strong flush memory scope. Other chips are unchanged.

In a microbenchmark of 1 and 64 dispatch-plus-barrier pairs per command
buffer, the memory-scope form cost nothing measurable: 106-109 us and
167-172 us per command buffer either way.
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The latest revision appears safe to merge; it changes comments only.

Summary

Adds memory-scope flushes on G17P for host reads and compute or transfer dependencies while aliasing resources exist.

  • The barrier codec can encode memory-scope flushes on G17P.
  • Host-read barriers request memory-scope flushes on G17P.
  • Device dependencies use scoped flushes when live resources can alias memory.

No new actionable issues were found.

Reviews (5) · Last reviewed commit: "hk: name the unrelated-stream gap in the..." · Reviewed by Greptile

Comment thread src/asahi/vulkan/hk_cmd_buffer.h Resolved
hk_cs_merge_cdm() folds the next stream's opening strong flush into a
trailing barrier and must keep the memory scope that record carried,
and keep memory_flush_end exact for later merges and cache flushes. The
merge test only checked metadata and links, so dropping either would
have passed it and brought back stale reads through another mapping.

Cover a memory-scoped tail folded with a plain opening barrier, a
barrier-only second stream, repeated merges, a memory-scoped opener
folded onto a plain tail and an unfolded copy, checking each barrier
word and memory_flush_end. With more than one call site the merge stays
out of line, so the test stubs hk_drm_attachment_set_add() like the
link emitter. The test fails when the fold drops the memory bit, and
when a barrier-only fold loses the marker.

asahi meson suite: 107 tests pass.

(cherry picked from commit c22e446)

@scottjones scottjones left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We read this at 18ff4cd against the gpubench sources at db3fec6, and ran it on one Neo through a private Vulkan driver (VK_DRIVER_FILES), so the desktop stayed on the installed Mesa.

With linux-aurora #278, #275, #276, #264 and #277, the current pin (672657e) failed queue.depth with stale compute data at submission 637 and sparse.churn at iteration 0. With this PR on that same kernel, queue.depth passed 5/5 and sparse.churn 3/3, at about 1.6M-1.8M queue checks and 2.8M sparse checks per run, 0 errors. A three-PR kernel (#278, #275, #276) passed the same way. On the 7.2.11-10-1 release, queue.depth hit a kernel stall this PR doesn't touch: a lost device at 6.4 s and no GPU afterwards, so sparse.churn got no comparison there. That's one job per kernel, Neo only, and no CTS with this PR; the CTS numbers in your body we've taken as read.

The logic looks right. hk_cdm_flush() only overwrites the trailing record when it's already strong and lacks the scope (hk_cmd_dispatch.c:589-593, 620-627), and a plain flush can't downgrade a scoped one. The merge keeps the scope both ways (hk_cmd_buffer.h:1396-1404, 1423-1428). The counter balances on every error path we followed (hk_buffer.c:320, 344; hk_image.c:1706, 1738-1742). The patches match aurora-silicon#98 and #99 line for line.

Scope is what you say: G13/G14 return early and only gain the counter (hk_cmd_buffer.c:1100-1106), G15, G16 and M5 don't change, and Gallium isn't touched. T6040 maps to G17P, but test-chip-admission.c still refuses it, so today this is the Neo only.

The sparse.churn failure matches your sequence (t_sparse.c:151-159). The queue.depth value is the seed, k itself (st_ring.c:58-60), which fits a stale read and also a dispatch that hadn't run when the timeline signalled. We can't separate those the way your tracepoints did, but it hasn't come back.

Things we'd like answered:

  1. A barrier at the start of a later command buffer records no flush. hk_cmd_buffer.c:1157 needs an open stream with work in it, and hk_cdm_visibility_flush_needed() returns false at stats.cmds == 0 (hk_cmd_dispatch.c:557-558). That doesn't match "also covers writes recorded in earlier command buffers" (hk_device.h:364-365). This is from reading; we haven't run it. Could you try sparse.churn split that way? It isn't a regression, so it can follow, but the comment should say what's true.

  2. "Byte-identical" doesn't cover Zink. zink_batch.c:711-720 records a TRANSFER_WRITE to HOST_READ barrier at the end of every batch, so every Zink batch on the Neo gets the memory-scope word, often as a barrier-only stream opener (hk_cmd_dispatch.c:631-642). Neither your numbers nor our run cover that. Do you have a before and after? We'd want a GL client forced through Zink on the Neo with this build before it goes past edge.

  3. Update-after-bind descriptors reach a resource created after recording too (hk_physical_device.c:451-456), so we'd widen the comment at hk_device.h:365-367.

  4. The body says aurora-wip and the Qualification section says "this branch". Which is the CTS table?

For the RC we want this. A stale read is silent: an app mixing compute and render on one queue gets old contents after its fence, with no error. Outside the Neo nothing changes.

The comment on hk_cdm_alias_scope() said the memory-scope flush also
covers writes recorded in earlier command buffers. It does when a later
barrier records the flush: hk_CmdPipelineBarrier2() records it into the
open compute stream, and only when that stream already holds work
(hk_cdm_visibility_flush_needed() returns false at stats.cmds == 0). A
barrier recorded before any work in a command buffer has no such stream
and records no flush. Whether the hardware returns stale data then is
untested; the comment now says so instead of claiming coverage.

The "not covered" case also includes a descriptor set updated after bind
to reach a resource created after recording
(descriptorBinding*UpdateAfterBind is advertised).

From the review of #20.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 682223a)
@DjDeveloperr

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. The Neo is offline today, so this answers what can be settled from the code and the host build; the two hardware items are listed at the end so they can run as soon as it is back.

1. A barrier at the start of a later command buffer. Your reading is right, and the comment was wrong. hk_CmdPipelineBarrier2() records the memory-scope flush into cmd->current_cs.cs and only when hk_cdm_visibility_flush_needed() says so, which is false at stats.cmds == 0. A fresh command buffer has no open stream at all until its first work allocates one (hk_cmd_buffer_get_cs_general()), so the first barrier of a command buffer goes through hk_sync_barrier() alone, which orders streams and writes nothing into any of them. At submit every stream is its own kernel command (hk_queue.c:1156-1160, 1381-1384); nothing joins it to the earlier command buffer's stream, whose tail is whatever its last launch left. hk_CmdSetEvent2() has the same gate. So in the split sparse.churn, the fills' writes get no memory-scope flush before the copies read the pool through the alias, in the same submit or in a following one. Whether the hardware then returns stale data I have not measured; the only related data point is the one in the body, that two kernel commands in one submit did not help, which points that way but is not this case. The comment now says what the code does and that the hardware result is untested: aurora-silicon#123, cherry-picked here as 7c8c738. A fix is in draft, not hardware-tested, with the same shape as the host-read path: when no open compute stream holds work, the flush goes into a stream ordered after the source scope (hk_sync_get_cs_after()) and the destination kinds are ordered after that stream (hk_sync_after_stream()), so a reader in the same stream follows the record and a render waits for the stream. Upstream draft: aurora-silicon#124; mirror on this repo, based on this branch: #23. It should not merge before the split test runs on the Neo.

2. Zink. Agreed that "byte-identical" does not cover it: zink_batch.c:711-720 records TRANSFER_WRITE to HOST_READ at the end of every batch, so on the Neo every Zink batch gets one memory-scope record, as a barrier-only stream opener when the batch recorded no compute work. I have no before/after for it and cannot measure one until the Neo is back; I will not estimate the cost. The A/B is prepared (same boot, alternated rounds, baseline 672657e vs this branch, both with Zink and hk from the same private build selected by MESA_LOADER_DRIVER_OVERRIDE=zink plus VK_DRIVER_FILES), and I will post the numbers here when it has run.

3. Update-after-bind. Widened in the same comment commit: a descriptor set bound at recording and updated after it reaches a resource created after recording the same way an indirect device address does, and the gate at record time does not see it.

4. Which build the CTS table is from. The #98 branch, fix/g17p-memory-scope-flush on aurora-wip (commits c9a17ac and eee5753), not this branch. The three commits here are byte-identical patches of upstream c9a17ac, eee5753 and c22e446 (same git patch-id), so the results carry over, but nothing in the body ran on this branch on hardware; your review run is the only one. I have corrected the body to say so. What I ran on this branch today: host build in the Aurora build VM with gcc and -Dwerror=true, asahi unit suite 73/73 at 18ff4cd, and again 73/73 on the draft mirror #23 (this branch plus the comment commit plus the fix). One note for whoever builds this: clang with -Dwerror=true stops on a pre-existing -Winitializer-overrides at hk_cmd_meta.c:4574, which this PR does not touch; the werror claim holds for gcc.

Waiting on the Neo

  • Q1: gpubench sparse.churn with the barrier opening a second command buffer, in the same vkQueueSubmit2 and in a following one with no host wait. The variant is a scratch patch to gpubench db3fec6 (GB_SPARSE_SPLIT=1|2, default unchanged, below); on this branch, then with the draft fix. It compiles for aarch64; it has not run anywhere.
  • Q2: GL correctness subset and a per-batch-heavy GL workload through Zink on the Neo, baseline vs this branch, same boot, alternated rounds, with the measured cost of the memory-scope record.
gpubench sparse.churn split variant (scratch, against db3fec6)
diff --git a/stress/src/t_sparse.c b/stress/src/t_sparse.c
index 18fcd49..5ec8ea6 100644
--- a/stress/src/t_sparse.c
+++ b/stress/src/t_sparse.c
@@ -94,11 +94,21 @@ t_sparse_churn(struct st_run *r)
       have_alias = (amr.memoryTypeBits & (1u << mt)) && !vkBindBufferMemory(d.dev, alias, pool, 0);
    }
    struct st_buf rb;
-   VkCommandBuffer cb;
+   VkCommandBuffer cb, cb2;
    VkSemaphore tl;
    if ((vr = st_buf_create(&d, SP_BOUND * 2 * 256, VK_BUFFER_USAGE_TRANSFER_DST_BIT, true, &rb)) ||
-       (vr = st_cb_alloc(&d, 0, &cb)) || (vr = st_timeline(&d, 0, &tl)))
+       (vr = st_cb_alloc(&d, 0, &cb)) || (vr = st_cb_alloc(&d, 0, &cb2)) || (vr = st_timeline(&d, 0, &tl)))
       return st_fail(r, "setup: %s", st_vkr(vr));
+   /* Test-only split (omacom/mesa#20 review, question 1): GB_SPARSE_SPLIT=1 records the fills in
+    * one command buffer and the transfer barrier plus the copies in a second one, both in one
+    * vkQueueSubmit2; GB_SPARSE_SPLIT=2 submits the second command buffer in a following
+    * vkQueueSubmit2 with no host wait between them. The barrier then opens the second command
+    * buffer, and its first scope reaches the fills only through submission order. */
+   const char *split_env = getenv("GB_SPARSE_SPLIT");
+   const int split = split_env ? atoi(split_env) : 0;
+   if (split)
+      fprintf(stderr, "  [sparse.churn] GB_SPARSE_SPLIT=%d: barrier opens a second command buffer, %s\n", split,
+              split == 1 ? "same submit" : "separate submit, no host wait");
    uint32_t bpage[SP_BOUND] = {0}, ppage[SP_BOUND] = {0};
    uint32_t nbound = 0;
    uint64_t value = 0, binds = 0;
@@ -149,22 +159,52 @@ t_sparse_churn(struct st_run *r)
       st_cb_begin(cb);
       for (uint32_t i = 0; i < nbound; i++)
          vkCmdFillBuffer(cb, sb, bpage[i] * page, page, spval(bpage[i], iter));
-      st_barrier(cb, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT, VK_ACCESS_2_TRANSFER_WRITE_BIT,
+      /* With a split, the barrier is the first command of the second command buffer. */
+      VkCommandBuffer rcb = cb;
+      if (split) {
+         vkEndCommandBuffer(cb);
+         st_cb_begin(cb2);
+         rcb = cb2;
+      }
+      st_barrier(rcb, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT, VK_ACCESS_2_TRANSFER_WRITE_BIT,
                  VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT, VK_ACCESS_2_TRANSFER_READ_BIT);
       for (uint32_t i = 0; i < nbound; i++) {
          VkBufferCopy c = {.srcOffset = bpage[i] * page + page - 256, .dstOffset = i * 512ull, .size = 256};
-         vkCmdCopyBuffer(cb, sb, rb.buf, 1, &c);
+         vkCmdCopyBuffer(rcb, sb, rb.buf, 1, &c);
          if (have_alias) {
             VkBufferCopy a = {.srcOffset = ppage[i] * page, .dstOffset = i * 512ull + 256, .size = 256};
-            vkCmdCopyBuffer(cb, alias, rb.buf, 1, &a);
+            vkCmdCopyBuffer(rcb, alias, rb.buf, 1, &a);
          }
       }
-      st_barrier(cb, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT, VK_ACCESS_2_TRANSFER_WRITE_BIT, VK_PIPELINE_STAGE_2_HOST_BIT,
+      st_barrier(rcb, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT, VK_ACCESS_2_TRANSFER_WRITE_BIT, VK_PIPELINE_STAGE_2_HOST_BIT,
                  VK_ACCESS_2_HOST_READ_BIT);
-      vkEndCommandBuffer(cb);
+      vkEndCommandBuffer(rcb);
       struct st_sem_op w = {tl, bv}, s = {tl, bv + 1};
       value++;
-      if ((vr = st_submit(d.q[0], cb, &w, 1, &s, 1, VK_NULL_HANDLE)))
+      if (split == 1) {
+         /* Both command buffers in one vkQueueSubmit2. */
+         VkSemaphoreSubmitInfo wi = {.sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = tl, .value = bv,
+                                     .stageMask = VK_PIPELINE_STAGE_2_ALL_COMMANDS_BIT};
+         VkSemaphoreSubmitInfo si = {.sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = tl, .value = bv + 1,
+                                     .stageMask = VK_PIPELINE_STAGE_2_ALL_COMMANDS_BIT};
+         VkCommandBufferSubmitInfo cbi[2] = {
+            {.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_SUBMIT_INFO, .commandBuffer = cb},
+            {.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_SUBMIT_INFO, .commandBuffer = cb2},
+         };
+         VkSubmitInfo2 sub = {.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO_2, .waitSemaphoreInfoCount = 1,
+                              .pWaitSemaphoreInfos = &wi, .commandBufferInfoCount = 2, .pCommandBufferInfos = cbi,
+                              .signalSemaphoreInfoCount = 1, .pSignalSemaphoreInfos = &si};
+         vr = vkQueueSubmit2(d.q[0], 1, &sub, VK_NULL_HANDLE);
+      } else if (split == 2) {
+         /* Two vkQueueSubmit2 calls, no host wait between them; the second depends on the
+          * first only through submission order on the queue. */
+         vr = st_submit(d.q[0], cb, &w, 1, NULL, 0, VK_NULL_HANDLE);
+         if (!vr)
+            vr = st_submit(d.q[0], cb2, NULL, 0, &s, 1, VK_NULL_HANDLE);
+      } else {
+         vr = st_submit(d.q[0], cb, &w, 1, &s, 1, VK_NULL_HANDLE);
+      }
+      if (vr)
          return st_fail(r, "iteration %llu: submit %s", (unsigned long long)iter, st_vkr(vr));
       if ((vr = st_wait_value(&d, tl, value, 10 * ST_NS)))
          return st_fail(r, "iteration %llu: wait %s", (unsigned long long)iter, st_vkr(vr));

@scottjones scottjones left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

At 7c8c738 the only change from what we reviewed is the comment in hk_device.h, and it now says what the code does, so our run at 18ff4cd carries over and we didn't repeat it. That run was queue.depth and sparse.churn only, so as you say, nothing has run CTS on this branch.

The body's Fix section still says writes recorded in an earlier command buffer are covered, which is the sentence the comment just corrected.

We still want your Zink A/B before this goes past edge. One thing that narrows it: on our Neo at the current pin, Hyprland, quickshell and the portal map libgallium and not Honeykrisp, so that desktop isn't going through Zink. That's one boot, but it makes this a question for clients that choose Zink rather than for every session.

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

Approving for edge. I read this at 7c8c738 against 672657e, statically only: no build or hardware run on my side.

I agree with Scott's read of the mechanics:

  • The merge fold keeps the memory bits and the tail markers (hk_cmd_buffer.h:1396-1428, 1438-1440).
  • The in-place upgrade doesn't count a second flush (hk_cmd_dispatch.c:620-627).
  • The va_alias_resources count balances on every path in hk_buffer.c and hk_image.c, including the later allocation failures (hk_image.c:1817-1819).
  • With no alias resource, streams are unchanged except at host-read barriers.

There are three things for the follow-up, none of them a regression.

  1. The opener gap isn't only at the start of a command buffer. The alias flush needs current_cs.cs holding work (hk_cmd_buffer.c:1155-1161), and that stream gets closed partway through a command buffer:

    • by a timestamp (hk_sync.c:1345-1354)
    • by hk_sync_check_cs or hk_sync_get_cs_after
    • by query copies (hk_query_pool.c:1285)
    • by the deferred-clear replacement (hk_cmd_buffer.c:4243)

    So a fill through the sparse buffer, then vkCmdWriteTimestamp2, then a barrier, then a copy through the alias gets no memory-scope flush. #23's alias_opener covers this, and its comment says so. The hk_device.h comment here only names the first barrier of a command buffer.

  2. When the open stream holds unrelated work, graphics isn't ordered after the flush. The sequence:

    • transfer writes in stream A, which then ends;
    • stream B opens with unrelated compute;
    • an XFER→GFX barrier puts the memory-scope flush at the end of B;
    • the render's requirement is cut at last[XFER], which is A (hk_sync.c:385-387, 549-550).

    The render can therefore start before B's flush. #23 only adds hk_sync_after_stream() when alias_opener is true, which it isn't here because B holds work. Compute and transfer readers are fine, because they follow B on the CDM subqueue. Worth adding this case to the split sparse.churn run, or ordering the destination after the flush stream whenever it isn't the source stream.

  3. Graphics-only sources never ask for memory scope (src_kinds & (COMPUTE|XFER), hk_cmd_buffer.c:1157). A render that writes through a sparse image's own address, followed by a transfer or compute read through a plain alias, gets the plain post-graphics strong flush. That's untested rather than shown to fail. Please name it in the hk_device.h comment next to the update-after-bind case, until attachment and shader writes are covered by a test.

Nit: the Fix section of the body still says "writes recorded in an earlier command buffer are covered too", which the corrected comment contradicts.

+1 to Scott's Zink A/B before this goes past edge.

Name barriers after a compute stream closes as well as command-buffer openers. Also document the untested graphics-only source case, which keeps the ordinary strong flush. No behaviour changes.

(cherry picked from commit 1498a03)
@DjDeveloperr

Copy link
Copy Markdown
Collaborator Author

@maralcbr

  1. The comment now describes any barrier recorded with no open compute stream holding work, including one after a timestamp or query copy partway through a command buffer. Comment-only follow-up to merged #123: aurora-silicon/mesa#128, commit 1498a030ff6. Cherry-picked here as d9345510b19; hk: give G17P host-read and aliased-memory flushes memory scope #20's behavior is unchanged. The draft branches carry the same correction with their opener description preserved: ad6e364afd2 in #124 and 3bb4aba9b51 in hk: flush with memory scope for a G17P alias dependency recorded before any work #23.

  2. The source code supports your reading: the XFER source cut can name A while the memory-scope flush is in B. The code fix is confined to draft aurora-silicon/mesa#124, d2e11d960f8, cherry-picked into hk: flush with memory scope for a G17P alias dependency recorded before any work #23 as 78198e0ded5. After the alias flush, it extends a graphics destination's CDM requirement to B only when its existing cut precedes B or has no local cut. When the source stream carries the flush, its cut already includes it, so no ordering is added. Compute and transfer destinations retain their CDM subqueue ordering. The caller requires a live G17P alias resource and a compute/transfer source; devices without alias resources gain no ordering or flushes from this change. It adds no flush records. The required graphics wait has no measured hardware cost yet. The existing hk_sync_prior CPU harness now has cases for all three graphics destination classes, the source-stream case, and CDM-only destinations; these new cases have not run.

  3. The same comment commit names graphics-only sources next to update-after-bind: attachment or shader writes through a sparse image followed by transfer or compute reads through a plain alias still receive the ordinary post-graphics strong flush. No behavior change for that case. It remains untested, not demonstrated to fail.

The Fix-section body nit is already fixed; I left #20's body unchanged. #124 and #23's bodies now describe both the mid-command-buffer opener and the unrelated-compute-stream case, and retain their not-hardware-tested statement.

The gpubench scratch patch retains split variants 1/2 and adds GB_SPARSE_SPLIT=3 (timestamp between fills and barrier in one command buffer) and =4 (timestamp, unrelated compute in B, XFER-to-fragment barrier, render reading the plain pool alias). The changed C source compiles as one arm64 macOS object with -Wall -Wextra -Werror. The Linux target build and all new Mesa builds/suite runs are blocked: the shared build VM returned no output from repeated usage//bin/true probes and build requests. I canceled my pending requests and did not stop or restart the VM. No new Mesa test count or passing result is claimed, and the negative-control run of the sync test remains pending.

Nothing has run on the Neo: it is offline. The new variants, later-stream cache visibility, graphics-only source case, and the requested Zink A/B remain untested on hardware. Both code PRs remain draft; nothing was merged.

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

Still approving, at d934551. It changes only the hk_device.h comment, so the behavior is what I approved at 7c8c738, and CI is green.

The comment now covers gaps 1 and 3 and the body's Fix section is corrected, thanks. Two small things are left:

  • Neither the comment nor the body's "Not covered" section names gap 2 for this PR as it lands: when the flush goes into an unrelated open stream B, a graphics destination is still ordered only after the source stream, so it can start before B's flush. One sentence pointing to #23 would stop the comment implying that every recorded alias flush orders its graphics reader.
  • One comment line is 85 columns: "after such a barrier have no memory-scope flush between them. Whether the hardware".

The gate before this goes past edge is unchanged: the Zink A/B, plus the split sparse.churn variants once the Neo is back. I haven't reviewed #23 (now at 6a13909) for approval.

DjDeveloperr added a commit to aurora-silicon/mesa that referenced this pull request Oct 31, 2026
When the open compute stream holds work unrelated to the source, the
memory-scope flush goes at its end, but a graphics destination is still
ordered only after the source's stream, so it can start before the flush.
Say so, so the comment does not imply that every recorded alias flush
orders its graphics reader. Also rewrap the one line past 80 columns.

Comment only. Raised in review of omacom#20.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the open compute stream holds work unrelated to the source, the
memory-scope flush goes at its end, but a graphics destination is still
ordered only after the source's stream, so it can start before the flush.
Say so, so the comment does not imply that every recorded alias flush
orders its graphics reader. Also rewrap the one line past 80 columns.

Comment only. Raised in review of #20.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit f1a1e8b)
@DjDeveloperr

Copy link
Copy Markdown
Collaborator Author

@maralcbr Both are done, comment only.

  • Gap 2. The comment now says that when the open stream holds work unrelated to the source, the flush goes at its end, but a graphics destination is still ordered only after the source's stream, so it can start before the flush. The body's "Not covered" section says the same and points to hk: flush with memory scope for a G17P alias dependency recorded before any work #23.
  • Long line. The 85-column line is rewrapped; the comment is within 80 columns.

Upstream as aurora-silicon/mesa#128 f1a1e8b, cherry-picked here as 75238e5. Behavior is unchanged from 7c8c738.

DjDeveloperr added a commit that referenced this pull request Oct 31, 2026
…eam-opener

Keep this branch's alias-scope comment, which describes the flushes this
branch adds in place of the gaps #20's comment names.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@maralcbr
maralcbr merged commit a196911 into integration/aurora Oct 31, 2026
7 checks passed
ryanrhughes pushed a commit to omacom/omarchy-pkgs that referenced this pull request Oct 31, 2026
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
stevederico pushed a commit to stevederico/omarchy-pkgs that referenced this pull request Oct 31, 2026
…#1931)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
RyanTheTide pushed a commit to jgoyvaerts/mesa that referenced this pull request Nov 1, 2026
The comment on hk_cdm_alias_scope() said the memory-scope flush also
covers writes recorded in earlier command buffers. It does when a later
barrier records the flush: hk_CmdPipelineBarrier2() records it into the
open compute stream, and only when that stream already holds work
(hk_cdm_visibility_flush_needed() returns false at stats.cmds == 0). A
barrier recorded before any work in a command buffer has no such stream
and records no flush. Whether the hardware returns stale data then is
untested; the comment now says so instead of claiming coverage.

The "not covered" case also includes a descriptor set updated after bind
to reach a resource created after recording
(descriptorBinding*UpdateAfterBind is advertised).

From the review of omacom#20.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

3 participants