Sort PhysicalDevices to match ICD's ordering #1744
Conversation
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
|
|
Thank you. Works fine under Windows. with current Vulkan loader |
|
@peppsac I just merged a big PR which refactored the test framework. Not too many tests were changed, but some of the details of tests did. The refactor makes it easier to ensure the 'correct' order is maintained (ie, some tests used the wrong order, or had conditions that shouldn't have happened). Anyhow, I want to write to state that while I am very grateful for the PR, I am not 100% it will be accepted due to risk of unintended consequences. The order of physical devices is not defined by the spec, and while the docs in the loader do say something, if the source code does something else and the ecosystem relies on that behavior, then the docs are what need amending (that is more hypothetical than reality). I have gone ahead and put this PR as a item for the Vulkan System Integration TSG agenda, will see if there is broad concern that this PR is "rocking the boat" and shouldn't be accepted. I would like to think that the ecosystem could handle the order of physical devices changing, especially since the linux sorting logic is present, there is sorting logic on windows to follow the OS preference (that obviously takes priority), macOS has 1 driver, and android doesn't even use this loader implementation (ontop of being 1 driver per device usually). It would be a shame if the implementation details couldn't be improved just because of an app that happens to pick the 'wrong' device because the order changed, instead of apps gracefully picking the right physical device. And for many, this change will simply not affect them as they dont have more than 1 physical device in which to reorder. Still, due diligence should be taken when the implementation details can affect the apparent behavior of foundational software. |
| while (prev->next) | ||
| prev = prev->next; | ||
| prev->next = icd_term; | ||
| } |
There was a problem hiding this comment.
It would be nice to track the list tail to avoid iterating the entire list here, but probably a moot point with realistic numbers of ICDs, so not a big deal. This way is certainly less complex.
Thanks!
Where is this logic implemented for Windows? IMO this change is basically "follow the OS preference" for Linux. Distributions (I've checked Debian/Ubuntu and Fedora) that chose to install their Vulkan drivers icd files in |
|
@peppsac https://github.com/KhronosGroup/Vulkan-Loader/blob/main/docs/LoaderDriverInterface.md#physical-device-sorting covers it in brief. Mainly, the loader is not controlling sorting, it is done by the OS + drivers by having the loader queries adapters in the order the OS prefers. I will have to mention that to be able to accept this PR, the CLA must be signed and all checks must pass. The formatting check uses clang-format-16 and should be able to run clang-format on changed files to get the correct output. The failing windows checks are because the EnumerateAdapterPhysicalDevices.SameAdapterLUID_reordered test doesn't pass. Not too surprising since that test also assumes reverse insertion order. Be mindful that the test also has 'layered' implementations, which need to be put after 'real' physical devices. AKA this test is to make sure the Dozen driver doesn't get put before the other physical device that uses the same GPU. |
peppsac
force-pushed
the
physical_devices_sorted
branch
from
1e640c9 to
d129f08
Compare
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
Thanks, will look into it.
I've run git-clang-format and hopefully fixed the Windows-only test as well. |
The loop will only be entered whe .size() returns 1. For all the other case, it'll be skipped as the i == 0 condition is false.
charles-lunarg
force-pushed
the
physical_devices_sorted
branch
from
d129f08 to
2eab8b5
Compare
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Apologies for the delay, I've rebased and fixed a macos bundle test :) I initially was very hesitant to accept this as it has the potential to 'break people'. But on review, the current situation breaks intuition because its backwards to the 'found first, used first' idea which is a lot of how the loader functions. |
|
CI Vulkan-Loader build queued with queue ID 644306. |
|
CI Vulkan-Loader build # 3407 running. |
|
CI Vulkan-Loader build # 3407 failed. |
This commit switch from prepending newly loaded ICDs to the ICD list, to appending them. Both options are fine since the order VkPhysicalDevices appear is unspecified, but by using an append operation the VkPhysicalDevices are returned in the same order as the drivers are loaded. See KhronosGroup#1725
charles-lunarg
force-pushed
the
physical_devices_sorted
branch
from
2eab8b5 to
7db9142
Compare
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build. |
|
CI Vulkan-Loader build queued with queue ID 644559. |
|
CI Vulkan-Loader build # 3411 running. |
|
CI Vulkan-Loader build # 3411 passed. |
charles-lunarg
left a comment
charles-lunarg
left a comment
There was a problem hiding this comment.
This isn't a really controversial change - because the previous behavior is frankly obnoxious and this solves some real 'why....'.
I'm sure there are unknown side effects of the change, and may break someone. But I posit that those use cases were brittle and going to break anyway due to other reasons (so it was bound to happen.) The benefits outweigh the consequences in other words. Also thank you for going through and fixing up the tests. I've done the last little bit of rebasing on main and fixing 2 additional tests.
6 participants
The first commit is a bug fixes for tests.
The second commit changes the order of PhysicalDevices to match the order of ICDs and updates the tests that depend on the ordering (I could only test on Linux).
This fixes #1725.