Sort PhysicalDevices to match ICD's ordering

[peppsac]

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 https://github.com/KhronosGroup/Vulkan-Loader/issues/1725.

[ci-tester-lunarg]

Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build.

[CLAassistant]

All committers have signed the CLA.

[StefanG3D]

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.

Thank you. Works fine under Windows.

with current Vulkan loader https://gpuz.techpowerup.com/25/07/28/q6y.png with your pull request https://gpuz.techpowerup.com/25/07/28/u6v.png

[charles-lunarg]

@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.

[peppsac]

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.

Thanks!

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),

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 /usr/share/vulkan/icd.d are probably expecting that drivers installed in locations looked up first are going to show up first in the physical devices list as well.

[charles-lunarg]

@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.

[ci-tester-lunarg]

Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author peppsac not on autobuild list. Waiting for curator authorization before starting CI build.

[peppsac]

@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.

Thanks, will look into it.

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.

I've run git-clang-format and hopefully fixed the Windows-only test as well.