<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 23, 2017 at 11:23 PM, Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Patch adds required functionality for extension to manage a list of application provided callbacks and handle debug reporting from driver and application side. Signed-off-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>> --- src/intel/Makefile.sources | 1 + src/intel/vulkan/anv_debug_report.c | 133 ++++++++++++++++++++++++++++++++++++ src/intel/vulkan/anv_device.c | 40 +++++++++++ src/intel/vulkan/anv_extensions.py | 1 + src/intel/vulkan/anv_private.h | 32 +++++++++ 5 files changed, 207 insertions(+) create mode 100644 src/intel/vulkan/anv_debug_report.c diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources index 4074ba9ee5..200713b06e 100644 --- a/src/intel/Makefile.sources +++ b/src/intel/Makefile.sources @@ -205,6 +205,7 @@ VULKAN_FILES := \ vulkan/anv_batch_chain.c \ vulkan/anv_blorp.c \ vulkan/anv_cmd_buffer.c \ + vulkan/anv_debug_report.c \ vulkan/anv_descriptor_set.c \ vulkan/anv_device.c \ vulkan/anv_dump.c \ diff --git a/src/intel/vulkan/anv_debug_report.c b/src/intel/vulkan/anv_debug_report.c new file mode 100644 index 0000000000..1a4868cd52 --- /dev/null +++ b/src/intel/vulkan/anv_debug_report.c @@ -0,0 +1,133 @@ +/* + * Copyright © 2017 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "anv_private.h" +#include "vk_util.h" + +/* This file contains implementation for VK_EXT_debug_report. */ + +VkResult +anv_CreateDebugReportCallbackEXT(VkInstance _instance, + const VkDebugReportCallbackCreateInfoEXT* pCreateInfo, + const VkAllocationCallbacks* pAllocator, + VkDebugReportCallbackEXT* pCallback) +{ + ANV_FROM_HANDLE(anv_instance, instance, _instance); + const VkAllocationCallbacks *alloc = + pAllocator ? pAllocator : &instance->alloc; </blockquote><div> </div><div>This is what vk_alloc2 is for. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + vk_foreach_struct(info, pCreateInfo) { </blockquote><div> </div><div>Usually, we handle the primary structure directly and then call vk_foreach_struct on pCreateInfo->pNext. This is because the things in the pNext chain are going to be modifiers to the original thing so they probably need to happen between allocating the callback and list_addtail(). </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + switch (info->sType) { + case VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT: { + struct anv_debug_callback *cb = + vk_alloc(alloc, sizeof(struct anv_debug_callback), 8, + VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); + if (!cb) + return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY); + + cb->flags = pCreateInfo->flags; + cb->callback = pCreateInfo->pfnCallback; + cb->data = pCreateInfo->pUserData; + + list_addtail(&cb->link, &instance->callbacks); </blockquote><div> </div><div>What kind of threading guarantees does debug_report provide? I'm guessing none in which case we need to lock around this list. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + break; + } + default: + anv_debug_ignored_stype(info->sType); + break; + } + } + + return VK_SUCCESS; +} + +void +anv_DestroyDebugReportCallbackEXT(VkInstance _instance, + VkDebugReportCallbackEXT callback, + const VkAllocationCallbacks* pAllocator) +{ + ANV_FROM_HANDLE(anv_instance, instance, _instance); + const VkAllocationCallbacks *alloc = + pAllocator ? pAllocator : &instance->alloc; + + list_for_each_entry_safe(struct anv_debug_callback, debug_cb, + &instance->callbacks, link) { + /* Found a match, remove from list and destroy given callback. */ + if ((VkDebugReportCallbackEXT)debug_cb->callback == callback) { + list_del(&debug_cb->link); </blockquote><div> </div><div>lock </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + vk_free(alloc, debug_cb); + } + } +} + +void +anv_DebugReportMessageEXT(VkInstance _instance, + VkDebugReportFlagsEXT flags, + VkDebugReportObjectTypeEXT objectType, + uint64_t object, + size_t location, + int32_t messageCode, + const char* pLayerPrefix, + const char* pMessage) </blockquote><div> </div><div>Woah... This is a bit unexpected. I wonder why this entrypoint even exists. One would think that the loader could just do the aggrigation without it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + ANV_FROM_HANDLE(anv_instance, instance, _instance); + anv_debug_report(instance, flags, objectType, object, + location, messageCode, pLayerPrefix, pMessage); + +} + +void +anv_debug_report_call(struct anv_debug_callback *cb, + VkDebugReportFlagsEXT flags, + VkDebugReportObjectTypeEXT object_type, + uint64_t handle, + size_t location, + int32_t messageCode, + const char* pLayerPrefix, + const char *pMessage) </blockquote><div> </div><div>Why is this broken out into a helper? There's nothing strictly wrong with doing so but it seems kind-of pointless to me. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + cb->callback(flags, object_type, handle, location, messageCode, + pLayerPrefix, pMessage, cb->data); +} + +void +anv_debug_report(struct anv_instance *instance, + VkDebugReportFlagsEXT flags, + VkDebugReportObjectTypeEXT object_type, + uint64_t handle, + size_t location, + int32_t messageCode, + const char* pLayerPrefix, + const char *pMessage) +{ + /* Allow passing NULL for now. */ + if (!instance) + return; + + list_for_each_entry_safe(struct anv_debug_callback, cb, + &instance->callbacks, link) { </blockquote><div> </div><div>Why is are you using for_each_entry_safe? You're not deleting anything. I suppose it is possible that one of the callbacks could respond by deleting itself but that seems weird. If that's legal then we have a bunch of other weird things to consider.</div><div> </div><div>Also, I think we need to lock around walking this list. Just to keep things efficient, we probably want to do an "if (list_empty(&instance->callbacks)) return" before taking the lock. It's safe to check list_empty outside the lock because it's just one pointer compare. The worst that happens is list_empty returns false and then we go, lock, and walk the list. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cb->flags & flags) + anv_debug_report_call(cb, flags, object_type, handle, + location, messageCode, pLayerPrefix, pMessage); + } +} diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c index a6d5215ab8..01e1869720 100644 --- a/src/intel/vulkan/anv_device.c +++ b/src/intel/vulkan/anv_device.c @@ -441,9 +441,32 @@ VkResult anv_CreateInstance( VkInstance* pInstance) { struct anv_instance *instance; + struct anv_debug_callback ctor_cb = { 0 }; assert(pCreateInfo->sType == VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO); + /* Check if user passed a debug report callback to be used during + * Create/Destroy of instance. + */ + vk_foreach_struct(ext, pCreateInfo->pNext) { + switch (ext->sType) { + case VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT: { + VkDebugReportCallbackCreateInfoEXT *cb = + (VkDebugReportCallbackCreateInfoEXT *) ext; + ctor_cb.flags = cb->flags; + ctor_cb.callback = cb->pfnCallback; + ctor_cb.data = cb->pUserData; </blockquote><div> </div><div>Would it be worth adding this to the list and them removing it later? I don't know if that would actually simplify things or not.</div><div> </div><div>Also, have you written any tests for debug_report? Really, the only thing we can test it for is "did it crash?" but it's probably worth adding some debug_report stuff to crucible for that purpose. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + break; + } + default: + /* We ignore any other type by purpose here, we are only + * interested if debug report callback was found for instance + * ctor/dtor. + */ + break; + } + }; + uint32_t client_version; if (pCreateInfo->pApplicationInfo && pCreateInfo->pApplicationInfo->apiVersion != 0) { @@ -454,6 +477,17 @@ VkResult anv_CreateInstance( if (VK_MAKE_VERSION(1, 0, 0) > client_version || client_version > VK_MAKE_VERSION(1, 0, 0xfff)) { + + if (ctor_cb.callback && ctor_cb.flags & VK_DEBUG_REPORT_ERROR_BIT_EXT) + anv_debug_report_call(&ctor_cb, + VK_DEBUG_REPORT_ERROR_BIT_EXT, + VK_DEBUG_REPORT_OBJECT_TYPE_INSTANCE_EXT, + 0, /* No handle available yet. */ + __LINE__, + 0, + "anv", + "incompatible driver version"); + return vk_errorf(VK_ERROR_INCOMPATIBLE_DRIVER, "Client requested version %d.%d.%d", VK_VERSION_MAJOR(client_version), @@ -482,6 +516,12 @@ VkResult anv_CreateInstance( instance->apiVersion = client_version; instance->physicalDeviceCount = -1; + list_inithead(&instance->callbacks); + + /* Store report debug callback to be used during DestroyInstance. */ + if (ctor_cb.callback) + memcpy(&instance->dtor_cb, &ctor_cb, sizeof(struct anv_debug_callback)); + _mesa_locale_init(); VG(VALGRIND_CREATE_MEMPOOL(instance, 0, false)); diff --git a/src/intel/vulkan/anv_extensions.py b/src/intel/vulkan/anv_extensions.py index 3252e0fc56..db759bd467 100644 --- a/src/intel/vulkan/anv_extensions.py +++ b/src/intel/vulkan/anv_extensions.py @@ -70,6 +70,7 @@ EXTENSIONS = [ Extension('VK_KHR_xcb_surface', 6, 'VK_USE_PLATFORM_XCB_KHR'), Extension('VK_KHR_xlib_surface', 6, 'VK_USE_PLATFORM_XLIB_KHR'), Extension('VK_KHX_multiview', 1, True), + Extension('VK_EXT_debug_report', 8, True), ] class VkVersion: diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 6b2414429f..e3733d5052 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -61,6 +61,8 @@ typedef uint32_t xcb_window_t; struct anv_buffer; struct anv_buffer_view; struct anv_image_view; +struct anv_instance; +struct anv_debug_callback; struct gen_l3_config; @@ -237,6 +239,24 @@ void __anv_perf_warn(const char *file, int line, const char *format, ...) void anv_loge(const char *format, ...) anv_printflike(1, 2); void anv_loge_v(const char *format, va_list va); +void anv_debug_report(struct anv_instance *instance, + VkDebugReportFlagsEXT flags, + VkDebugReportObjectTypeEXT object_type, + uint64_t handle, + size_t location, + int32_t messageCode, + const char* pLayerPrefix, + const char *pMessage); + +void anv_debug_report_call(struct anv_debug_callback *cb, + VkDebugReportFlagsEXT flags, + VkDebugReportObjectTypeEXT object_type, + uint64_t handle, + size_t location, + int32_t messageCode, + const char* pLayerPrefix, + const char *pMessage); + /** * Print a FINISHME message, including its source location. */ @@ -665,6 +685,14 @@ struct anv_physical_device { int local_fd; }; +struct anv_debug_callback { + /* Link in the 'callbacks' list in anv_instance struct. */ + struct list_head link; + VkDebugReportFlagsEXT flags; + PFN_vkDebugReportCallbackEXT callback; + void * data; +}; + struct anv_instance { VK_LOADER_DATA _loader_data; @@ -673,6 +701,10 @@ struct anv_instance { uint32_t apiVersion; int physicalDeviceCount; struct anv_physical_device physicalDevice; + + /* VK_EXT_debug_report debug callbacks */ + struct list_head callbacks; + struct anv_debug_callback dtor_cb; }; VkResult anv_init_wsi(struct anv_physical_device *physical_device); -- 2.13.5 </blockquote></div> </div></div>