-
Notifications
You must be signed in to change notification settings - Fork 101
rocr: Remove dependency on KFD for vmem API to support xdnadriver #2515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the HSA vmem API to remove the dependency on the KFD driver, enabling support for XDNA devices. The main change consolidates driver-specific operations into their respective driver implementations rather than having them in the generic Runtime class.
Key changes:
- Introduces
Driver::CreateShareableHandleto encapsulate driver-specific handle creation logic previously inRuntime::GetAmdgpuDeviceArgs - Splits handle destruction into
DestroyShareableHandleandDestroyImportedShareableHandleto account for different reference counting behaviors between drivers - Updates XDNADriver to create anonymous mappings for memory-only allocations to ensure unique virtual addresses
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime.cpp | Removes GetAmdgpuDeviceArgs and refactors vmem mapping to use new driver API |
| runtime.h | Adds GetAmdgpuRenderFdFunc accessor and removes GetAmdgpuDeviceArgs declaration |
| driver.h | Defines new CreateShareableHandle, DestroyShareableHandle, and DestroyImportedShareableHandle APIs |
| amd_xdna_driver.h | Updates driver interface signatures and adds unmap_vaddr field to BOHandle |
| amd_virtio_driver.h | Updates driver interface signatures |
| amd_kfd_driver.h | Updates driver interface signatures |
| amd_xdna_driver.cpp | Implements new driver APIs and refactors memory allocation to support memory-only mode |
| amd_kfd_virtio_driver.cpp | Implements stub functions for new driver APIs |
| amd_kfd_driver.cpp | Moves vmem-related logic from Runtime into KfdDriver implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/rocr-runtime/runtime/hsa-runtime/core/driver/xdna/amd_xdna_driver.cpp
Outdated
Show resolved
Hide resolved
projects/rocr-runtime/runtime/hsa-runtime/core/driver/xdna/amd_xdna_driver.cpp
Show resolved
Hide resolved
projects/rocr-runtime/runtime/hsa-runtime/core/driver/kfd/amd_kfd_driver.cpp
Show resolved
Hide resolved
projects/rocr-runtime/runtime/hsa-runtime/core/driver/kfd/amd_kfd_driver.cpp
Outdated
Show resolved
Hide resolved
327b968 to
843330b
Compare
| MemoryHandle *memHandle = mappedHandle->mem_handle; | ||
|
|
||
| // Export memory from owner agent. | ||
| hsa_status_t status = memHandle->agentOwner()->driver().ExportDMABuf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these calls to ExportDMABuf/ImportDMABuf be replaced with CreateShareableHandle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the general case no, since there are potentially different source and target drivers involved.
It may be worth it as an optimization, i.e., source and target driver is the same. I'm not entirely sure.
b41af03 to
d97a7a1
Compare
da1c503 to
381e551
Compare
Motivation
The use of the HSA vmem API with XDNA devices was not possible due to its dependence on the amdgpu driver API use in runtime.cpp. This PR moves the relevant calls in the
KFDDriverand simplifies theDriverAPI thatRuntime::VMemoryHandleMapuses.Technical Details
Driver::CreateShareableHandlewas added that creates handles for vmem-related allocations.DestroyImportedShareableHandlevsDestroyShareableHandle).ThunkHandleis a only a pointer. This wastes an extra page for eachXDNADriver-based allocation if not needed to be mapped at allocation, but it guarantees the address uniqueness among drivers.Follow-ups:
core::ShareableHandle). They may need to be separated to two, one handle that does not own an allocation (core::ShareableHandle) and one that does (core::MemoryHandle); the latter is probably similar toRuntime::MemoryHandle. Further discussion is welcome.ThunkHandledefinition creates the need to waste a page inXDNADriveralthough we already have a BO handle. It's unclear how to address that without an extra mapping, e.g.,ThunkHandle-> {driver,handle}.fn_amdgpu_device_get_fdis still set byRuntime. It's unclear if this needs to move inKFDDriveror not.JIRA ID
NA
Test Plan
Test Result
Successful run.
Submission Checklist