Repository navigation
rocjitsu: unify compute queue ownership and packet format setup - #12544
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
PM4 retry snapshots, ring reconfiguration, idle reporting, and encoded scheduling percentages have correctness gaps.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Unifies AQL, native PM4, and DRM compute queues under shared ownership, lifecycle, and command state.
Changes:
- Introduces common compute queue configuration, records, and binding factory.
- Integrates PM4 ring processing, retries, dispatches, and teardown into
CommandProcessor. - Enables native KFD PM4 queues with target-XCC selection and expanded regression coverage.
| File | Description |
|---|---|
tests/xcd_fanout_kfd_test.cpp |
Updates unified queue terminology. |
tests/xcd_distribution_test.cpp |
Uses common queue configuration. |
tests/pm4_queue_controller_test.cpp |
Adds PM4 retry, wrap, snapshot, and cancellation tests. |
tests/pci/gpu_pci_device_test.cpp |
Uses the common binding factory. |
tests/kfd_ioctl_test.cpp |
Tests native PM4 queue creation and XCC placement. |
tests/gpu_vm_service_test.cpp |
Migrates queue lifecycle tests to common state. |
tests/gpu_vm_pipeline_test.cpp |
Updates queue configuration type. |
tests/execution_plugin_test.cpp |
Updates queue configuration type. |
tests/completion_tracker_test.cpp |
Uses unified queue records. |
tests/check_sdma_frontend_boundary.cmake |
Extends boundary checks for the new factory. |
tests/check_cp_sdma_boundary.cmake |
Updates command-processor source boundaries. |
tests/cdna5_sdma_test.cpp |
Migrates direct queue registrations. |
tests/aql_queue.h |
Uses common queue configuration. |
tests/amdgpu_vm_test.cpp |
Migrates AQL and DRM queue tests. |
vm/soc.cpp |
Routes AQL bindings through the common factory. |
pm4/pm4_queue_binding_factory.cpp |
Retains PM4 factory as an adapter. |
pm4.h |
Generalizes PM4 stream and command state. |
dispatch_entry.h |
Defines unified queue configuration and runtime state. |
compute_queue_binding_factory.h |
Declares the common binding factory. |
compute_queue_binding_factory.cpp |
Implements AQL and PM4 bindings. |
completion_tracker.h |
Accepts unified queue records. |
completion_tracker.cpp |
Migrates completion processing. |
command_processor.h |
Exposes unified queue APIs and storage. |
command_processor.cpp |
Consolidates queue execution and lifecycle logic. |
CMakeLists.txt |
Builds the renamed common factory. |
simulated_kfd.cpp |
Enables native KFD PM4 queues and XCC selection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kuhar
left a comment
There was a problem hiding this comment.
Can we migrate the remaining controller contract tests to CommandProcessor and update the ownership documentation as part of this change or a focused follow-up? Production queues now use the unified record, but emulation/rocjitsu/tests/pm4_queue_controller_test.cpp still exercises the old controller for reconfiguration and retry behavior, allowing those tests to pass while the production path differs.
The same cleanup should update emulation/rocjitsu/docs/architecture.md:142, docs/vm-design.md:36, and lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.cpp:4783 (the latter two relative to emulation/rocjitsu/). They still describe the old owner or separate PM4 locking, although command streams now run under hw_queue_mutex_. Retire the unused implementation when those tests are migrated; if it must remain supported, document that boundary.
f47ccff to
bdac8f4
Compare
Represent AQL rings, PM4 rings, and DRM submissions with one compute queue record and binding lifecycle. Keep the initial packet format and its cursor units separate from persistent CP registers and command frames. Route native KFD PM4 queues through the common owner, preserve target-XCC selection, and retain VM snapshots and durable ring cursor publication across retries. Cover registration, teardown, ring wrapping, VM replacement, and native PM4 admission.
Preserve retry snapshots, defer transient publication retries, validate native PM4 notifications, and retain suspend/resume progress. Reject unsupported polling and register writes, include PM4 work in idle detection, and decode KFD scheduling independently from target XCC bits. Migrate PM4 queue contract coverage to CommandProcessor, remove the unused controller, and update ownership documentation. Validate with 414 focused CPU/simulator tests.
bdac8f4 to
bcf375f
Compare
Decode ROCr vendor IB envelopes and execute their PM4 streams using the parent compute queue registers, VM ownership, and CU selection. Return to AQL decoding after the outer stream ends; nested IBs return to their PM4 caller. Delay vendor completion until child dispatches retire. Handle CDNA PRED_EXEC and ACQUIRE_MEM layouts and share completion across XCD replicas. Cover blocked and nested IB return, persistent registers, shader dispatch, malformed streams, and exactly-once fanout completion. rocjitsu: fix vendor PM4 faults and profiler packet support Merge the compute queue lifecycle fixes while preserving the existing PR commits. Route immediate and resumed vendor stream failures through the parent AQL fault path, cancel peer streams, and keep callbacks from retaining erased entries or invalidating their own iteration. Accept the IB cache and temporal hint fields emitted by aqlprofile while retaining validation of unsupported control bits. Support CDNA5 PRED_EXEC with distinct per-XCD grid sizes under single-threaded and per-XCD execution. Exercise valid vendor packets with both barrier-bit values. Validation: 475 focused CPU/simulator tests and four architecture boundary checks. Integration for the runtime torture branch: Reuse #12546 rather than reimplement the vendor IB feature. Apply its remaining changes on top of #12544, retaining develop's graphics-packet support and the torture fixes. Local validation: shared interposer/CLI/tests build, 64 focused AQL, PM4 and XCD tests pass. The aql_indirect_buffers manifest cases on both gfx1201 and gfx1250 reach their data/completion checks, then expose the separate missing AQL slot-invalidation bug. Co-authored-by: Jakub Kuderski <jakub@nod-labs.com>
TheRock Submodule Bump ActivityNewest first
|


Motivation
Represent AQL and PM4 as the initial packet format of a compute queue, with one owner for registration, VM lifetime, CP register state, dispatches, and teardown.
Technical Details
ComputeQueueConfigandComputeQueueRecord.KFD reference:
set_queue_properties_from_usermaps COMPUTE and COMPUTE_AQL to the same compute queue type, with format configured separately.Existing exception handling is retained; this change adds no explicit throw statements or try/catch blocks.
Issue Tracking
Related discussion: #12526. This PR establishes the shared queue model used by the follow-up IB fix.
Test Plan
rocjitsu_testsandinterposer_dup_testwith AMD Clang in Release mode, including the existing-Werrorchecks.InterposerPm4Testcases with the gfx1100_w7900 and gfx1201_r9700 emulator configurations.Test Result
git diff --checkpassed.Submission Checklist