[Mesa-dev] [PATCH 1/2] anv: implementation of VK_EXT_debug_report extension
Jason Ekstrand
jason at jlekstrand.net
Fri Aug 25 05:08:21 UTC 2017
On Thu, Aug 24, 2017 at 9:52 PM, Tapani Pälli <tapani.palli at intel.com>
wrote:
> Hi;
>
> On 08/24/2017 08:36 PM, Jason Ekstrand wrote:
>
>> On Wed, Aug 23, 2017 at 11:23 PM, Tapani Pälli <tapani.palli at intel.com
>> <mailto: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
>> <mailto: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.
>>
>
> Thanks, I had a feeling that there might be a helper for this but did not
> figure it out.
>
> +
>> + 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().
>>
>
> Right, this would be for extending the functionality. I wrote it like this
> because it seems for me that the typical pattern would be to chain few
> report callbacks and send them all at once rather than calling the function
> n times. I'll change so that I handle first item out of the loop.
>
Is that allowed??? That would be weird but it honestly wouldn't surprise me
that much for this extension. Normally, you don't chain multiple of the
same struct together. You chain extension structs on to modify the
original struct. If inserting multiple callbacks at one go this way is
allowed, then we should probably do it the way you did it.
> + 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.
>>
>
> True, this is something I completely forgot. Will add locking.
>
>
>
> + 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.
>>
>
> Yep, I was wondering about this one as well.
>
> +{
>> + 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.
>>
>
> Yes, does not make sense. I went this way because I wanted instance
> ctor/dtor to call same functionality as anv_debug_report in case there will
> be something more that the call itself ... but now that it's just 2 lines
> .. I'll remove it.
>
> +{
>> + 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.
>>
>
> Because I copy-pasted the line from anv_DestroyDebugReportCallbackEXT at
> some point :) Will change to be unsafe version. Spec prohibits calling
> destroy from callback, section for vkDestroyDebugReportCallbackEXT says:
>
> "callback is an externally synchronized object and must not be used on
> more than one thread at a time. This means that
> vkDestroyDebugReportCallbackEXT must not be called when a callback is
> active."
>
Please put that spec quote as a comment above the list walk.
> 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.
>>
>
> I thought about it but it seems more complex as then one would need to
> special case this one callback when calling and know that we are inside
> instance ctor/dtor.
>
Fair enough.
> 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.
>>
>
> I have own test application that creates/destroys callbacks and then I've
> modified Mesa to report extra errors and perf warnings here and there to
> see that functionality works. Writing a crucible test makes sense, I'll do
> that.
>
I think it makes sense to just tie it in at a fairly high level in crucible
like we do for pAllocator. Crucible provides a pAllocator at the instance
level that memsets everything to 139 to help test for undefined values. We
can do something similar where crucible plugs in a debug report callback
and ties it into crucible's logging infastructure. Shouldn't be too hard.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170824/ac89829c/attachment-0001.html>
More information about the mesa-dev
mailing list