On 08/25/2017 08:08 AM, Jason Ekstrand wrote:
On Thu, Aug 24, 2017 at 9:52 PM, Tapani Pälli <tapani.pa...@intel.com <mailto:tapani.pa...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to