blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 776 Projects 19 Wiki Activity

Vulkan: Provide utilities to put markers and labels where needed in the GPUmodule #106098

Merged
Jeroen Bakker merged 29 commits from :vk_debug_update into main 2023-04-21 12:32:52 +02:00
Conversation 1 Commits 29 Files Changed 8 +40 -40
3 changed files with 40 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 35bfe856b6 - Show all commits

8
source/blender/gpu/vulkan/vk_context.cc
View File

@ -31,11 +31,11 @@ VKContext::VKContext(void *ghost_window, void *ghost_context)
&vk_device_,
&vk_queue_family_,
&vk_queue_);
debug::init_vk_callbacks(this, vkGetInstanceProcAddr);
debug::init_callbacks(this, vkGetInstanceProcAddr);
init_physical_device_limits();
debug::object_vk_label(this, vk_device_, "VkLogicalDevice");
debug::object_vk_label(this, vk_queue_, "VkGraphicsQueue");
debug::object_label(this, vk_device_, "LogicalDevice");
debug::object_label(this, vk_queue_, "GenericQueue");
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-04-03 13:36:00 +02:00
Outdated
Review
Copy Link

In the name Vk is redundant. Would use
"LogicalDevice" and "GenericQueue" is enough.

Generic as it handles compute shaders as well. In the future the plan is to have multiple queue families (async compute) as a separate queue.

In the name Vk is redundant. Would use "LogicalDevice" and "GenericQueue" is enough. Generic as it handles compute shaders as well. In the future the plan is to have multiple queue families (async compute) as a separate queue.
/* Initialize the memory allocator. */
VmaAllocatorCreateInfo info = {};
@ -61,7 +61,7 @@ VKContext::VKContext(void *ghost_window, void *ghost_context)
VKContext::~VKContext()
{
vmaDestroyAllocator(mem_allocator_);
debug::destroy_vk_callbacks(this);
debug::destroy_callbacks(this);
Kazashi Yoshioka commented 2023-04-12 14:19:20 +02:00
Review
Copy Link

This break is as a chain on the device. It has no direct relationship with VKContext.

This break is as a chain on the device. It has no direct relationship with VKContext.
}
void VKContext::init_physical_device_limits()

50
source/blender/gpu/vulkan/vk_debug.cc
View File

@ -60,10 +60,10 @@ void VKContext::debug_capture_scope_end(void * /*scope*/) {}
namespace blender::gpu::debug {
static void vulkan_dynamic_debug_functions(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
static void vulkan_dynamic_debug_functions(VKContext *context, PFN_vkGetInstanceProcAddr instload)
Jeroen Bakker commented 2023-04-17 09:10:41 +02:00
Outdated
Review
Copy Link

rename function to load_dynamic_functions. the terms vulkan and debug are redundant.

rename function to `load_dynamic_functions`. the terms vulkan and debug are redundant.
{
VKDebuggingTools &tools = ctx->debuggingtools_get();
VkInstance instance = ctx->instance_get();
VKDebuggingTools &tools = context->debuggingtools_get();
VkInstance instance = context->instance_get();
if (instload) {
@ -110,7 +110,7 @@ static void vulkan_dynamic_debug_functions(VKContext *ctx, PFN_vkGetInstanceProc
}
}
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
bool init_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
{
if (instload) {
vulkan_dynamic_debug_functions(ctx, instload);
@ -119,36 +119,36 @@ bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
return false;
vnapdv marked this conversation as resolved
Jeroen Bakker commented 2023-04-03 13:40:40 +02:00
Review
Copy Link

use context when referring to VKContext (be consistent with other areas of the code-base)

When referring to instances of the Vulkan API it is used. Eg VkCommandBuffer parameters should be named vk_command_buffer VKCommandBuffer (internal class) should be referred to as command_buffer.

Abbreviations like q should not be used, just call them what they are queue.

etc

use `context` when referring to `VKContext` (be consistent with other areas of the code-base) When referring to instances of the Vulkan API it is used. Eg VkCommandBuffer parameters should be named `vk_command_buffer` VKCommandBuffer (internal class) should be referred to as `command_buffer`. Abbreviations like `q` should not be used, just call them what they are `queue`. etc
}
void destroy_vk_callbacks(VKContext *ctx)
void destroy_callbacks(VKContext *context)
{
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 14:14:14 +01:00
Outdated
Review
Copy Link

/home/jeroen/blender-git/blender/source/blender/gpu/vulkan/vk_debug.cc:125:26: warning: variable ‘tools’ set but not used [-Wunused-but-set-variable]

`/home/jeroen/blender-git/blender/source/blender/gpu/vulkan/vk_debug.cc:125:26: warning: variable ‘tools’ set but not used [-Wunused-but-set-variable]`
vulkan_dynamic_debug_functions(ctx, nullptr);
vulkan_dynamic_debug_functions(context, nullptr);
}
}
void object_vk_label(blender::gpu::VKContext *ctx,
void object_label(VKContext *context,
Jeroen Bakker commented 2023-04-17 09:12:50 +02:00
Outdated
Review
Copy Link

obj isn't very useful. would use object_handle

`obj` isn't very useful. would use `object_handle`
VkObjectType objType,
Jeroen Bakker commented 2023-04-04 12:09:11 +02:00
Outdated
Review
Copy Link

objType -> vk_object_type

`objType` -> `vk_object_type`
uint64_t obj,
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 14:26:44 +01:00
Outdated
Review
Copy Link

Add empty line between each function.

Add empty line between each function.
const char *name)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
VkDebugUtilsObjectNameInfoEXT info = {};
info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_OBJECT_NAME_INFO_EXT;
info.objectType = objType;
info.objectHandle = obj;
info.pObjectName = name;
tools.vkSetDebugUtilsObjectNameEXT_r(ctx->device_get(), &info);
tools.vkSetDebugUtilsObjectNameEXT_r(context->device_get(), &info);
}
}
Jeroen Bakker commented 2023-04-17 09:14:23 +02:00
Review
Copy Link

cmd -> vk_command_buffer. No need to confuse readers with incorrect abbreviations.

Also check the other functions.

`cmd` -> `vk_command_buffer`. No need to confuse readers with incorrect abbreviations. Also check the other functions.
}
void pushMarker(blender::gpu::VKContext *ctx, VkCommandBuffer cmd, const char *name)
void push_marker(VKContext *context, VkCommandBuffer cmd, const char *name)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
VkDebugUtilsLabelEXT info = {};
info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT;
@ -158,10 +158,10 @@ void pushMarker(blender::gpu::VKContext *ctx, VkCommandBuffer cmd, const char *n
}
}
void setMarker(blender::gpu::VKContext *ctx, VkCommandBuffer cmd, const char *name)
void set_marker(VKContext *context, VkCommandBuffer cmd, const char *name)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
VkDebugUtilsLabelEXT info = {};
info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT;
@ -171,48 +171,48 @@ void setMarker(blender::gpu::VKContext *ctx, VkCommandBuffer cmd, const char *na
}
}
void popMarker(blender::gpu::VKContext *ctx, VkCommandBuffer cmd)
void pop_marker(VKContext *context, VkCommandBuffer cmd)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
tools.vkCmdEndDebugUtilsLabelEXT_r(cmd);
}
}
Jeroen Bakker commented 2023-04-17 09:16:32 +02:00
Review
Copy Link

queue -> vk_queue.

I add vk_ to all Vulkan API data types to not confuse them with our internal types. Best to keep this consistent. I know not all parts already use this convention. But would want all the new code to follow it.

Also check other functions below.

`queue` -> `vk_queue`. I add `vk_` to all Vulkan API data types to not confuse them with our internal types. Best to keep this consistent. I know not all parts already use this convention. But would want all the new code to follow it. Also check other functions below.
}
void pushMarker(blender::gpu::VKContext *ctx, VkQueue q, const char *name)
void push_marker(VKContext *context, VkQueue queue, const char *name)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
VkDebugUtilsLabelEXT info = {};
info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT;
info.pLabelName = name;
tools.vkQueueBeginDebugUtilsLabelEXT_r(q, &info);
tools.vkQueueBeginDebugUtilsLabelEXT_r(queue, &info);
}
}
}
void setMarker(blender::gpu::VKContext *ctx, VkQueue q, const char *name)
void set_marker(VKContext *context, VkQueue queue, const char *name)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
VkDebugUtilsLabelEXT info = {};
info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT;
info.pLabelName = name;
tools.vkQueueInsertDebugUtilsLabelEXT_r(q, &info);
tools.vkQueueInsertDebugUtilsLabelEXT_r(queue, &info);
}
}
}
void popMarker(blender::gpu::VKContext *ctx, VkQueue q)
void pop_marker(VKContext *context, VkQueue queue)
{
if (G.debug & G_DEBUG_GPU) {
VKDebuggingTools tools = ctx->debuggingtools_get();
VKDebuggingTools tools = context->debuggingtools_get();
if (tools.enabled) {
tools.vkQueueEndDebugUtilsLabelEXT_r(q);
tools.vkQueueEndDebugUtilsLabelEXT_r(queue);
}
}
}

22
source/blender/gpu/vulkan/vk_debug.hh
View File

@ -32,11 +32,11 @@ typedef struct VKDebuggingTools {
PFN_vkSetDebugUtilsObjectTagEXT vkSetDebugUtilsObjectTagEXT_r = nullptr;
} VKDebuggingTools;
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload);
void destroy_vk_callbacks(VKContext *ctx);
void object_vk_label(VKContext *ctx, VkObjectType objType, uint64_t obj, const char *name);
bool init_callbacks(VKContext *context, PFN_vkGetInstanceProcAddr instload);
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-04-03 13:49:41 +02:00
Outdated
Review
Copy Link

we should remove vk from the function names it is redundant in these cases

we should remove vk from the function names it is redundant in these cases
void destroy_callbacks(VKContext *context);
void object_label(VKContext *context, VkObjectType objType, uint64_t obj, const char *name);
template<typename T> void object_vk_label(VKContext *ctx, T obj, const char *name)
template<typename T> void object_label(VKContext *context, T obj, const char *name)
Jeroen Bakker commented 2023-04-17 09:17:40 +02:00
Outdated
Review
Copy Link

obj -> object_handle

`obj` -> `object_handle`
{
if (!(G.debug & G_DEBUG_GPU)) {
return;
@ -46,15 +46,15 @@ template<typename T> void object_vk_label(VKContext *ctx, T obj, const char *nam
memset(label, 0, label_size);
static int stats = 0;
SNPRINTF(label, "%s_%d", name, stats++);
object_vk_label(ctx, to_vk_object_type(obj), (uint64_t)obj, (const char *)label);
object_label(context, to_vk_object_type(obj), (uint64_t)obj, (const char *)label);
};
void pushMarker(VKContext *ctx, VkCommandBuffer cmd, const char *name);
void setMarker(VKContext *ctx, VkCommandBuffer cmd, const char *name);
void popMarker(VKContext *ctx, VkCommandBuffer cmd);
void pushMarker(VKContext *ctx, VkQueue q, const char *name);
void setMarker(VKContext *ctx, VkQueue q, const char *name);
void popMarker(VKContext *ctx, VkQueue q);
void push_marker(VKContext *context, VkCommandBuffer cmd, const char *name);
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-04-03 13:50:34 +02:00
Outdated
Review
Copy Link

push_marker / pop_marker / set_marker

`push_marker` / `pop_marker` / `set_marker`
Jeroen Bakker commented 2023-04-17 09:18:14 +02:00
Outdated
Review
Copy Link

Also update the definitions to match the parameter names of the implementation.

Also update the definitions to match the parameter names of the implementation.
void set_marker(VKContext *context, VkCommandBuffer cmd, const char *name);
void pop_marker(VKContext *context, VkCommandBuffer cmd);
void push_marker(VKContext *context, VkQueue queue, const char *name);
void set_marker(VKContext *context, VkQueue queue, const char *name);
void pop_marker(VKContext *context, VkQueue queue);
} // namespace debug
} // namespace blender::gpu