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
Contributor

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


This functionality is roughly equivalent to that used in OpenGL.

The list is as follows.

  • Give a name to the Object created by the vulkan function.
  • Do marking to reveal the scope of the CommandBuffer.
  • Marking to reveal the scope of the Queue.

Technical note:

This PR uses the VK_EXT_debug_utils extension, but it's only for labeling, so it doesn't rely on the VK_LAYER_KHRONOS_validation functionality.

The functions that do these things are loaded into the runtime as vulkan extensions.

Declare the function pointers in a struct and make them members of vk_context.

#### GPU Vulkan:Provide utilities to put markers and labels where needed in the GPUmodule ---- #### This functionality is roughly equivalent to that used in OpenGL. The list is as follows. - Give a name to the Object created by the vulkan function. - Do marking to reveal the scope of the CommandBuffer. - Marking to reveal the scope of the Queue. #### Technical note: This PR uses the VK_EXT_debug_utils extension, but it's only for labeling, so it doesn't rely on the VK_LAYER_KHRONOS_validation functionality. The functions that do these things are loaded into the runtime as vulkan extensions. Declare the function pointers in a struct and make them members of vk_context.
Kazashi Yoshioka added 11 commits 2023-03-24 11:54:14 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-24 11:55:05 +01:00
Jeroen Bakker requested changes 2023-03-24 14:31:33 +01:00
Jeroen Bakker left a comment
Member

I just did a quick overview. still need to do a more indepth review to make it more Blenderish.

I just did a quick overview. still need to do a more indepth review to make it more Blenderish.
@ -869,3 +873,2 @@
vector<const char *> extensions_device;
vector<const char *> extensions_enabled;
Member

One empty line is enough here.

Make sure you run make format or have clang-format configured correctly in your IDE.

One empty line is enough here. Make sure you run `make format` or have clang-format configured correctly in your IDE.
vnapdv marked this conversation as resolved
@ -256,6 +256,8 @@ set(VULKAN_SRC
vulkan/vk_texture.hh
vulkan/vk_uniform_buffer.hh
vulkan/vk_vertex_buffer.hh
vulkan/vk_debug.hh
Member

Try to keep the list of files sorted. This way it is easier to find missing ones.

Try to keep the list of files sorted. This way it is easier to find missing ones.
vnapdv marked this conversation as resolved
@ -16,6 +16,7 @@
#include "vk_mem_alloc.h"
#include "gpu_texture_private.hh"
#include <typeinfo>
Member

Add one line between imports of different categories. Put system includes on top.

Add one line between imports of different categories. Put system includes on top.
vnapdv marked this conversation as resolved
@ -28,0 +88,4 @@
BLI_assert_unreachable();
#undef VK_EQ_TYPEID
return VK_OBJECT_TYPE_UNKNOWN;
} // namespace blender::gpu
Member

// namespace blender::gpu can be removed.

`// namespace blender::gpu` can be removed.
vnapdv marked this conversation as resolved
@ -5,3 +5,3 @@
* \ingroup gpu
*/
#include "BKE_global.h"
Member

Add empty lines between each category of includes

Add empty lines between each category of includes
vnapdv marked this conversation as resolved
@ -8,2 +10,4 @@
#include "vk_debug.hh"
#include "vk_backend.hh"
#include "vk_context.hh"
static CLG_LogRef LOG = { "gpu.debug.vulkan" };
Member

Add empty line between includes and code.

Add empty line between includes and code.
vnapdv marked this conversation as resolved
@ -63,0 +122,4 @@
bool init_vk_callbacks(VKContext* ctx, PFN_vkGetInstanceProcAddr instload)
{
CLOG_ENSURE(&LOG);
VKDebuggingTools tools = ctx->debuggingtools_get();
Member

/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]`
vnapdv marked this conversation as resolved
@ -63,0 +129,4 @@
};
return false;
}
void destroy_vk_callbacks(VKContext* ctx)
Member

Add empty line between each function.

Add empty line between each function.
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2023-03-24 17:01:20 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-24 17:03:53 +01:00
Kazashi Yoshioka added 4 commits 2023-03-28 23:51:45 +02:00
Author
Contributor

Regarding the labeling function of vk_ext_debug_utils, it seems that it does not depend on VK_VALIDATION_LAYER.

Regarding the labeling function of vk_ext_debug_utils, it seems that it does not depend on VK_VALIDATION_LAYER.
Jeroen Bakker requested changes 2023-04-03 13:55:11 +02:00
Jeroen Bakker left a comment
Member

I did a first review. haven't tested it yet.

I did a first review. haven't tested it yet.
@ -256,6 +257,8 @@ set(VULKAN_SRC
vulkan/vk_texture.hh
vulkan/vk_uniform_buffer.hh
vulkan/vk_vertex_buffer.hh
Member

Empty lines can be removed

Empty lines can be removed
vnapdv marked this conversation as resolved
@ -34,2 +35,4 @@
init_physical_device_limits();
debug::object_vk_label(this, vk_device_, "VkLogicalDevice");
debug::object_vk_label(this, vk_queue_, "VkGraphicsQueue");
Member

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.
vnapdv marked this conversation as resolved
@ -63,0 +116,4 @@
}
}
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
Member

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
vnapdv marked this conversation as resolved
@ -0,0 +32,4 @@
PFN_vkSetDebugUtilsObjectTagEXT vkSetDebugUtilsObjectTagEXT_r = nullptr;
} VKDebuggingTools;
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload);
Member

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
vnapdv marked this conversation as resolved
@ -0,0 +49,4 @@
object_vk_label(ctx, to_vk_object_type(obj), (uint64_t)obj, (const char *)label);
};
void pushMarker(VKContext *ctx, VkCommandBuffer cmd, const char *name);
Member

push_marker / pop_marker / set_marker

`push_marker` / `pop_marker` / `set_marker`
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2023-04-03 15:31:35 +02:00
Kazashi Yoshioka added 2 commits 2023-04-03 15:58:37 +02:00
Kazashi Yoshioka added 1 commit 2023-04-03 16:01:49 +02:00
Kazashi Yoshioka added 1 commit 2023-04-03 16:07:25 +02:00
38c4349f1b FIX:Refactoring Support VulkanObjectLabel,etc
Added naming convention for vulkan objects.
Kazashi Yoshioka added 1 commit 2023-04-03 16:09:01 +02:00
60f47e80dc FIX:Refactoring Support VulkanObjectLabel,etc
Added naming convention for vulkan objects.
Kazashi Yoshioka added 1 commit 2023-04-03 16:15:19 +02:00
995d327954 FIX:Refactoring Support VulkanObjectLabel,etc
Added naming convention(VKNameSUFFIX->name_suffix) for vulkan objects.
Kazashi Yoshioka requested review from Jeroen Bakker 2023-04-03 16:18:43 +02:00
Kazashi Yoshioka added 1 commit 2023-04-12 14:01:05 +02:00
bf8e9db521 Vulkan:DebugUtils:Add const qualifier
Access other than initialization and destruction should be const.
Kazashi Yoshioka added 1 commit 2023-04-12 14:04:58 +02:00
Kazashi Yoshioka reviewed 2023-04-12 14:19:20 +02:00
@ -57,6 +61,7 @@ VKContext::VKContext(void *ghost_window, void *ghost_context)
VKContext::~VKContext()
{
vmaDestroyAllocator(mem_allocator_);
debug::destroy_callbacks(this);
Author
Contributor

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.
Kazashi Yoshioka reviewed 2023-04-12 14:21:24 +02:00
@ -9,1 +9,3 @@
#ifdef _WIN32
# define IMATH_DLL
#endif
Author
Contributor

I added it because an error occurred in the build of windows. I don't know if it's the best.

I added it because an error occurred in the build of windows. I don't know if it's the best.
Member

I will have a check and see for a solution.

I will add the next part to the GPU CMakeLists.txt, similar solution we already use in other locations as well.

if(WIN32)
  if(EXISTS ${LIBDIR}/imath/bin/imath.dll)
    add_definitions(-DIMATH_DLL)
  endif()
endif()

Thanks for pointing this one out. This has been added to the main branch. Next time you sync you can remove these lines.

I will have a check and see for a solution. I will add the next part to the GPU CMakeLists.txt, similar solution we already use in other locations as well. ``` if(WIN32) if(EXISTS ${LIBDIR}/imath/bin/imath.dll) add_definitions(-DIMATH_DLL) endif() endif() ``` Thanks for pointing this one out. This has been added to the main branch. Next time you sync you can remove these lines.
Jeroen Bakker added the
Interest
Vulkan
label 2023-04-17 09:05:40 +02:00
Jeroen Bakker added this to the 3.6 LTS milestone 2023-04-17 09:05:44 +02:00
Jeroen Bakker added this to the EEVEE & Viewport project 2023-04-17 09:05:49 +02:00
Jeroen Bakker requested changes 2023-04-17 09:21:18 +02:00
Jeroen Bakker left a comment
Member

Sore for this late review. I do want to add this to main this week. Think we should do the renames of functions and parameters and I will check if it compiles on all platforms. Hooking this up to GPU_debug_group* can be done when this PR lands in main.

Sore for this late review. I do want to add this to main this week. Think we should do the renames of functions and parameters and I will check if it compiles on all platforms. Hooking this up to `GPU_debug_group*` can be done when this PR lands in main.
@ -57,0 +60,4 @@
namespace blender::gpu::debug {
static void vulkan_dynamic_debug_functions(VKContext *context, PFN_vkGetInstanceProcAddr instload)
Member

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.
@ -57,0 +127,4 @@
}
}
void object_label(VKContext *context, VkObjectType objType, uint64_t obj, const char *name)
Member

obj isn't very useful. would use object_handle

`obj` isn't very useful. would use `object_handle`
@ -57,0 +128,4 @@
}
void object_label(VKContext *context,
VkObjectType objType,
Member

objType -> vk_object_type

`objType` -> `vk_object_type`
@ -57,0 +142,4 @@
}
}
void push_marker(VKContext *context, VkCommandBuffer cmd, const char *name)
Member

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.
@ -57,0 +178,4 @@
}
}
void push_marker(VKContext *context, VkQueue queue, const char *name)
Member

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.
@ -0,0 +36,4 @@
void destroy_callbacks(VKContext *context);
void object_label(VKContext *context, VkObjectType objType, uint64_t obj, const char *name);
template<typename T> void object_label(VKContext *context, T obj, const char *name)
Member

obj -> object_handle

`obj` -> `object_handle`
@ -0,0 +49,4 @@
object_label(context, to_vk_object_type(obj), (uint64_t)obj, (const char *)label);
};
void push_marker(VKContext *context, VkCommandBuffer cmd, const char *name);
Member

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

Also update the definitions to match the parameter names of the implementation.
Kazashi Yoshioka added 2 commits 2023-04-18 05:34:14 +02:00
Kazashi Yoshioka reviewed 2023-04-18 09:46:28 +02:00
@ -146,3 +146,3 @@
/* Construct VBO. */
static GPUVertFormat format = {0};
GPUVertFormat format = {0};
Author
Contributor

The OpenGL test increments the vertex buffer attribute length, causing a subsequent Vulkan test to run-time check failure #2.

The OpenGL test increments the vertex buffer attribute length, causing a subsequent Vulkan test to run-time check failure #2.
Member

You're right. Will change in main branch.

You're right. Will change in main branch.
Kazashi Yoshioka added 1 commit 2023-04-21 11:41:25 +02:00
Kazashi Yoshioka added 1 commit 2023-04-21 11:50:47 +02:00
Jeroen Bakker approved these changes 2023-04-21 12:31:08 +02:00
Jeroen Bakker left a comment
Member

This seems fine for a first commit.

There has been some discussion about ownership of the debugging tools and if we would package our own vulkan loader. For now (development stage) the debugging tools would be part of the context. we can move it to backend after having some feedback during using. This might lead to double messages that we will fix later on.

Vulkan loader will for now just use the user installed one.

This seems fine for a first commit. There has been some discussion about ownership of the debugging tools and if we would package our own vulkan loader. For now (development stage) the debugging tools would be part of the context. we can move it to backend after having some feedback during using. This might lead to double messages that we will fix later on. Vulkan loader will for now just use the user installed one.
Jeroen Bakker merged commit bebb17a973 into main 2023-04-21 12:32:52 +02:00
Kazashi Yoshioka deleted branch vk_debug_update 2023-04-21 12:46:34 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106098
No description provided.