From 76ce8c30075a5bd62bd8c2cf0f07a0f851a24a55 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Wed, 13 Apr 2022 14:00:16 +0000 Subject: [PATCH] Refactored how debug labels are set with Vulkan Use templated functions to make calling SetDebugLabel safer, with less opportunities to screw up the casting and automatic lookup of the VkObjectType for most handles. Change-Id: I0938ad6fd7d5fe81569bdee5bc7ec7e396db7bcd Bug: dawn:1323 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/86580 Kokoro: Kokoro Reviewed-by: Corentin Wallez Commit-Queue: Brandon Jones --- src/dawn/native/vulkan/BindGroupLayoutVk.cpp | 3 +- src/dawn/native/vulkan/BindGroupVk.cpp | 3 +- src/dawn/native/vulkan/BufferVk.cpp | 3 +- src/dawn/native/vulkan/ComputePipelineVk.cpp | 3 +- src/dawn/native/vulkan/DeviceVk.cpp | 6 +--- src/dawn/native/vulkan/PipelineLayoutVk.cpp | 3 +- src/dawn/native/vulkan/QuerySetVk.cpp | 3 +- src/dawn/native/vulkan/QueueVk.cpp | 7 +--- src/dawn/native/vulkan/RenderPipelineVk.cpp | 3 +- src/dawn/native/vulkan/SamplerVk.cpp | 3 +- src/dawn/native/vulkan/ShaderModuleVk.cpp | 4 +-- src/dawn/native/vulkan/StagingBufferVk.cpp | 3 +- src/dawn/native/vulkan/TextureVk.cpp | 6 ++-- src/dawn/native/vulkan/UtilsVulkan.cpp | 29 ++++++++++++++--- src/dawn/native/vulkan/UtilsVulkan.h | 34 ++++++++++++++++++-- 15 files changed, 70 insertions(+), 43 deletions(-) diff --git a/src/dawn/native/vulkan/BindGroupLayoutVk.cpp b/src/dawn/native/vulkan/BindGroupLayoutVk.cpp index c377c969dd..8ed9b93a56 100644 --- a/src/dawn/native/vulkan/BindGroupLayoutVk.cpp +++ b/src/dawn/native/vulkan/BindGroupLayoutVk.cpp @@ -192,8 +192,7 @@ namespace dawn::native::vulkan { } void BindGroupLayout::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_DESCRIPTOR_SET_LAYOUT, - reinterpret_cast(mHandle), "Dawn_BindGroupLayout", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_BindGroupLayout", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/BindGroupVk.cpp b/src/dawn/native/vulkan/BindGroupVk.cpp index 00e70cfc16..b55c10f62e 100644 --- a/src/dawn/native/vulkan/BindGroupVk.cpp +++ b/src/dawn/native/vulkan/BindGroupVk.cpp @@ -157,8 +157,7 @@ namespace dawn::native::vulkan { } void BindGroup::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_DESCRIPTOR_SET, - reinterpret_cast(mDescriptorSetAllocation.set), "Dawn_BindGroup", + SetDebugName(ToBackend(GetDevice()), mDescriptorSetAllocation.set, "Dawn_BindGroup", GetLabel()); } diff --git a/src/dawn/native/vulkan/BufferVk.cpp b/src/dawn/native/vulkan/BufferVk.cpp index c7e9fb02d3..4045c50e75 100644 --- a/src/dawn/native/vulkan/BufferVk.cpp +++ b/src/dawn/native/vulkan/BufferVk.cpp @@ -381,8 +381,7 @@ namespace dawn::native::vulkan { } void Buffer::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_BUFFER, - reinterpret_cast(mHandle), "Dawn_Buffer", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_Buffer", GetLabel()); } void Buffer::InitializeToZero(CommandRecordingContext* recordingContext) { diff --git a/src/dawn/native/vulkan/ComputePipelineVk.cpp b/src/dawn/native/vulkan/ComputePipelineVk.cpp index 68ac7d9be0..9a3fa8b676 100644 --- a/src/dawn/native/vulkan/ComputePipelineVk.cpp +++ b/src/dawn/native/vulkan/ComputePipelineVk.cpp @@ -93,8 +93,7 @@ namespace dawn::native::vulkan { } void ComputePipeline::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_PIPELINE, - reinterpret_cast(mHandle), "Dawn_ComputePipeline", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_ComputePipeline", GetLabel()); } ComputePipeline::~ComputePipeline() = default; diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp index 9e77066054..c3ded043aa 100644 --- a/src/dawn/native/vulkan/DeviceVk.cpp +++ b/src/dawn/native/vulkan/DeviceVk.cpp @@ -1053,11 +1053,7 @@ namespace dawn::native::vulkan { } void Device::SetLabelImpl() { - // VKDevice reinterpret_casts to a uint64_t rather than a uint64_t& like most other types - // because it's a dispatchable handle, and thus doesn't have the VkHandle wrapper that - // Dawn creates for anything defined with VK_DEFINE_NON_DISPATCHABLE_HANDLE. - SetDebugName(this, VK_OBJECT_TYPE_DEVICE, reinterpret_cast(mVkDevice), - "Dawn_Device", GetLabel()); + SetDebugName(this, VK_OBJECT_TYPE_DEVICE, mVkDevice, "Dawn_Device", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/PipelineLayoutVk.cpp b/src/dawn/native/vulkan/PipelineLayoutVk.cpp index e6653bd980..560731b856 100644 --- a/src/dawn/native/vulkan/PipelineLayoutVk.cpp +++ b/src/dawn/native/vulkan/PipelineLayoutVk.cpp @@ -83,8 +83,7 @@ namespace dawn::native::vulkan { } void PipelineLayout::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_PIPELINE_LAYOUT, - reinterpret_cast(mHandle), "Dawn_PipelineLayout", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_PipelineLayout", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/QuerySetVk.cpp b/src/dawn/native/vulkan/QuerySetVk.cpp index 398179304b..04895f083d 100644 --- a/src/dawn/native/vulkan/QuerySetVk.cpp +++ b/src/dawn/native/vulkan/QuerySetVk.cpp @@ -110,8 +110,7 @@ namespace dawn::native::vulkan { } void QuerySet::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_QUERY_POOL, - reinterpret_cast(mHandle), "Dawn_QuerySet", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_QuerySet", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/QueueVk.cpp b/src/dawn/native/vulkan/QueueVk.cpp index 526a6ef584..2166be1f54 100644 --- a/src/dawn/native/vulkan/QueueVk.cpp +++ b/src/dawn/native/vulkan/QueueVk.cpp @@ -66,14 +66,9 @@ namespace dawn::native::vulkan { void Queue::SetLabelImpl() { Device* device = ToBackend(GetDevice()); - // VKDevice reinterpret_casts to a uint64_t rather than a uint64_t& like most other types - // because it's a dispatchable handle, and thus doesn't have the VkHandle wrapper that - // Dawn creates for anything defined with VK_DEFINE_NON_DISPATCHABLE_HANDLE. - // TODO(crbug.com/dawn/1344): When we start using multiple queues this needs to be adjusted // so it doesn't always change the default queue's label. - SetDebugName(device, VK_OBJECT_TYPE_QUEUE, reinterpret_cast(device->GetQueue()), - "Dawn_Queue", GetLabel()); + SetDebugName(device, VK_OBJECT_TYPE_QUEUE, device->GetQueue(), "Dawn_Queue", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/RenderPipelineVk.cpp b/src/dawn/native/vulkan/RenderPipelineVk.cpp index 2eca3c95b8..9b5349dcea 100644 --- a/src/dawn/native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn/native/vulkan/RenderPipelineVk.cpp @@ -574,8 +574,7 @@ namespace dawn::native::vulkan { } void RenderPipeline::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_PIPELINE, - reinterpret_cast(mHandle), "Dawn_RenderPipeline", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_RenderPipeline", GetLabel()); } VkPipelineVertexInputStateCreateInfo RenderPipeline::ComputeVertexInputDesc( diff --git a/src/dawn/native/vulkan/SamplerVk.cpp b/src/dawn/native/vulkan/SamplerVk.cpp index c7fc1a362c..629fd1bd51 100644 --- a/src/dawn/native/vulkan/SamplerVk.cpp +++ b/src/dawn/native/vulkan/SamplerVk.cpp @@ -124,8 +124,7 @@ namespace dawn::native::vulkan { } void Sampler::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_SAMPLER, - reinterpret_cast(mHandle), "Dawn_Sampler", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_Sampler", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp index da9ca3db7a..81e136845f 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp @@ -245,9 +245,7 @@ namespace dawn::native::vulkan { mTransformedShaderModuleCache->AddOrGet(cacheKey, newHandle, std::move(spirv)); } - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_SHADER_MODULE, - reinterpret_cast(moduleAndSpirv.first), "Dawn_ShaderModule", - GetLabel()); + SetDebugName(ToBackend(GetDevice()), moduleAndSpirv.first, "Dawn_ShaderModule", GetLabel()); return std::move(moduleAndSpirv); #else diff --git a/src/dawn/native/vulkan/StagingBufferVk.cpp b/src/dawn/native/vulkan/StagingBufferVk.cpp index fb6631592e..97b0c61dfc 100644 --- a/src/dawn/native/vulkan/StagingBufferVk.cpp +++ b/src/dawn/native/vulkan/StagingBufferVk.cpp @@ -58,8 +58,7 @@ namespace dawn::native::vulkan { return DAWN_INTERNAL_ERROR("Unable to map staging buffer."); } - SetDebugName(mDevice, VK_OBJECT_TYPE_BUFFER, reinterpret_cast(mBuffer), - "Dawn_StagingBuffer"); + SetDebugName(mDevice, mBuffer, "Dawn_StagingBuffer"); return {}; } diff --git a/src/dawn/native/vulkan/TextureVk.cpp b/src/dawn/native/vulkan/TextureVk.cpp index e6adf1478c..06dd6887b9 100644 --- a/src/dawn/native/vulkan/TextureVk.cpp +++ b/src/dawn/native/vulkan/TextureVk.cpp @@ -889,8 +889,7 @@ namespace dawn::native::vulkan { } void Texture::SetLabelHelper(const char* prefix) { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_IMAGE, - reinterpret_cast(mHandle), prefix, GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, prefix, GetLabel()); } void Texture::SetLabelImpl() { @@ -1423,8 +1422,7 @@ namespace dawn::native::vulkan { } void TextureView::SetLabelImpl() { - SetDebugName(ToBackend(GetDevice()), VK_OBJECT_TYPE_IMAGE_VIEW, - reinterpret_cast(mHandle), "Dawn_InternalTextureView", GetLabel()); + SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_TextureView", GetLabel()); } } // namespace dawn::native::vulkan diff --git a/src/dawn/native/vulkan/UtilsVulkan.cpp b/src/dawn/native/vulkan/UtilsVulkan.cpp index c5290d9114..0d0e86fa69 100644 --- a/src/dawn/native/vulkan/UtilsVulkan.cpp +++ b/src/dawn/native/vulkan/UtilsVulkan.cpp @@ -26,6 +26,25 @@ namespace dawn::native::vulkan { +#define VK_OBJECT_TYPE_GETTER(object, objectType) \ + template <> \ + VkObjectType GetVkObjectType(object handle) { \ + return objectType; \ + } + + VK_OBJECT_TYPE_GETTER(VkBuffer, VK_OBJECT_TYPE_BUFFER) + VK_OBJECT_TYPE_GETTER(VkDescriptorSetLayout, VK_OBJECT_TYPE_DESCRIPTOR_SET_LAYOUT) + VK_OBJECT_TYPE_GETTER(VkDescriptorSet, VK_OBJECT_TYPE_DESCRIPTOR_SET) + VK_OBJECT_TYPE_GETTER(VkPipeline, VK_OBJECT_TYPE_PIPELINE) + VK_OBJECT_TYPE_GETTER(VkPipelineLayout, VK_OBJECT_TYPE_PIPELINE_LAYOUT) + VK_OBJECT_TYPE_GETTER(VkQueryPool, VK_OBJECT_TYPE_QUERY_POOL) + VK_OBJECT_TYPE_GETTER(VkSampler, VK_OBJECT_TYPE_SAMPLER) + VK_OBJECT_TYPE_GETTER(VkShaderModule, VK_OBJECT_TYPE_SHADER_MODULE) + VK_OBJECT_TYPE_GETTER(VkImage, VK_OBJECT_TYPE_IMAGE) + VK_OBJECT_TYPE_GETTER(VkImageView, VK_OBJECT_TYPE_IMAGE_VIEW) + +#undef VK_OBJECT_TYPE_GETTER + VkCompareOp ToVulkanCompareOp(wgpu::CompareFunction op) { switch (op) { case wgpu::CompareFunction::Never: @@ -182,11 +201,11 @@ namespace dawn::native::vulkan { return region; } - void SetDebugName(Device* device, - VkObjectType objectType, - uint64_t objectHandle, - const char* prefix, - std::string label) { + void SetDebugNameInternal(Device* device, + VkObjectType objectType, + uint64_t objectHandle, + const char* prefix, + std::string label) { if (!objectHandle) { return; } diff --git a/src/dawn/native/vulkan/UtilsVulkan.h b/src/dawn/native/vulkan/UtilsVulkan.h index 4ef8e459ff..650fe6fc88 100644 --- a/src/dawn/native/vulkan/UtilsVulkan.h +++ b/src/dawn/native/vulkan/UtilsVulkan.h @@ -103,11 +103,41 @@ namespace dawn::native::vulkan { const TextureCopy& textureCopy, const Extent3D& copySize); + // Gets the associated VkObjectType for any non-dispatchable handle + template + VkObjectType GetVkObjectType(HandleType handle); + + void SetDebugNameInternal(Device* device, + VkObjectType objectType, + uint64_t objectHandle, + const char* prefix, + std::string label); + + // The majority of Vulkan handles are "non-dispatchable". Dawn wraps these by overriding + // VK_DEFINE_NON_DISPATCHABLE_HANDLE to add some capabilities like making null comparisons + // easier. In those cases we can make setting the debug name a bit easier by getting the + // object type automatically and handling the indirection to the native handle. + template + void SetDebugName(Device* device, + detail::VkHandle objectHandle, + const char* prefix, + std::string label = "") { + SetDebugNameInternal(device, GetVkObjectType(objectHandle), + reinterpret_cast(objectHandle.GetHandle()), prefix, label); + } + + // Handles like VkQueue and VKDevice require a special path because they are dispatchable, so + // they require an explicit VkObjectType and cast to a uint64_t directly rather than by getting + // the non-dispatchable wrapper's underlying handle. + template void SetDebugName(Device* device, VkObjectType objectType, - uint64_t objectHandle, + HandleType objectHandle, const char* prefix, - std::string label = ""); + std::string label = "") { + SetDebugNameInternal(device, objectType, reinterpret_cast(objectHandle), prefix, + label); + } // Returns nullptr or &specializationInfo // specializationInfo, specializationDataEntries, specializationMapEntries needs to