Repository navigation
Studio: find the AMD Vulkan driver through the Windows device registrations - #10564
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Follow-ups from reviewing the device-registry discovery, all checked against windows_get_device_registry_files in the Vulkan loader's loader_windows.c. Skip devnodes pending reboot. An Adrenalin update writes VulkanDriverName and drops its manifest before the restart that binds the driver. In between, the adapter is present, the file is on disk, and the loader skips the devnode (DN_HAS_PROBLEM with CM_PROB_NEED_RESTART or DN_NEED_RESTART). Counting it routed a gfx115x host to the Vulkan bundle for a driver that cannot load yet, which is the silent CPU fallback _amd_vulkan_icd_present exists to prevent. CM_Get_DevNode_Status now gates each device, and an unreadable status is skipped for the same reason the loader skips it. Retry the device-id list on CR_BUFFER_SMALL. A device arriving between the size query and the read makes the block outgrow the buffer, and the API refuses rather than truncating. That transient reported presence as unknown, both classes were skipped, and the host this feature exists for quietly installed the HIP bundle. The loader loops on the same condition; this bounds the loop at four attempts so a churning list answers unknown instead of spinning. Scope the registry catch to one instance rather than the whole class. A single unreadable entry discarded every remaining instance in that class, and on a two-adapter host the integrated part often enumerates first, so the AMD registration was the one lost. Tests: pending-reboot and unrelated problem codes, an unreadable status, a device arriving mid-enumeration, a list that never settles, and one unreadable instance not costing the rest of the class. Also cover what the existing suite could not reach. _FakeCfgMgr answers through native ctypes, where sizeof(c_wchar) is 4 on Linux and 2 on Windows, so the byte-to-element conversion for CM_Get_DevNode_Registry_PropertyW was only ever exercised at the runner's width; a ctypes shim now runs the probe at Windows' widths, and reverting the conversion fails it. Registration paths are checked through ntpath, since tmp_path made every existing case posixpath, where "C:\..." is not absolute. The loader-override test now asserts that neither the registry nor cfgmgr32 was touched. The restricted-subkey case gained a digit-named instance, without which the PermissionError arm was unreachable. _CR_FAILURE was 0x0D, which is CR_NO_SUCH_DEVNODE; cfg.h puts CR_FAILURE at 0x13.
|
Reviewed this and pushed a follow-up commit to the branch (ef247c6). Summary of what I checked and what changed. VerificationLinux host, so no Windows execution, no AMD driver load and no inference. What that leaves answerable is the discovery logic, the routing consequences, and the byte/character arithmetic under a modelled Windows data layout.
What the follow-up changesThree things where the implementation diverged from
Plus test coverage for holes the existing doubles could not reach:
Left as-is, worth recordingSoftwareComponents are still enumerated by presence rather than reached as children of a present display adapter, which is what the loader does ( Relative Nice fix, and thanks for the live A/B on real hardware. |
|
Also merged current main into the branch, which settles the CI question. Every one of the five
|
|
this matches the gap I reproduced in a controlled Windows A/B test, automatic picked ROCm with adapter-only registration and switched to Vulkan when discovery included it, restoring the original discovery switched it back to ROCm, all 13 cases passed, good to see your hardware test confirms it too |
|
@codex review |
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f6c22a273
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return False | ||
| if not status.value & _DN_HAS_PROBLEM: | ||
| return True | ||
| return problem.value not in (_CM_PROB_NEED_RESTART, _DN_NEED_RESTART) |
There was a problem hiding this comment.
When a driver update is pending a reboot and CM_Get_DevNode_Status reports DN_NEED_RESTART, that constant is a flag in status, not a value in problem. Comparing problem.value with 0x100 therefore misses this state (and the preceding early return also accepts it when DN_HAS_PROBLEM is clear), so the newly registered manifest can be treated as usable before the driver is bound, routing automatic selection to Vulkan and potentially leaving inference on CPU. Test status.value & _DN_NEED_RESTART directly and reserve the problem comparison for CM_PROB_NEED_RESTART.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in 8e4ffd8. cfg.h lists DN_NEED_RESTART (DN_LIAR, 0x100) under "Device Instance status flags, returned by call to CM_Get_DevInst_Status", and the CM_PROB_ space stops at 0x39 (CM_PROB_GUEST_ASSIGNMENT_FAILED), so comparing it against pulProblemNumber could never match. Reproduced both halves against the code at head: a devnode with the bit set in status and DN_HAS_PROBLEM clear took the early return and was counted, and so did the bit alongside an unrelated problem code. The check now tests status.value & _DN_NEED_RESTART before the problem word and independently of DN_HAS_PROBLEM, and keeps the problem comparison for CM_PROB_NEED_RESTART alone. The regression test carried the same mistake (it passed 0x100 as a problem code, which is why it passed); it is now parametrized over the problem code, the status bit alone, and the status bit beside an unrelated problem, and the last two fail against the previous check. Worth noting the Vulkan loader has the same expression in windows_get_device_registry_files, so this is stricter than the loader now, which is the right direction here: a device awaiting a restart has no bound driver whether or not the loader reads its key.
…oblem code cfg.h lists DN_NEED_RESTART (DN_LIAR, 0x100) under "Device Instance status flags, returned by call to CM_Get_DevInst_Status", so it is a bit in pulStatus. The check compared it against pulProblemNumber, where CM_PROB_ codes stop at 0x39, so that half of the comparison could never match. A devnode reporting the status bit without also raising DN_HAS_PROBLEM took the earlier return and was counted, which is the pending-reboot window the check exists to exclude: the manifest is registered and on disk, the driver is not bound until the restart, and automatic selection would move a gfx115x host to Vulkan and leave inference on CPU. Test the bit in the status word, before the problem word and independently of DN_HAS_PROBLEM, and reserve the problem comparison for CM_PROB_NEED_RESTART. The parametrized regression test carried the same mistake, passing 0x100 as a problem code; it now covers the status bit alone and the status bit beside an unrelated problem, and both fail against the previous check.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4 participants
Problem
Windows Strix Halo can still show Automatic (ROCm) despite having a working Vulkan driver and the gfx1150/gfx1151 preference introduced in #10381.
_amd_vulkan_icd_manifest_paths()only checkedHKLM\SOFTWARE\Khronos\Vulkan\Drivers. On the tested Radeon 8060S, that legacy key was absent: AMD registered its driver through device-specificVulkanDriverNamevalues instead. The driver-presence check therefore returned false, and Automatic continued to select ROCm.Change
VulkanDriverNamefrom present display adapters and SoftwareComponents before the legacy key, supporting bothREG_SZandREG_MULTI_SZ.Existing library validation, 64-bit filtering, environment overrides, and backend-selection guards remain in effect.
A/B
Live Windows run on Ryzen AI MAX+ 395 / Radeon 8060S with Adrenalin 32.0.22018.5. Both sides used llama.cpp release
b10840-mix-d5c17a0and suppliedgfx1151through the existing remembered-architecture mechanism because the runner had no HIP SDK.windows-rocmwindows-vulkanBoth backends remained available. The fix found the valid adapter manifest and rejected a stale SoftwareComponent registration whose manifest was missing.
Verification
test_install_resolve_prebuilt.py,test_llama_backend_selection.py, andtest_llama_backend_switch.py: 399 passed, 1 skipped.git diff --checkpassed. Subsequent comment-only cleanup preserved the executable AST.Scope
This repairs detection for automatic backend selection. Existing ROCm installations can re-apply Automatic through the existing migration offer.
SoftwareComponents are filtered by presence without checking their parent-adapter association, so discovery is broader than the Vulkan loader's traversal. Relative registration paths remain unsupported.
The live test covered discovery and backend resolution. No model inference or benchmark was run.