From 9c934e9d9e35af5346d1c0398cdae4cb34615594 Mon Sep 17 00:00:00 2001 From: Pierre-Eric Pelloux-Prayer Date: Wed, 11 Jun 2025 14:15:29 +0200 Subject: [PATCH 1/2] Fix backward loop condition in tests 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. --- tests/loader_version_tests.cpp | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/tests/loader_version_tests.cpp b/tests/loader_version_tests.cpp index f57833934..28bae8999 100644 --- a/tests/loader_version_tests.cpp +++ b/tests/loader_version_tests.cpp @@ -994,20 +994,24 @@ void CheckDirectDriverLoading(FrameworkEnvironment& env, std::vector // We have to iterate through the driver lists backwards because the loader *prepends* icd's, so the last found ICD is found // first in the driver list uint32_t driver_index = 0; - for (size_t i = normal_drivers.size() - 1; i == 0; i--) { - if (normal_drivers.at(i).expect_to_find) { - VkPhysicalDeviceProperties props{}; - inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); - ASSERT_EQ(props.driverVersion, normal_drivers.at(i).driver_version); - driver_index++; + if (!normal_drivers.empty()) { + for (int i = normal_drivers.size() - 1; i >= 0; i--) { + if (!exclusive && normal_drivers.at(i).expect_to_find) { + VkPhysicalDeviceProperties props{}; + inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); + ASSERT_EQ(props.driverVersion, normal_drivers.at(i).driver_version); + driver_index++; + } } } - for (size_t i = direct_drivers.size() - 1; i == 0; i--) { - if (direct_drivers.at(i).expect_to_find) { - VkPhysicalDeviceProperties props{}; - inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); - ASSERT_EQ(props.driverVersion, direct_drivers.at(i).driver_version); - driver_index++; + if (!direct_drivers.empty()) { + for (int i = direct_drivers.size() - 1; i >= 0; i--) { + if (direct_drivers.at(i).expect_to_find) { + VkPhysicalDeviceProperties props{}; + inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); + ASSERT_EQ(props.driverVersion, direct_drivers.at(i).driver_version); + driver_index++; + } } } } From 7db9142c77015704991f04233df04daffa18ce60 Mon Sep 17 00:00:00 2001 From: Pierre-Eric Pelloux-Prayer Date: Wed, 11 Jun 2025 14:11:10 +0200 Subject: [PATCH 2/2] Store ICDs in the loading order 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 https://github.com/KhronosGroup/Vulkan-Loader/issues/1725 --- loader/loader.c | 11 +++-- tests/loader_get_proc_addr_tests.cpp | 2 +- tests/loader_regression_tests.cpp | 67 ++++++++++++++-------------- tests/loader_settings_tests.cpp | 4 +- tests/loader_version_tests.cpp | 18 ++++---- tests/loader_wsi_tests.cpp | 12 ++--- 6 files changed, 59 insertions(+), 55 deletions(-) diff --git a/loader/loader.c b/loader/loader.c index f8b01b6df..ff637e7ab 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1768,9 +1768,14 @@ struct loader_icd_term *loader_icd_add(struct loader_instance *ptr_inst, const s icd_term->scanned_icd = scanned_icd; icd_term->this_instance = ptr_inst; - // Prepend to the list - icd_term->next = ptr_inst->icd_terms; - ptr_inst->icd_terms = icd_term; + // Append to the list + struct loader_icd_term *prev = ptr_inst->icd_terms; + if (prev == NULL) { + ptr_inst->icd_terms = icd_term; + } else { + while (prev->next) prev = prev->next; + prev->next = icd_term; + } ptr_inst->icd_terms_count++; return icd_term; diff --git a/tests/loader_get_proc_addr_tests.cpp b/tests/loader_get_proc_addr_tests.cpp index 89fa51ac9..8a89e8159 100644 --- a/tests/loader_get_proc_addr_tests.cpp +++ b/tests/loader_get_proc_addr_tests.cpp @@ -188,8 +188,8 @@ TEST(GetProcAddr, Verify10FunctionsFailToLoadWithSingleDriver) { TEST(GetProcAddr, Verify10FunctionsLoadWithMultipleDrivers) { FrameworkEnvironment env{}; - env.add_icd(TEST_ICD_PATH_VERSION_2).add_physical_device({}); env.add_icd(TEST_ICD_PATH_VERSION_2).add_physical_device({}).set_can_query_GetPhysicalDeviceFuncs(false); + env.add_icd(TEST_ICD_PATH_VERSION_2).add_physical_device({}); InstWrapper inst{env.vulkan_functions}; inst.CheckCreate(); diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp index a76960f34..fdb309e6c 100644 --- a/tests/loader_regression_tests.cpp +++ b/tests/loader_regression_tests.cpp @@ -3658,31 +3658,31 @@ TEST(SortedPhysicalDevices, DevicesSortedDisabled) { ASSERT_EQ(VK_SUCCESS, instance->vkEnumeratePhysicalDevices(instance, &device_count, physical_devices.data())); ASSERT_EQ(device_count, max_phys_devs); - // make sure the order is what we started with - but its a bit wonky due to the loader reading physical devices "backwards" + // make sure the order is what we started with VkPhysicalDeviceProperties props{}; instance->vkGetPhysicalDeviceProperties(physical_devices[0], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU); - ASSERT_STREQ(props.deviceName, "pd5"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); + ASSERT_STREQ(props.deviceName, "pd0"); instance->vkGetPhysicalDeviceProperties(physical_devices[1], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd3"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU); + ASSERT_STREQ(props.deviceName, "pd1"); instance->vkGetPhysicalDeviceProperties(physical_devices[2], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd4"); - - instance->vkGetPhysicalDeviceProperties(physical_devices[3], &props); ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_CPU); ASSERT_STREQ(props.deviceName, "pd2"); + instance->vkGetPhysicalDeviceProperties(physical_devices[3], &props); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); + ASSERT_STREQ(props.deviceName, "pd3"); + instance->vkGetPhysicalDeviceProperties(physical_devices[4], &props); ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd0"); + ASSERT_STREQ(props.deviceName, "pd4"); instance->vkGetPhysicalDeviceProperties(physical_devices[5], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU); - ASSERT_STREQ(props.deviceName, "pd1"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU); + ASSERT_STREQ(props.deviceName, "pd5"); // Make sure if we call enumerate again, the information is the same std::array physical_devices_again; @@ -3974,39 +3974,40 @@ TEST(SortedPhysicalDevices, DeviceGroupsSortedDisabled) { ASSERT_EQ(VK_SUCCESS, inst->vkEnumeratePhysicalDeviceGroups(inst, &group_count, physical_device_groups.data())); ASSERT_EQ(group_count, max_phys_dev_groups); - // make sure the order is what we started with - but its a bit wonky due to the loader reading physical devices "backwards" + // make sure the order is what we started with VkPhysicalDeviceProperties props{}; + inst->vkGetPhysicalDeviceProperties(physical_devices[0], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU); - ASSERT_STREQ(props.deviceName, "pd7"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); + ASSERT_STREQ(props.deviceName, "pd0"); inst->vkGetPhysicalDeviceProperties(physical_devices[1], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd4"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU); + ASSERT_STREQ(props.deviceName, "pd1"); inst->vkGetPhysicalDeviceProperties(physical_devices[2], &props); ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd5"); + ASSERT_STREQ(props.deviceName, "pd2"); inst->vkGetPhysicalDeviceProperties(physical_devices[3], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd6"); - - inst->vkGetPhysicalDeviceProperties(physical_devices[4], &props); ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_CPU); ASSERT_STREQ(props.deviceName, "pd3"); + inst->vkGetPhysicalDeviceProperties(physical_devices[4], &props); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); + ASSERT_STREQ(props.deviceName, "pd4"); + inst->vkGetPhysicalDeviceProperties(physical_devices[5], &props); ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd0"); + ASSERT_STREQ(props.deviceName, "pd5"); inst->vkGetPhysicalDeviceProperties(physical_devices[6], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU); - ASSERT_STREQ(props.deviceName, "pd1"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); + ASSERT_STREQ(props.deviceName, "pd6"); inst->vkGetPhysicalDeviceProperties(physical_devices[7], &props); - ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU); - ASSERT_STREQ(props.deviceName, "pd2"); + ASSERT_EQ(props.deviceType, VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU); + ASSERT_STREQ(props.deviceName, "pd7"); // Make sure if we call enumerate again, the information is the same std::array physical_device_groups_again{}; @@ -4446,9 +4447,9 @@ TEST(ManifestDiscovery, AppleBundles) { // Should get both GPUs, in reverse order to driver enumeration (due to enumerating the last driver first) VkPhysicalDeviceProperties props{}; inst->vkGetPhysicalDeviceProperties(physical_devices[0], &props); - ASSERT_EQ(test_physical_device_1.properties.deviceID, props.deviceID); - inst->vkGetPhysicalDeviceProperties(physical_devices[1], &props); ASSERT_EQ(test_physical_device_0.properties.deviceID, props.deviceID); + inst->vkGetPhysicalDeviceProperties(physical_devices[1], &props); + ASSERT_EQ(test_physical_device_1.properties.deviceID, props.deviceID); } // Add two drivers, one to the bundle and one using the driver env-var @@ -4717,10 +4718,10 @@ TEST(EnumerateAdapterPhysicalDevices, SameAdapterLUID_reordered) { // Physical devices are enumerated: // a) first in the order of LUIDs showing up in DXGIAdapter list - // b) then in the reverse order to the drivers insertion into the test framework - add_dxgi_adapter(env, "physical_device_2", LUID{10, 100}, 2); - add_dxgi_adapter(env, "physical_device_1", LUID{20, 200}, 1); + // b) then in the order to the drivers insertion into the test framework auto phys_dev_handle = add_dxgi_adapter(env, "physical_device_0", LUID{10, 100}, 2); + add_dxgi_adapter(env, "physical_device_1", LUID{20, 200}, 1); + add_dxgi_adapter(env, "physical_device_2", LUID{10, 100}, 2); { uint32_t returned_physical_count = 0; @@ -4764,7 +4765,7 @@ TEST(EnumerateAdapterPhysicalDevices, SameAdapterLUID_reordered) { } // Set the first physical device that is enumerated to be a 'layered' driver so it should be swapped with the first physical // device - env.get_test_icd(2).physical_devices.at(phys_dev_handle).layered_driver_underlying_api = + env.get_test_icd(0).physical_devices.at(phys_dev_handle).layered_driver_underlying_api = VK_LAYERED_DRIVER_UNDERLYING_API_D3D12_MSFT; { uint32_t returned_physical_count = 0; diff --git a/tests/loader_settings_tests.cpp b/tests/loader_settings_tests.cpp index 1c7f5ef67..f3abe67dc 100644 --- a/tests/loader_settings_tests.cpp +++ b/tests/loader_settings_tests.cpp @@ -3069,8 +3069,8 @@ TEST(SettingsFile, AdditionalDrivers) { VkPhysicalDeviceProperties props1{}, props2{}; inst.functions->vkGetPhysicalDeviceProperties(pds.at(0), &props1); inst.functions->vkGetPhysicalDeviceProperties(pds.at(1), &props2); - ASSERT_TRUE(string_eq(props1.deviceName, regular_driver_name)); - ASSERT_TRUE(string_eq(props2.deviceName, settings_driver_name)); + ASSERT_TRUE(string_eq(props1.deviceName, settings_driver_name)); + ASSERT_TRUE(string_eq(props2.deviceName, regular_driver_name)); } // settings file provided drivers replacing system found drivers TEST(SettingsFile, ExclusiveAdditionalDrivers) { diff --git a/tests/loader_version_tests.cpp b/tests/loader_version_tests.cpp index 28bae8999..56d5c44bf 100644 --- a/tests/loader_version_tests.cpp +++ b/tests/loader_version_tests.cpp @@ -991,25 +991,23 @@ void CheckDirectDriverLoading(FrameworkEnvironment& env, std::vector auto phys_devs = inst.GetPhysDevs(); ASSERT_EQ(phys_devs.size(), expected_driver_count); - // We have to iterate through the driver lists backwards because the loader *prepends* icd's, so the last found ICD is found - // first in the driver list uint32_t driver_index = 0; - if (!normal_drivers.empty()) { - for (int i = normal_drivers.size() - 1; i >= 0; i--) { - if (!exclusive && normal_drivers.at(i).expect_to_find) { + if (!direct_drivers.empty()) { + for (size_t i = 0; i < direct_drivers.size(); i++) { + if (direct_drivers.at(i).expect_to_find) { VkPhysicalDeviceProperties props{}; inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); - ASSERT_EQ(props.driverVersion, normal_drivers.at(i).driver_version); + ASSERT_EQ(props.driverVersion, direct_drivers.at(i).driver_version); driver_index++; } } } - if (!direct_drivers.empty()) { - for (int i = direct_drivers.size() - 1; i >= 0; i--) { - if (direct_drivers.at(i).expect_to_find) { + if (!normal_drivers.empty()) { + for (size_t i = 0; i < normal_drivers.size(); i++) { + if (!exclusive && normal_drivers.at(i).expect_to_find) { VkPhysicalDeviceProperties props{}; inst.functions->vkGetPhysicalDeviceProperties(phys_devs.at(driver_index), &props); - ASSERT_EQ(props.driverVersion, direct_drivers.at(i).driver_version); + ASSERT_EQ(props.driverVersion, normal_drivers.at(i).driver_version); driver_index++; } } diff --git a/tests/loader_wsi_tests.cpp b/tests/loader_wsi_tests.cpp index c77ffbadc..bb1d1e778 100644 --- a/tests/loader_wsi_tests.cpp +++ b/tests/loader_wsi_tests.cpp @@ -1067,9 +1067,9 @@ TEST(WsiTests, MultiPlatformGetPhysicalDeviceSurfaceSupportKHR) { inst.CheckCreate(); auto phys_devs = inst.GetPhysDevs(); - // Physical devices are enumerated in reverse order to the ICD order - VkPhysicalDevice xcb_physical_device = phys_devs[1]; - VkPhysicalDevice wayland_physical_device = phys_devs[0]; + // Physical devices are enumerated in order to the ICD order + VkPhysicalDevice xcb_physical_device = phys_devs[0]; + VkPhysicalDevice wayland_physical_device = phys_devs[1]; VkPhysicalDeviceProperties props0{}; inst->vkGetPhysicalDeviceProperties(wayland_physical_device, &props0); ASSERT_TRUE(string_eq(props0.deviceName, wayland_device_name)); @@ -1113,9 +1113,9 @@ TEST(WsiTests, MultiPlatformGetPhysicalDeviceSurfaceSupportKHR) { inst.CheckCreate(); auto phys_devs = inst.GetPhysDevs(); - // Physical devices are enumerated in reverse order to the ICD order - VkPhysicalDevice xcb_physical_device = phys_devs[1]; - VkPhysicalDevice wayland_physical_device = phys_devs[0]; + // Physical devices are enumerated in ICD discovery order + VkPhysicalDevice xcb_physical_device = phys_devs[0]; + VkPhysicalDevice wayland_physical_device = phys_devs[1]; VkPhysicalDeviceProperties props0{}; inst->vkGetPhysicalDeviceProperties(wayland_physical_device, &props0); ASSERT_TRUE(string_eq(props0.deviceName, wayland_device_name));