Added support for MCDM and GPU-PV through the DXCore API - #72
Conversation
|
🎉 Thank you for your contribution, we really appreciate it! 🎉 Like many open source projects, we require contributors to sign our Contributor License Agreement (CLA). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility. ✍ To sign, please post a new comment on this PR with exactly the following text: ✍ I have read and agree to the Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds C-compatible DXCore interfaces and integrates DXCore adapter matching, VRAM budget polling, fallback WDDM queries, re-initialization handling, and cleanup into Windows shared-memory detection. The ChangesDXCore VRAM budget detection
Sequence Diagram(s)sequenceDiagram
participant CUDADevice
participant aimdo_wddm_init
participant DXCoreFactory
participant DXCoreAdapter
participant poll_budget_deficit
CUDADevice->>aimdo_wddm_init: provide InstanceLuid
aimdo_wddm_init->>DXCoreFactory: enumerate adapters
DXCoreFactory-->>aimdo_wddm_init: adapter list
aimdo_wddm_init->>DXCoreAdapter: match InstanceLuid
DXCoreAdapter-->>aimdo_wddm_init: matched adapter
aimdo_wddm_init-->>CUDADevice: initialization success
CUDADevice->>poll_budget_deficit: poll memory budget
poll_budget_deficit->>DXCoreAdapter: QueryState local segment budget
DXCoreAdapter-->>poll_budget_deficit: budget and usage
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-win/shmem-detect.cpp`:
- Around line 55-61: The initialization of `AimdoHipDeviceProp` in
`shmem-detect.cpp` only zeroes the first union member, so `hip_props.bytes` may
contain uninitialized data before `g_device_get_properties` fills it. Update the
`__HIP_PLATFORM_AMD__` handling around `hip_props` to zero the entire union
storage before use (or otherwise ensure the full `bytes` buffer is initialized),
then keep using `g_device_get_properties` and `hip_props.luid` as the source for
the LUID copy. This should be fixed in the same block where `CHECK_CU` and
`memcpy` are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c535a26-889f-4835-a887-bc010fe76535
📒 Files selected for processing (3)
src-win/shmem-detect.csrc-win/shmem-detect.cppsrc/plat.h
💤 Files with no reviewable changes (1)
- src-win/shmem-detect.c
| #if defined(__HIP_PLATFORM_AMD__) | ||
| AimdoHipDeviceProp hip_props = {0}; | ||
| if (!g_device_get_properties || !CHECK_CU(g_device_get_properties(hip_props.bytes, dev))) { | ||
| log(ERROR, "comfy-aimdo: failed to get HIP device properties\n"); | ||
| return false; | ||
| } | ||
| memcpy(&cuda_luid, hip_props.luid, sizeof(cuda_luid)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Zero-init the whole union with memset, or don't trust bytes beyond the first member.
AimdoHipDeviceProp hip_props = {0}; only initializes the first union member (the ~280-byte struct); the remaining storage up to bytes[4096] isn't guaranteed zeroed. You then hand hip_props.bytes to g_device_get_properties and read hip_props.luid back. If the property call doesn't populate the LUID region fully, you'd match against uninitialized memory. Cheap fix, and it hushes the Cppcheck UnionZeroInit warning too.
🛡️ Proposed fix
- AimdoHipDeviceProp hip_props = {0};
+ AimdoHipDeviceProp hip_props;
+ memset(&hip_props, 0, sizeof(hip_props));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if defined(__HIP_PLATFORM_AMD__) | |
| AimdoHipDeviceProp hip_props = {0}; | |
| if (!g_device_get_properties || !CHECK_CU(g_device_get_properties(hip_props.bytes, dev))) { | |
| log(ERROR, "comfy-aimdo: failed to get HIP device properties\n"); | |
| return false; | |
| } | |
| memcpy(&cuda_luid, hip_props.luid, sizeof(cuda_luid)); | |
| `#if` defined(__HIP_PLATFORM_AMD__) | |
| AimdoHipDeviceProp hip_props; | |
| memset(&hip_props, 0, sizeof(hip_props)); | |
| if (!g_device_get_properties || !CHECK_CU(g_device_get_properties(hip_props.bytes, dev))) { | |
| log(ERROR, "comfy-aimdo: failed to get HIP device properties\n"); | |
| return false; | |
| } | |
| memcpy(&cuda_luid, hip_props.luid, sizeof(cuda_luid)); |
🧰 Tools
🪛 Cppcheck (2.21.0)
[portability] 56-56: Zero initializing union 'hip_props' does not guarantee its complete storage to be zero initialized as its largest member is not declared as the first member. Consider making bytes the first member or favor memset().
(UnionZeroInit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-win/shmem-detect.cpp` around lines 55 - 61, The initialization of
`AimdoHipDeviceProp` in `shmem-detect.cpp` only zeroes the first union member,
so `hip_props.bytes` may contain uninitialized data before
`g_device_get_properties` fills it. Update the `__HIP_PLATFORM_AMD__` handling
around `hip_props` to zero the entire union storage before use (or otherwise
ensure the full `bytes` buffer is initialized), then keep using
`g_device_get_properties` and `hip_props.luid` as the source for the LUID copy.
This should be fixed in the same block where `CHECK_CU` and `memcpy` are used.
Source: Linters/SAST tools
rattus128
left a comment
rattus128
left a comment
There was a problem hiding this comment.
Please keep it in pure C.
That's a really strict tandard. 😂 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-win/shmem-detect.c`:
- Around line 79-91: Update the DXCore match path in the adapter discovery logic
so it does not return before initializing g_wddm_adapter; continue into
DXGI/WDDM discovery after a DXCore match. In the fallback condition around the
DXCore budget query and WDDM setup, attempt WDDM whenever the DXCore query is
unsuccessful, including when g_dxcore_adapter is already non-null, rather than
gating it solely on the pointer being null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5a408e7-87e1-465e-970e-44cb7485986a
📒 Files selected for processing (3)
src-win/dxcore_c.hsrc-win/shmem-detect.csrc/plat.h
💤 Files with no reviewable changes (1)
- src/plat.h
2 participants
Contribution Agreement
By introducing Microsoft's DXCore API, more modern computing device models can be supported, such as MCDM and GPU-PV.
After applying this patch, I can now load aimdo on my A100 (running in MCDM mode) and enable new features such as dynamic vram and multi-threaded loading.
DXCore can only be called via C++. After applying this patch, this project will depend on the MSVC++ runtime!Maybe we can use the
/MTcompiler parameter to avoid this, anddxcore.libalso needs to be added to the compilation command in workflow: