[Mesa-dev] [PATCH 1/2] anv: implementation of VK_EXT_debug_report extension
Jason Ekstrand
jason at jlekstrand.net
Thu Aug 24 17:36:19 UTC 2017
On Wed, Aug 23, 2017 at 11:23 PM, Tapani Pälli <tapani.palli at intel.com>
wrote:
> 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 <tapani.palli at intel.com>
> ---
> 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;
>
This is what vk_alloc2 is for.
> +
> + vk_foreach_struct(info, pCreateInfo) {
>
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().
> + 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);
>
What kind of threading guarantees does debug_report provide? I'm guessing
none in which case we need to lock around this list.
> + 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);
>
lock
> + 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)
>
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.
> +{
> + 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)
>
Why is this broken out into a helper? There's nothing strictly wrong with
doing so but it seems kind-of pointless to me.
> +{
> + 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) {
>
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.
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.
> + 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;
>
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.
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.
> + 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170824/8a59176b/attachment-0001.html>
More information about the mesa-dev
mailing list