From: Christophe Date: Tue, 3 Jan 2023 15:37:15 +0000 (+0100) Subject: Warn user when using device layers field #1086 X-Git-Tag: upstream/1.3.268~225 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=02ac9a774693e61408a662e496f70c574eb9fe41;p=platform%2Fupstream%2FVulkan-Loader.git Warn user when using device layers field #1086 The device layer fields in VkDeviceCreateInfo are deprecated and have been for years. Users should not be adding anything to them (and if they do, it should match the layers added in VkInstanceCreateInfo). This would be a warning that 'device layers are deprecated, they do nothing' in case a user is trying to enable validation layers using them. Charles has seen multiple users not knowing that they are deprecated and get burned by the API due to not having validation layers enabled. Since there may be applications in the wild which currently do provide a list of layers to device create info, this warning should only be issued if the list differs from instance creation. That way no spurious warnings are created. --- diff --git a/loader/loader.c b/loader/loader.c index c98d37cf..37ae1805 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -4710,6 +4710,20 @@ VkResult loader_create_instance_chain(const VkInstanceCreateInfo *pCreateInfo, c memcpy(&loader_create_info, pCreateInfo, sizeof(VkInstanceCreateInfo)); + if (pCreateInfo->enabledLayerCount > 0 && pCreateInfo->ppEnabledLayerNames != NULL) { + inst->enabled_layer_count = pCreateInfo->enabledLayerCount; + + inst->enabled_layer_names = (char **)loader_instance_heap_alloc(inst, sizeof(char *) * pCreateInfo->enabledLayerCount, + VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); + + for (uint32_t i = 0, n = inst->enabled_layer_count; i < n; ++i) { + size_t size = strlen(pCreateInfo->ppEnabledLayerNames[i]) + 1; + inst->enabled_layer_names[i] = (char *)loader_instance_heap_alloc(inst, sizeof(char) * size, VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); + memset(inst->enabled_layer_names[i], '\0', sizeof(char) * size); + strcpy(inst->enabled_layer_names[i], pCreateInfo->ppEnabledLayerNames[i]); + } + } + if (inst->expanded_activated_layer_list.count > 0) { chain_info.u.pLayerInfo = NULL; chain_info.pNext = pCreateInfo->pNext; @@ -5004,6 +5018,34 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre memcpy(&loader_create_info, pCreateInfo, sizeof(VkDeviceCreateInfo)); + if (loader_create_info.enabledLayerCount > 0 && loader_create_info.ppEnabledLayerNames != NULL) { + bool invalid_device_layer_usage = false; + + if (loader_create_info.enabledLayerCount != inst->enabled_layer_count && loader_create_info.enabledLayerCount > 0) { + invalid_device_layer_usage = true; + } else if (loader_create_info.enabledLayerCount > 0 && loader_create_info.ppEnabledLayerNames == NULL) { + invalid_device_layer_usage = true; + } else if (loader_create_info.enabledLayerCount == 0 && loader_create_info.ppEnabledLayerNames != NULL) { + invalid_device_layer_usage = true; + } else if (inst->enabled_layer_names != NULL) { + for (uint32_t i = 0; i < loader_create_info.enabledLayerCount; i++) { + const char *device_layer_names = loader_create_info.ppEnabledLayerNames[i]; + + if (strcmp(device_layer_names, inst->enabled_layer_names[i]) != 0) { + invalid_device_layer_usage = true; + break; + } + } + } + + if (invalid_device_layer_usage) { + loader_log( + inst, VULKAN_LOADER_WARN_BIT, 0, + "loader_create_device_chain: Using deprecated and ignored 'ppEnabledLayerNames' member of 'VkDeviceCreateInfo' " + "when creating a Vulkan device."); + } + } + // Before we continue, we need to find out if the KHR_device_group extension is in the enabled list. If it is, we then // need to look for the corresponding VkDeviceGroupDeviceCreateInfoKHR struct in the device list. This is because we // need to replace all the incoming physical device values (which are really loader trampoline physical device values) @@ -5809,6 +5851,15 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const } loader_free_dev_ext_table(ptr_instance); loader_free_phys_dev_ext_table(ptr_instance); + + for (uint32_t i = 0, n = ptr_instance->enabled_layer_count; i < n; ++i) { + loader_instance_heap_free(ptr_instance, ptr_instance->enabled_layer_names[i]); + } + + if (ptr_instance->enabled_layer_count > 0) { + loader_instance_heap_free(ptr_instance, ptr_instance->enabled_layer_names); + memset(&ptr_instance->enabled_layer_names, 0, sizeof(ptr_instance->enabled_layer_names)); + } } VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physicalDevice, const VkDeviceCreateInfo *pCreateInfo, diff --git a/loader/loader_common.h b/loader/loader_common.h index 5e5adb22..d1b770fb 100644 --- a/loader/loader_common.h +++ b/loader/loader_common.h @@ -266,6 +266,9 @@ struct loader_instance { struct loader_msg_callback_map_entry *icd_msg_callback_map; + uint32_t enabled_layer_count; + char **enabled_layer_names; + struct loader_layer_list instance_layer_list; bool override_layer_present; diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp index 2e8fd74f..11e64f7a 100644 --- a/tests/framework/test_environment.cpp +++ b/tests/framework/test_environment.cpp @@ -194,6 +194,11 @@ void InstWrapper::CheckCreate(VkResult result_to_check) { ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst)); } +void InstWrapper::CheckCreateWithInfo(InstanceCreateInfo& create_info, VkResult result_to_check) { + ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst)); +} + + std::vector InstWrapper::GetPhysDevs(uint32_t phys_dev_count, VkResult result_to_check) { uint32_t physical_count = phys_dev_count; std::vector physical_devices; diff --git a/tests/framework/test_environment.h b/tests/framework/test_environment.h index 78577e31..6043250d 100644 --- a/tests/framework/test_environment.h +++ b/tests/framework/test_environment.h @@ -272,6 +272,7 @@ struct InstWrapper { // Construct this VkInstance using googletest to assert if it succeeded void CheckCreate(VkResult result_to_check = VK_SUCCESS); + void CheckCreateWithInfo(InstanceCreateInfo& create_info, VkResult result_to_check = VK_SUCCESS); // Convenience operator VkInstance() { return inst; } diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp index ed0cffb6..0f1d436f 100644 --- a/tests/loader_regression_tests.cpp +++ b/tests/loader_regression_tests.cpp @@ -969,6 +969,95 @@ TEST(CreateDevice, LayersNotPresent) { dev.CheckCreate(phys_dev); } +// Device layers are deprecated. +// Ensure that no error occur if instance and device are created with the same list of layers. +// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation +TEST(CreateDevice, MatchInstanceAndDeviceLayers) { + FrameworkEnvironment env{}; + env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)); + env.get_test_icd().physical_devices.emplace_back("physical_device_0"); + + const char* layer_name = "TestLayer"; + env.add_explicit_layer( + ManifestLayer{}.add_layer( + ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)), + "test_layer.json"); + + InstWrapper inst{env.vulkan_functions}; + inst.create_info.add_layer(layer_name); + inst.CheckCreate(); + + VkPhysicalDevice phys_dev = inst.GetPhysDev(); + + DeviceWrapper dev{inst}; + dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f)); + + dev.CheckCreate(phys_dev); +} + +// Device layers are deprecated. +// Ensure that a message is generated when instance and device are created with different list of layers. +// At best , the user can list only instance layers in the device layer list +// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation +TEST(CreateDevice, UnmatchInstanceAndDeviceLayers) { + FrameworkEnvironment env{}; + env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)); + env.get_test_icd().physical_devices.emplace_back("physical_device_0"); + + const char* layer_name = "TestLayer"; + env.add_explicit_layer( + ManifestLayer{}.add_layer( + ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)), + "test_layer.json"); + + DebugUtilsLogger debug_log{VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT}; + InstWrapper inst{env.vulkan_functions}; + FillDebugUtilsCreateDetails(inst.create_info, debug_log); + inst.CheckCreate(); + + DebugUtilsWrapper log{inst, VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT}; + CreateDebugUtilsMessenger(log); + + VkPhysicalDevice phys_dev = inst.GetPhysDev(); + + DeviceWrapper dev{inst}; + dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f)); + + dev.CheckCreate(phys_dev); + + ASSERT_TRUE(log.find("loader_create_device_chain: Using deprecated and ignored 'ppEnabledLayerNames' member of 'VkDeviceCreateInfo' when creating a Vulkan device.")); +} + +// Device layers are deprecated. +// Ensure that when VkInstanceCreateInfo is deleted, the check of the instance layer lists is running correctly during VkDevice creation +// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation +TEST(CreateDevice, CheckCopyOfInstanceLayerNames) { + FrameworkEnvironment env{}; + env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)); + env.get_test_icd().physical_devices.emplace_back("physical_device_0"); + + const char* layer_name = "TestLayer"; + env.add_explicit_layer( + ManifestLayer{}.add_layer( + ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)), + "test_layer.json"); + + InstWrapper inst{env.vulkan_functions}; + { + // We intentionally create a local InstanceCreateInfo that goes out of scope at the } so that when dev.CheckCreate is called the layer name pointers are no longer valid + InstanceCreateInfo create_info{}; + create_info.add_layer(layer_name); + inst.CheckCreateWithInfo(create_info); + } + + VkPhysicalDevice phys_dev = inst.GetPhysDev(); + + DeviceWrapper dev{inst}; + dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f)); + + dev.CheckCreate(phys_dev); +} + TEST(CreateDevice, ConsecutiveCreate) { FrameworkEnvironment env{}; env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2));