[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