ggml-cuda : restore prop.integrated on HIP builds#24233
Conversation
PR ggml-org#16308 set info.devices[id].integrated = false unconditionally for all CUDA/HIP devices as a workaround for corrupted output on Jetson Orin (ggml-org#15034). On HIP/ROCm the device's real hipDeviceProp_t.integrated flag is needed: with the cached field forced to false, supports_buft() refuses CUDA host buffers on AMD APU/UMA parts, while get_type() already reads prop.integrated (ggml-org#23007) — an inconsistency that breaks integrated-GPU host-buffer use on ROCm. Guard the workaround so it only applies to non-HIP (CUDA) builds and restore prop.integrated for HIP, keeping the Jetson workaround intact for CUDA. Fixes ggml-org#23977 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
|
Hi @liminfei-amd, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
Thanks for the review and for the AI-usage policy reminder. Disclosure: I used an AI assistant to help draft the patch and this description, but the change is a minimal, human-directed fix that I fully understand and can maintain. Below is a line-by-line explanation and the hardware evidence behind it. Background
The change, line by line#if defined(GGML_USE_HIP)
info.devices[id].integrated = prop.integrated;
#else
info.devices[id].integrated = false; // Temporarily disabled due to issues with corrupted output (e.g. #15034)
#endif
Net effect: discrete AMD GPUs report Hardware validationReproduced and verified on an AMD Ryzen AI MAX+ 395 (gfx1151, RDNA3.5 APU) running ROCm 7.2.1:
Happy to adjust the wording if preferred — I can explain any part of the change in more detail. |
|
This came up in a different discussion, it's still valid, right? |
|
(Already several weeks ago) I had an off-site discussion with NVIDIA engineers regarding UMA in reaction to this PR. In the course of the discussion it became clear that getting the memory coherence right is non-trivial but one them said they "will work on it". However, nothing else came of it. If according to AMD engineers the program logic as it is being changed in this PR is correct for AMD hardware then my opinion is that we can just merge it since they very likely know better than me. |
|
Thanks @0cc4m for reopening this, and thanks @JohannesGaessler for the review and the NVIDIA context. Yes, this is still valid for HIP. We revalidated it on a gfx1151 APU: the ROCm backend changes from rejecting Short generation tests with both CPU-heavy and full-HIP offload completed normally, with no measurable performance regression. |
PR ggml-org#16308 set info.devices[id].integrated = false unconditionally for all CUDA/HIP devices as a workaround for corrupted output on Jetson Orin (ggml-org#15034). On HIP/ROCm the device's real hipDeviceProp_t.integrated flag is needed: with the cached field forced to false, supports_buft() refuses CUDA host buffers on AMD APU/UMA parts, while get_type() already reads prop.integrated (ggml-org#23007) — an inconsistency that breaks integrated-GPU host-buffer use on ROCm. Guard the workaround so it only applies to non-HIP (CUDA) builds and restore prop.integrated for HIP, keeping the Jetson workaround intact for CUDA. Fixes ggml-org#23977 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
|
Judging by the influx of issue reports that are traced back to this change, it might not have been correctly validated update. |
PR #16308 sets
info.devices[id].integrated = falseunconditionally for everyCUDA/HIP device — a workaround for corrupted output on Jetson Orin (#15034). On
HIP/ROCm this discards the device's real
hipDeviceProp_t.integratedflag.With the cached
integratedfield forced tofalse,supports_buft()refusesCUDA host buffers on AMD APU/UMA parts, even though
get_type()already readsprop.integrateddirectly (#23007). The two paths therefore disagree, breakinghost-buffer use on integrated GPUs under ROCm.
This guards the workaround to non-HIP (CUDA) builds and restores
prop.integratedfor HIP, so the Jetson workaround stays in place for CUDA whileAMD APUs report their true integrated status:
Validated on an AMD Ryzen AI MAX+ 395 (gfx1151 APU, ROCm 7.2.1): the HIP device
now reports
integrated=1and the cachedinfo.devices[id].integratedmatchesprop.integrated. Discrete AMD GPUs reportintegrated=0and are unaffected.Fixes #23977
This is an experimental, AI-generated code change, reviewed by AMD engineers before submission.