From e7bf86e164e88409324fa2c8b41e0775f9d48501 Mon Sep 17 00:00:00 2001 From: Mark Young Date: Mon, 7 Nov 2016 13:27:02 -0700 Subject: [PATCH] loader: gh1120/gh1134 - Object wrapping issues First issue was that we needed to override vkGetDeviceProcAddr. Instead of allowing this to always go directly to the ICD, we needed to intercept a few commands because they require a loader trampoline and terminator call. Most commands still return a pointer directly to ICD command. GH1120 - Unwrap both the physical device handles and the KHR_surface handles in the loader during both the trampoline and terminator calls for DebugMarker commands. This has to be done since the values given to an application are the loader trampoline versions, and the values given to the last layer is the loader terminator versions. GH1134 - We were passing down the wrong device object to the ICD functions when querying the ICD command function address and comparing it in the override functions. Thanks to Baldur (Mr. Renderdoc) for discovering this, testing my fixes, and resolving several bugs. Change-Id: I7618d71ffee6c53d9842758210a9261f6b3a1797 --- loader/extensions.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++ loader/extensions.h | 9 +++++ loader/loader.c | 63 +++++++++++++++---------------- loader/loader.h | 9 +++-- loader/table_ops.h | 20 +++++++--- loader/trampoline.c | 11 ++---- 6 files changed, 170 insertions(+), 49 deletions(-) diff --git a/loader/extensions.c b/loader/extensions.c index 9b59eb5..643911d 100644 --- a/loader/extensions.c +++ b/loader/extensions.c @@ -26,6 +26,100 @@ #include "loader.h" #include "extensions.h" #include +#include "wsi.h" + +// Definitions for the VK_EXT_debug_marker extension commands which +// need to have a terminator function + +VKAPI_ATTR VkResult VKAPI_CALL vkDebugMarkerSetObjectTagEXT( + VkDevice device, VkDebugMarkerObjectTagInfoEXT *pTagInfo) { + const VkLayerDispatchTable *disp = loader_get_dispatch(device); + // If this is a physical device, we have to replace it with the proper one + // for the next call. + if (pTagInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT) { + struct loader_physical_device_tramp *phys_dev_tramp = + (struct loader_physical_device_tramp *)pTagInfo->object; + pTagInfo->object = (uint64_t)phys_dev_tramp->phys_dev; + } + return disp->DebugMarkerSetObjectTagEXT(device, pTagInfo); +} + +VKAPI_ATTR VkResult VKAPI_CALL terminator_DebugMarkerSetObjectTagEXT( + VkDevice device, VkDebugMarkerObjectTagInfoEXT *pTagInfo) { + uint32_t icd_index = 0; + struct loader_device *dev; + struct loader_icd_term *icd_term = + loader_get_icd_and_device(device, &dev, &icd_index); + // If this is a physical device, we have to replace it with the proper one + // for the next call. + if (pTagInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT) { + struct loader_physical_device_term *phys_dev_term = + (struct loader_physical_device_term *)pTagInfo->object; + pTagInfo->object = (uint64_t)phys_dev_term->phys_dev; + + // If this is a KHR_surface, and the ICD has created its own, we have to + // replace it with the proper one for the next call. + } else if (pTagInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_SURFACE_KHR_EXT) { + if (NULL != icd_term && NULL != icd_term->CreateSwapchainKHR) { + VkIcdSurface *icd_surface = (VkIcdSurface *)(pTagInfo->object); + if (NULL != icd_surface->real_icd_surfaces) { + pTagInfo->object = + (uint64_t)icd_surface->real_icd_surfaces[icd_index]; + } + } + } + assert(icd_term != NULL && icd_term->DebugMarkerSetObjectTagEXT && + "loader: null DebugMarkerSetObjectTagEXT ICD pointer"); + return icd_term->DebugMarkerSetObjectTagEXT(device, pTagInfo); +} + +VKAPI_ATTR VkResult VKAPI_CALL vkDebugMarkerSetObjectNameEXT( + VkDevice device, VkDebugMarkerObjectNameInfoEXT *pNameInfo) { + const VkLayerDispatchTable *disp = loader_get_dispatch(device); + // If this is a physical device, we have to replace it with the proper one + // for the next call. + if (pNameInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT) { + struct loader_physical_device_tramp *phys_dev_tramp = + (struct loader_physical_device_tramp *)pNameInfo->object; + pNameInfo->object = (uint64_t)phys_dev_tramp->phys_dev; + } + return disp->DebugMarkerSetObjectNameEXT(device, pNameInfo); +} + +VKAPI_ATTR VkResult VKAPI_CALL terminator_DebugMarkerSetObjectNameEXT( + VkDevice device, VkDebugMarkerObjectNameInfoEXT *pNameInfo) { + uint32_t icd_index = 0; + struct loader_device *dev; + struct loader_icd_term *icd_term = + loader_get_icd_and_device(device, &dev, &icd_index); + // If this is a physical device, we have to replace it with the proper one + // for the next call. + if (pNameInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT) { + struct loader_physical_device_term *phys_dev_term = + (struct loader_physical_device_term *)pNameInfo->object; + pNameInfo->object = (uint64_t)phys_dev_term->phys_dev; + + // If this is a KHR_surface, and the ICD has created its own, we have to + // replace it with the proper one for the next call. + } else if (pNameInfo->objectType == + VK_DEBUG_REPORT_OBJECT_TYPE_SURFACE_KHR_EXT) { + if (NULL != icd_term && NULL != icd_term->CreateSwapchainKHR) { + VkIcdSurface *icd_surface = (VkIcdSurface *)(pNameInfo->object); + if (NULL != icd_surface->real_icd_surfaces) { + pNameInfo->object = + (uint64_t)icd_surface->real_icd_surfaces[icd_index]; + } + } + } + assert(icd_term != NULL && icd_term->DebugMarkerSetObjectNameEXT && + "loader: null DebugMarkerSetObjectNameEXT ICD pointer"); + return icd_term->DebugMarkerSetObjectNameEXT(device, pNameInfo); +} // Definitions for the VK_NV_external_memory_capabilities extension @@ -118,6 +212,19 @@ bool extension_instance_gpa(struct loader_instance *ptr_instance, const char *name, void **addr) { *addr = NULL; + // Definitions for the VK_EXT_debug_marker extension commands which + // need to have a terminator function. Since these are device + // commands, we always need to return a valid value for them. + + if (!strcmp("vkDebugMarkerSetObjectTagEXT", name)) { + *addr = (void *)vkDebugMarkerSetObjectTagEXT; + return true; + } + if (!strcmp("vkDebugMarkerSetObjectNameEXT", name)) { + *addr = (void *)vkDebugMarkerSetObjectNameEXT; + return true; + } + // Functions for the VK_NV_external_memory_capabilities extension if (!strcmp("vkGetPhysicalDeviceExternalImageFormatPropertiesNV", name)) { diff --git a/loader/extensions.h b/loader/extensions.h index 651d5f5..2e922e5 100644 --- a/loader/extensions.h +++ b/loader/extensions.h @@ -28,6 +28,15 @@ bool extension_instance_gpa(struct loader_instance *ptr_instance, void extensions_create_instance(struct loader_instance *ptr_instance, const VkInstanceCreateInfo *pCreateInfo); +// Definitions for the VK_EXT_debug_marker extension + + +VKAPI_ATTR VkResult VKAPI_CALL terminator_DebugMarkerSetObjectTagEXT( + VkDevice device, VkDebugMarkerObjectTagInfoEXT *pTagInfo); + +VKAPI_ATTR VkResult VKAPI_CALL terminator_DebugMarkerSetObjectNameEXT( + VkDevice device, VkDebugMarkerObjectNameInfoEXT *pNameInfo); + // Definitions for the VK_NV_external_memory_capabilities extension VKAPI_ATTR VkResult VKAPI_CALL diff --git a/loader/loader.c b/loader/loader.c index b433237..73281f9 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1262,9 +1262,10 @@ loader_get_icd_and_device(const VkDevice device, icd_term = icd_term->next) { for (struct loader_device *dev = icd_term->logical_device_list; dev; dev = dev->next) - /* Value comparison of device prevents object wrapping by layers - */ - if (loader_get_dispatch(dev->device) == + // Value comparison of device prevents object wrapping by layers + if (loader_get_dispatch(dev->icd_device) == + loader_get_dispatch(device) || + loader_get_dispatch(dev->chain_device) == loader_get_dispatch(device)) { *found_dev = dev; if (NULL != icd_index) { @@ -1644,6 +1645,8 @@ static bool loader_icd_init_entrys(struct loader_icd_term *icd_term, LOOKUP_GIPA(GetPhysicalDeviceSparseImageFormatProperties, true); LOOKUP_GIPA(CreateDebugReportCallbackEXT, false); LOOKUP_GIPA(DestroyDebugReportCallbackEXT, false); + LOOKUP_GIPA(DebugMarkerSetObjectTagEXT, false); + LOOKUP_GIPA(DebugMarkerSetObjectNameEXT, false); LOOKUP_GIPA(GetPhysicalDeviceSurfaceSupportKHR, false); LOOKUP_GIPA(GetPhysicalDeviceSurfaceCapabilitiesKHR, false); LOOKUP_GIPA(GetPhysicalDeviceSurfaceFormatsKHR, false); @@ -3268,36 +3271,28 @@ loader_gpa_instance_internal(VkInstance inst, const char *pName) { return NULL; } -void loader_override_terminating_device_proc( - VkDevice device, struct loader_dev_dispatch_table *disp_table) { - struct loader_device *dev; - struct loader_icd_term *icd_term = - loader_get_icd_and_device(device, &dev, NULL); - - // Certain device entry-points still need to go through a terminator before - // hitting the ICD. This could be for several reasons, but the main one - // is currently unwrapping an object before passing the appropriate info - // along to the ICD. - if ((PFN_vkVoidFunction)disp_table->core_dispatch.CreateSwapchainKHR == - (PFN_vkVoidFunction)icd_term->GetDeviceProcAddr( - device, "vkCreateSwapchainKHR")) { - disp_table->core_dispatch.CreateSwapchainKHR = - terminator_vkCreateSwapchainKHR; - } -} - VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL loader_gpa_device_internal(VkDevice device, const char *pName) { struct loader_device *dev; struct loader_icd_term *icd_term = loader_get_icd_and_device(device, &dev, NULL); - // Certain device entry-points still need to go through a terminator before - // hitting the ICD. This could be for several reasons, but the main one - // is currently unwrapping an object before passing the appropriate info - // along to the ICD. - if (!strcmp(pName, "vkCreateSwapchainKHR")) { + // NOTE: Device Funcs needing Trampoline/Terminator. + // Overrides for device functions needing a trampoline and + // a terminator because certain device entry-points still need to go + // through a terminator before hitting the ICD. This could be for + // several reasons, but the main one is currently unwrapping an + // object before passing the appropriate info along to the ICD. + // This is why we also have to override the direct ICD call to + // vkGetDeviceProcAddr to intercept those calls. + if (!strcmp(pName, "vkGetDeviceProcAddr")) { + return (PFN_vkVoidFunction)loader_gpa_device_internal; + } else if (!strcmp(pName, "vkCreateSwapchainKHR")) { return (PFN_vkVoidFunction)terminator_vkCreateSwapchainKHR; + } else if (!strcmp(pName, "vkDebugMarkerSetObjectTagEXT")) { + return (PFN_vkVoidFunction)terminator_DebugMarkerSetObjectTagEXT; + } else if (!strcmp(pName, "vkDebugMarkerSetObjectNameEXT")) { + return (PFN_vkVoidFunction)terminator_DebugMarkerSetObjectNameEXT; } return icd_term->GetDeviceProcAddr(device, pName); @@ -3321,7 +3316,7 @@ static void loader_init_dispatch_dev_ext_entry(struct loader_instance *inst, void *gdpa_value; if (dev != NULL) { gdpa_value = dev->loader_dispatch.core_dispatch.GetDeviceProcAddr( - dev->device, funcName); + dev->chain_device, funcName); if (gdpa_value != NULL) dev->loader_dispatch.ext_dispatch.dev_ext[idx] = (PFN_vkDevExt)gdpa_value; @@ -3332,7 +3327,7 @@ static void loader_init_dispatch_dev_ext_entry(struct loader_instance *inst, while (ldev) { gdpa_value = ldev->loader_dispatch.core_dispatch.GetDeviceProcAddr( - ldev->device, funcName); + ldev->chain_device, funcName); if (gdpa_value != NULL) ldev->loader_dispatch.ext_dispatch.dev_ext[idx] = (PFN_vkDevExt)gdpa_value; @@ -3982,15 +3977,15 @@ loader_create_device_chain(const struct loader_physical_device_tramp *pd, if (res != VK_SUCCESS) { return res; } - dev->device = created_device; + dev->chain_device = created_device; } else { // Couldn't find CreateDevice function! return VK_ERROR_INITIALIZATION_FAILED; } - /* Initialize device dispatch table */ + // Initialize device dispatch table loader_init_device_dispatch_table(&dev->loader_dispatch, nextGDPA, - dev->device); + dev->chain_device); return res; } @@ -4332,6 +4327,8 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice( PFN_vkCreateDevice fpCreateDevice = icd_term->CreateDevice; struct loader_extension_list icd_exts; + dev->phys_dev_term = phys_dev_term; + icd_exts.list = NULL; if (fpCreateDevice == NULL) { @@ -4398,7 +4395,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice( } res = fpCreateDevice(phys_dev_term->phys_dev, &localCreateInfo, pAllocator, - &dev->device); + &dev->icd_device); if (res != VK_SUCCESS) { loader_log(icd_term->this_instance, VK_DEBUG_REPORT_ERROR_BIT_EXT, 0, "vkCreateDevice call failed in ICD %s", @@ -4406,7 +4403,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice( goto out; } - *pDevice = dev->device; + *pDevice = dev->icd_device; loader_add_logical_device(icd_term->this_instance, icd_term, dev); /* Init dispatch pointer in new device object */ diff --git a/loader/loader.h b/loader/loader.h index daf663e..fc035ed 100644 --- a/loader/loader.h +++ b/loader/loader.h @@ -166,7 +166,9 @@ struct loader_dev_dispatch_table { // per CreateDevice structure struct loader_device { struct loader_dev_dispatch_table loader_dispatch; - VkDevice device; // device object from the icd + VkDevice chain_device; // device object from the dispatch chain + VkDevice icd_device; // device object from the icd + struct loader_physical_device_term *phys_dev_term; struct loader_layer_list activated_layer_list; @@ -200,6 +202,8 @@ struct loader_icd_term { PFN_vkCreateDebugReportCallbackEXT CreateDebugReportCallbackEXT; PFN_vkDestroyDebugReportCallbackEXT DestroyDebugReportCallbackEXT; PFN_vkDebugReportMessageEXT DebugReportMessageEXT; + PFN_vkDebugMarkerSetObjectTagEXT DebugMarkerSetObjectTagEXT; + PFN_vkDebugMarkerSetObjectNameEXT DebugMarkerSetObjectNameEXT; PFN_vkGetPhysicalDeviceSurfaceSupportKHR GetPhysicalDeviceSurfaceSupportKHR; PFN_vkGetPhysicalDeviceSurfaceCapabilitiesKHR GetPhysicalDeviceSurfaceCapabilitiesKHR; @@ -526,8 +530,7 @@ void loader_init_dispatch_dev_ext(struct loader_instance *inst, struct loader_device *dev); void *loader_dev_ext_gpa(struct loader_instance *inst, const char *funcName); void *loader_get_dev_ext_trampoline(uint32_t index); -void loader_override_terminating_device_proc( - VkDevice device, struct loader_dev_dispatch_table *disp_table); +void loader_override_terminating_device_proc(struct loader_device *dev); struct loader_instance *loader_get_instance(const VkInstance instance); void loader_deactivate_layers(const struct loader_instance *instance, struct loader_device *device, diff --git a/loader/table_ops.h b/loader/table_ops.h index f8f7225..0e42d0c 100644 --- a/loader/table_ops.h +++ b/loader/table_ops.h @@ -31,6 +31,7 @@ static VkResult vkDevExtError(VkDevice dev) { struct loader_device *found_dev; + // The device going in is a trampoline device struct loader_icd_term *icd_term = loader_get_icd_and_device(dev, &found_dev, NULL); @@ -541,11 +542,6 @@ loader_lookup_device_dispatch_table(const VkLayerDispatchTable *table, if (!strcmp(name, "CmdExecuteCommands")) return (void *)table->CmdExecuteCommands; - if (!strcmp(name, "CreateSwapchainKHR")) { - // For CreateSwapChainKHR we need to use trampoline and terminator - // functions to properly unwrap the SurfaceKHR object. - return (void *)vkCreateSwapchainKHR; - } if (!strcmp(name, "DestroySwapchainKHR")) return (void *)table->DestroySwapchainKHR; if (!strcmp(name, "GetSwapchainImagesKHR")) @@ -555,6 +551,20 @@ loader_lookup_device_dispatch_table(const VkLayerDispatchTable *table, if (!strcmp(name, "QueuePresentKHR")) return (void *)table->QueuePresentKHR; + // NOTE: Device Funcs needing Trampoline/Terminator. + // Overrides for device functions needing a trampoline and + // a terminator because certain device entry-points still need to go + // through a terminator before hitting the ICD. This could be for + // several reasons, but the main one is currently unwrapping an + // object before passing the appropriate info along to the ICD. + if (!strcmp(name, "CreateSwapchainKHR")) { + return (void *)vkCreateSwapchainKHR; + } else if (!strcmp(name, "DebugMarkerSetObjectTagEXT")) { + return (void *)vkDebugMarkerSetObjectTagEXT; + } else if (!strcmp(name, "DebugMarkerSetObjectNameEXT")) { + return (void *)vkDebugMarkerSetObjectNameEXT; + } + return NULL; } diff --git a/loader/trampoline.c b/loader/trampoline.c index dfeac5f..ff09ad4 100644 --- a/loader/trampoline.c +++ b/loader/trampoline.c @@ -710,7 +710,7 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateDevice( goto out; } - *pDevice = dev->device; + *pDevice = dev->chain_device; // Initialize any device extension dispatch entry's from the instance list loader_init_dispatch_dev_ext(inst, dev); @@ -721,12 +721,6 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateDevice( &dev->loader_dispatch, dev->loader_dispatch.core_dispatch.GetDeviceProcAddr, *pDevice); - // The loader needs to override some terminating device procs. Usually, - // these are device procs which need to go through a loader terminator. - // This needs to occur if the loader needs to perform some work prior - // to passing the work along to the ICD. - loader_override_terminating_device_proc(*pDevice, &dev->loader_dispatch); - out: // Failure cleanup @@ -761,8 +755,9 @@ vkDestroyDevice(VkDevice device, const VkAllocationCallbacks *pAllocator) { disp = loader_get_dispatch(device); disp->DestroyDevice(device, pAllocator); - dev->device = NULL; + dev->chain_device = NULL; loader_remove_logical_device(inst, icd_term, dev, pAllocator); + dev->icd_device = NULL; loader_platform_thread_unlock_mutex(&loader_lock); } -- 2.7.4