[Mesa-dev] [PATCH 1/2] anv: implementation of VK_EXT_debug_report extension
Tapani Pälli
tapani.palli at intel.com
Fri Aug 25 05:22:51 UTC 2017
On 08/25/2017 08:08 AM, Jason Ekstrand wrote:
> On Thu, Aug 24, 2017 at 9:52 PM, Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>> wrote:
>
>
> +
> + 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.
Heh I see now that this was my own invention, I thought these chains
allow one to create multiple objects with one call but that's not true,
these are meant for extending. Sorry about that, will fix.
> + 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.
will do
> 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.
OK will get familiar with crucible, thanks!
// tapani
More information about the mesa-dev
mailing list