Repository navigation
[1/5] rocjitsu: Fix KFD queue compatibility and AQL slot retirement - #12934
Conversation
Groverkss
commented
Oct 6, 2026
Groverkss
commented
- Accept older CREATE_QUEUE payload sizes by zero-extending requests and copying back only the caller-provided bytes.
- Allow clients to unmap complete doorbell views while preserving the simulator's backing and private monitoring alias until process teardown.
- Invalidate consumed AQL packet headers before publishing the read index, retaining admission state across transient memory failures.
Groverkss
changed the title
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The advertised runtime-cts CI integration is absent from the changed files.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Improves rocjitsu’s KFD and AQL compatibility, with regression coverage for payload sizing, doorbell remapping, and packet retirement.
Changes:
- Supports variable-sized CREATE_QUEUE payloads.
- Preserves doorbell backing across client unmaps.
- Invalidates AQL slots before cursor publication with retry-safe state.
| File | Description |
|---|---|
simulated_kfd_test.cpp |
Tests persistent doorbell backing. |
kfd_ioctl_test.cpp |
Tests variable payload sizes. |
gpu_vm_service_test.cpp |
Tests AQL release ordering and retries. |
cdna5_sdma_test.cpp |
Updates packet-read fault hooks. |
dispatch_entry.h |
Stores pending AQL slot release state. |
command_processor.cpp |
Implements ordered slot invalidation. |
simulated_kfd.cpp |
Updates ioctl and doorbell-unmap behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
AQL fetch no longer acquires packets published with a 16-bit header — emulation/rocjitsu/lib/
rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.cpp:4403
Reasoning: The PR changes the header load from a 16-bit acquire to a 32-bit acquire. A 32-bit atomic
load does not synchronize with a producer’s 16-bit release store. For a VM-polled queue, the CP can
fetch after the write index advances but before a doorbell provides another acquire edge. It can
then see a valid header while reading stale setup or packet body data.
Evidence: ROCr still release-stores a 16-bit header in projects/rocr-runtime/runtime/hsa-runtime/
core/runtime/intercept_queue.cpp:319 (and for its retry barrier at line 276). The CP’s VM-polled
fetch path does not clamp admission to the doorbell (command_processor.cpp:4374–4389), and it now
takes setup from the 32-bit load at line 4433. The new slot-release test uses a fully written
packet, so it does not exercise concurrent publication.
Suggested fix: Define a publication and acquire protocol that covers both 16-bit and 32-bit
producers. Keep a matching 16-bit acquire for supported 16-bit publishers, or move those publishers
to a documented common width, and add a concurrent test for each supported form.
Confidence: High in the synchronization mismatch; a runtime manifestation has not been reproduced.
f7388a0 to
0029cfe
Compare
Clients built with older KFD headers send an 88-byte CREATE_QUEUE request. Exact-size dispatch silently ignored it, leaving the queue and doorbell unset. Zero-extend the supplied payload into the current structure, dispatch queue creation, and copy back only the caller-provided bytes.
Runtime clients finish queue work but fail teardown because munmap rejects their doorbell view while the process remains open. Allow complete client views to be unmapped while retaining the canonical backing and private monitoring alias until process teardown. Serialize unmapping with mapping replacement and retain protection for the private alias.
AQL producers observe an advanced read index while the consumed slot still contains a valid packet header, breaking ring reuse and drain checks. Release the packet format with a 32-bit atomic store before publishing the cursor. Retain the admitted packet snapshot across transient store failures so retries neither replay work nor release a different mapping.
0029cfe to
a631325
Compare
gfx1250 B0-to-A0 translation baselineWarning The PR result is not directly comparable to the latest develop baseline. Changed provenance:
Compared PR head |
Groverkss
changed the title
Addressed |
TheRock Submodule Bump ActivityNewest first
|
…12934) - Accept older CREATE_QUEUE payload sizes by zero-extending requests and copying back only the caller-provided bytes. - Allow clients to unmap complete doorbell views while preserving the simulator's backing and private monitoring alias until process teardown. - Invalidate consumed AQL packet headers before publishing the read index, retaining admission state across transient memory failures.
