Repository navigation
libhsakmt(dxg): add cwsr allocation support in runtime - #10573
Conversation
- WDDMDevice::AllocateCwsrSize(): computes per-XCC control-stack +
wave/WG-state + debugger region sizes using the same constants as KMD
CalCwsrSaveAreaSize() (gfx10+: sgpr=20KB, hwreg=5KB, vgpr=256KB/CU;
older: sgpr=16KB, hwreg=4KB). Returns total page-aligned size for all
XCCs.
- WDDMDevice::FillCwsrHeader(): initializes HsaUserContextSaveAreaHeader
at the base of each per-XCC slot (ErrorEventId, ErrorReason, DebugOffset,
DebugSize; CtlStack/WaveState zeroed pending KMD write-back).
- CreateHwQueue(): allocates CWSR region in system (GTT) memory, fills
headers, and passes CwsrMemHandle (D3DKMT_HANDLE) to KMD via
FillinHwQueuePrivData / UMDKMDIF_CREATEHWQUEUE_PRIVATE_DATA.
- DestroyHwQueue(): frees CWSR memory when the queue is destroyed.
- ComputeQueue ctor: accepts EventId (from HsaEvent passed to
hsaKmtCreateQueueV2) to populate error_event_id_ / error_reason_ on
the queue, used by FillCwsrHeader.
✅ All Policy Checks Passed
📖 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 |
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
queue.h/device.h signature additions, queues.cpp event-id threading, and queue.cpp constructor wiring (error_reason_/error_event_id_) are faithful to the Linux fill_cwsr_header reference; DebugOffset stride and total-size layout match the KFD segregated [all-ctx][all-debug] layout.
[Important — no line anchor] Error-path CWSR leak in CreateHwQueue: cwsr_mem_ is allocated at line 1010 (inside if (queue->cwsr_mem_ == nullptr)). When D3DKMTCreateHwQueue fails further down, the failure branch only calls free(priv_data); return false; — it does not free cwsr_mem_. The sole caller WDDMQueue::HwsInit() propagates the error with return HSA_STATUS_ERROR and does not call DestroyHwQueue, so the CWSR GpuMemory leaks and cwsr_mem_handle_ is left dangling. Fix: free (and null) cwsr_mem_/cwsr_mem_handle_ in the D3DKMTCreateHwQueue failure branch before returning.
gandryey
changed the title
gandryey
changed the title
gandryey
changed the title
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author responses
Prior [Important] finding (CWSR memory leak on D3DKMTCreateHwQueue failure) rebutted: the Linux KFD path has the same behavior, and D3DKMTCreateHwQueue failure is a fatal path where the process does not continue. Accepted — the WDDM/DXG implementation intentionally mirrors Linux KFD for initial parity.
Nits addressed:
uint32_toverflow inwg_data_size: would require ~3200 CUs; Linux uses identical types. Accepted.HSAuint32truncation inFillCwsrHeader: the struct field definition is the constraint — widening the calculation operands would still truncate at assignment; Linux has the same narrowing. Accepted.
assert(Event == nullptr) removal accepted: only EventId is extracted downstream; the raw Event pointer never escapes the constructor.
Optional #2 (AllocateCwsrSize missing KFD clamps for gfx10.x control-stack cap and pre-Navi10 wave_num bound) remains open, non-blocking.
Verdict changes from request-changes to approve-with-nits.
There was a problem hiding this comment.
Pull request overview
This PR adds Windows (DXG/WDDM) runtime support for allocating and initializing a CWSR (Context Wave Save/Restore) region so the KMD can report queue exceptions back to the runtime/debugger, mirroring the Linux KFD queues.c behavior.
Changes:
- Add device-side CWSR size calculation and header initialization helpers, then allocate/free the CWSR region during HW queue create/destroy.
- Plumb the CWSR allocation handle into HW queue private data so it can be passed to the KMD.
- Propagate the queue creation
HsaEvent::EventIdintoComputeQueueand into the CWSR header (ErrorEventId), and remove the Windows-side assertion thatEventmust be null.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocr-runtime/libhsakmt/src/dxg/wddm/queue.cpp | Extends ComputeQueue construction to capture error reason pointer + event id for CWSR header population. |
| projects/rocr-runtime/libhsakmt/src/dxg/wddm/device.cpp | Implements CWSR sizing/header fill and wires allocation/cleanup into CreateHwQueue/DestroyHwQueue, passing the handle to KMD. |
| projects/rocr-runtime/libhsakmt/src/dxg/queues.cpp | Uses Event->EventId (when present) and passes it into ComputeQueue. |
| projects/rocr-runtime/libhsakmt/include/impl/wddm/queue.h | Adds per-queue state for CWSR allocation handle and error reporting metadata. |
| projects/rocr-runtime/libhsakmt/include/impl/wddm/device.h | Declares the new CWSR helper methods and includes needed HSAKMT types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update
Prior [Important] resolved: the CWSR cleanup block is now in the D3DKMT_CREATEHWQUEUE failure path — cwsr_mem_ is freed and zeroed before returning false.
All other findings validly rebutted:
- [Optional] CWSR leak on
DestroyHwQueuefailure: Linux path has identical behavior — accepted. - [Optional]
AllocateCwsrSizemissing caps: the original suggestion to addGetMinorVersion() == 0was incorrect (the Linux mask0x3f0000only covers bits 16–23, minor is irrelevant) — rebuttal accepted. - [Nit]
uint32_toverflow: 3200 CUs unrealistic, matches Linux — accepted. - [Nit]
HSAuint32truncation: struct field width is the binding constraint, not the cast — accepted. - [Question]
assert(Event == nullptr)removal:Event ? Event->EventId : 0is the full interface, nothing downstream sees the raw pointer — answered.
The "Add copilot suggestions" commit adds explanatory comments to FillCwsrHeader — no logic changes. Ready to approve.
TheRock Submodule Bump ActivityNewest first
|
Motivation
Exceptions support in Windows
Technical Details
Issue Tracking
Jira ID : AIRUNTIME-2309
Test Plan
N/A KMD team tested private runtime build during development
Test Result
KMD was able to report queue exceptions.
Submission Checklist
N/A