Repository navigation
feat(rocr): Implement ROCr APIs for GL2 residency control - #10183
amd-hsivasun merged 1 commit into
Conversation
bimaniamd
commented
Aug 14, 2026
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
Reviewed the projects/rocr-runtime/ changes for the GL2 residency-control path (libhsakmt, KFD driver, GpuAgent, public hsa_ext_amd surface, rocrtst). The UAPI amdgpu_drm.h edits are consistently synced across the three trees. Left inline comments on several blocking items — see below.
f65ee95 to
b7be71b
Compare
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update
The update adds the previously-missing gl2_cache_test.cc/.h, so the rocrtst target now compiles, and the new GL2CacheTest::SetUp() correctly calls TestBase::SetUp() (platform filtering preserved). Good.
However, all four prior [Important] correctness findings remain unaddressed at head:
libhsakmt/src/memory.c—hsaKmtSetPersistingCacheSizeCtxstill fills adrm_amdgpu_vmstruct but submits it viaAMDKFD_IOC_AVAILABLE_MEMORY; still has strayprintf,pr_debug,//TODO: BINCYmarker, andreturn 0;.amd_gpu_agent.cpp(HSA_AMD_AGENT_INFO_MAX_PERSISTING_L2_CACHE_SIZE) — braces were added to theif, butreturn HSA_STATUS_SUCCESS;is still inside theforloop, so onlycache_props_[0]is examined and*valueis left uninitialized when the first entry isn't the L2 persisting cache.- ABI width —
hsa_ext_amd.hstill documentssize_twhileGetInfo/SetAgentAttributeread/write via*((uint32_t*)value)(backing field stillsize_t). - rocrtst YAML registration — the six new
GL2_PersistingCache_*tests still aren't intest_categories.yamlorplatform_config.yaml, so CI won't schedule/filter them.
Debug/WIP markers also remain across the diff (//TODO: BINCY, Bincy TODO remove). Still request-changes.
b7be71b to
0d9df8d
Compare
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after force-push (0d9df8d)
Thanks for the updates. Two of the prior [Important] issues are now resolved:
- ioctl fixed:
hsaKmtSetPersistingCacheSizeCtxnow resolves the amdgpu render-node fd and issuesdrmIoctl(DRM_IOCTL_AMDGPU_VM, ...)(withpr_debug+ properHSAKMT_STATUSreturns) instead of the mismatchedAMDKFD_IOC_AVAILABLE_MEMORYon adrm_amdgpu_vmstruct. - loop early-return fixed: the max-size query is now
GetMaxPersistingL2CacheSize(), which iterates correctly and returns 0 when absent, andpersisting_l2_cache_size_is initialized to 0 inInitCacheList().
Still outstanding before merge:
- [Important] size_t vs uint32_t ABI mismatch — the public header still documents these attributes as
size_t, butGetInfo/SetAgentAttributeread/write them asuint32_t. See inline comments. - [Important] rocrtst config not updated — neither
rocrtst/config/test_categories.yamlnorplatform_config.yamlregisters the six newGL2_PersistingCache_*tests, so CI won't bucket/filter them. (GL2CacheTest::SetUpdoes callTestBase::SetUp, so platform filtering itself is preserved — good.) - [Nit] WIP markers — 4
//TODO:BINCY remove/Bincy TODO removelines remain, two of them in the publichsa_ext_amd.hheader on the newly exported ABI. Public exports are effectively permanent; please strip these before merge.
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author replies (2026-09-01)
Author response accepted: The argument for hsa_amd_agent_set_attribute using an extensible enum (mirroring the existing hsa_amd_agent_info_t pattern) is valid — new attributes can be added over time without new exported symbols. Dropping the prior [Optional] API-shape concern.
Two [Important] issues remain unaddressed:
-
ABI width mismatch (
hsa_ext_amd.h+amd_gpu_agent.cpp): the public header documents these attributes assize_tand the enum comment repeats that, butGetInfo/SetAgentAttributecast through(uint32_t*)andGetMaxPersistingL2CacheSize()returnsuint32_t. A caller that allocates asize_t(8-byte) buffer per the documented contract gets only 4 bytes written on a query. Choose one width and align header docs, casts, and the backing member. -
rocrtst YAML registration (
test_categories.yaml+platform_config.yaml): the six newGL2_PersistingCache_*tests still have no entries in either file — CI won't bucket or filter them.
WIP markers (//TODO:BINCY remove, /** Bincy TODO remove */) still present in hsa_ext_amd.h on newly exported ABI.
Note: @marifamd has CHANGES_REQUESTED on the amdsmi/rocm-smi-lib portions — the rocm-smi-lib update was flagged as unnecessary (slated for deprecation), and the amdsmi copy needs confirmation that the kernel UAPI adds the corresponding field.
89bbde0 to
71b3212
Compare
asierrag-amd
left a comment
There was a problem hiding this comment.
Here are some comments based on the test validation.
66edb07 to
4ac9682
Compare
Already incorporated the changes requested
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after the 2026-09-02 update
Thanks for the rework — all four of my prior [Important] items are resolved:
- KFD ioctl / arg-struct: now goes through the amdgpu render-node
drmIoctl(DRM_IOCTL_AMDGPU_VM)path instead ofAMDKFD_IOC_AVAILABLE_MEMORYon adrm_amdgpu_vmstruct. - Query loop early-return: refactored into
GetMaxPersistingL2CacheSize()(loops correctly, returns 0 when absent; member initialized to 0). size_tvsuint32_tABI width: bothHSA_AMD_AGENT_INFO_*_PERSISTING_L2_CACHE_SIZEqueries and theHSA_AMD_AGENT_ATTRIBUTE_REQUEST_PERSISTING_L2_CACHE_SIZEsetter now usesize_t, matching the header docs and the backing member.- rocrtst YAML registration: the six
GL2_PersistingCache_*tests are now in bothtest_categories.yamlandplatform_config.yaml.
The //TODO:BINCY markers and the out-of-scope amdsmi/rocm-smi-lib amdgpu_drm.h copies are gone too.
I concur with @dayatsin-amd's CHANGES_REQUESTED and defer to those items — I'm not re-posting them. Flagging the ones that are correctness/build-blocking rather than style: bump HSA_AMD_INTERFACE_VERSION_MINOR for the new API; MIT SPDX headers on the new gl2_cache_test.{h,cc}; and, per @asierrag-amd, actually wire the hsa_amd_agent_set_attribute export so it resolves at runtime (the test hit set_attr_fn == nullptr / "not found in runtime"), plus the rocprofiler-sdk registration for the new symbol (ref #10895). No new blocking items from me beyond those.
4ac9682 to
79f511b
Compare
shwetagkhatri
left a comment
There was a problem hiding this comment.
In runtime/hsa-runtime/inc/hsa_api_trace_version.h (HSA_AMD_EXT_API_TABLE_STEP_VERSION):
This still reads 0x13, but adding a function to AmdExtTable needs a bump to 0x14.I think you may already have a reason to not declare it?
This is separate from the HSA_AMD_INTERFACE_VERSION_MINOR bump you already did (that one is the public header API version. This one is the ext-table ABI version):
- Your rocprofiler-sdk changes are currently dead code. Everything added in amd_ext_api_id.h, api_args.h, abi.cpp and hsa.def.cpp is wrapped in #if HSA_AMD_EXT_API_TABLE_STEP_VERSION >= 0x14. Until this is bumped, none of it compiles in, so the new API can't actually be traced.
- ABI: Also the table grew by one pointer (expected_amd_ext_table_size went 752 to 760 in hsa_api_trace.cpp).
Leaving the version at 0x13 means two different table layouts share one version number, so layers above ROCr that are built against the old 0x13 can't tell the new slot exists. The version bump is important. Please set it to 0x14 to match the >= 0x14 guards you wrote.
112a7c6 to
902e951
Compare
902e951 to
75e872f
Compare
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after 75e872f (2026-09-10)
All my prior [Important] findings remain resolved. dayatsin-amd approved on 2026-09-03.
Looking at the new commit relative to shwetagkhatri's CHANGES_REQUESTED (2026-09-03):
- HSA_AMD_EXT_API_TABLE_STEP_VERSION bumped 0x13 → 0x14 in
hsa_api_trace_version.h✓ - hsa_api_trace.cpp self-recursion fixed:
amd_ext_api.hsa_amd_agent_set_attribute_fn = AMD::hsa_amd_agent_set_attribute(correct namespace qualifier) ✓ - KfdVirtioDriver::SetPersistingCacheSize override added (returns
HSA_STATUS_ERROR_INVALID_AGENT) ✓ - rocprofiler-sdk registration files updated (
VERSION,amd_ext_api_id.h,api_args.h,abi.cpp,hsa.def.cpp,enum_string.hpp) ✓ - License headers on
gl2_cache_test.handgl2_cache_test.cc✓
All three of shwetagkhatri's items appear addressed. Pending her re-review of the latest commit.
75e872f to
f13020c
Compare
f13020c to
fae8868
Compare
## Motivation
New feature implementation for the GL2 residency control.
## Technical Details
rocr API's
HSA_AMD_AGENT_INFO_LIMIT_PERSISTING_L2_CACHE_SIZE
HSA_AMD_AGENT_ATTRIBUTE_REQUEST_PERSISTING_L2_CACHE_SIZE.
And their related structures added
## Issue Tracking
JIRA ID: AILIROCR-405
## Test Plan
Added with this change
## Test Result
Awaiting results.
## Submission Checklist
- [x] Look over the contributing guidelines at
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
fae8868 to
59524fc
Compare
|
Bypassed, CI failures are unrelated. |
Resolve thunk_loader.h against develop's thunk-loader platform split (#7125) and the GL2 residency entry point (#10183): keep this branch's nullptr-initialized table and add hsaKmtSetPersistingCacheSize to it. Bind hsaKmtSetPersistingCacheSize as an optional entry point, like hsaKmtSetSigbusDelay. librocdxg does not export it, so as a required export it would make LoadThunkApiTable() fail and hsa_init() refuse to start under WSL. The static path defaults it to the NOT_SUPPORTED stub and the Linux BindPlatformStaticApis() binds the libhsakmt implementation, which was otherwise left unbound.
TheRock Submodule Bump ActivityNewest first
|