Vulkan: Provide utilities to put markers and labels where needed in the GPUmodule #106098
No reviewers
Labels
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 project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#106098
Loading…
Reference in New Issue
No description provided.
Delete Branch ":vk_debug_update"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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.
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;
One empty line is enough here.
Make sure you run
make format
or have clang-format configured correctly in your IDE.@ -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
Try to keep the list of files sorted. This way it is easier to find missing ones.
@ -16,6 +16,7 @@
#include "vk_mem_alloc.h"
#include "gpu_texture_private.hh"
#include <typeinfo>
Add one line between imports of different categories. Put system includes on top.
@ -28,0 +88,4 @@
BLI_assert_unreachable();
#undef VK_EQ_TYPEID
return VK_OBJECT_TYPE_UNKNOWN;
} // namespace blender::gpu
// namespace blender::gpu
can be removed.@ -5,3 +5,3 @@
* \ingroup gpu
*/
#include "BKE_global.h"
Add empty lines between each category of includes
@ -8,2 +10,4 @@
#include "vk_debug.hh"
#include "vk_backend.hh"
#include "vk_context.hh"
static CLG_LogRef LOG = { "gpu.debug.vulkan" };
Add empty line between includes and code.
@ -63,0 +122,4 @@
bool init_vk_callbacks(VKContext* ctx, PFN_vkGetInstanceProcAddr instload)
{
CLOG_ENSURE(&LOG);
VKDebuggingTools tools = ctx->debuggingtools_get();
/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]
@ -63,0 +129,4 @@
};
return false;
}
void destroy_vk_callbacks(VKContext* ctx)
Add empty line between each function.
Regarding the labeling function of vk_ext_debug_utils, it seems that it does not depend on VK_VALIDATION_LAYER.
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
Empty lines can be removed
@ -34,2 +35,4 @@
init_physical_device_limits();
debug::object_vk_label(this, vk_device_, "VkLogicalDevice");
debug::object_vk_label(this, vk_queue_, "VkGraphicsQueue");
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.
@ -63,0 +116,4 @@
}
}
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload)
use
context
when referring toVKContext
(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 ascommand_buffer
.Abbreviations like
q
should not be used, just call them what they arequeue
.etc
@ -0,0 +32,4 @@
PFN_vkSetDebugUtilsObjectTagEXT vkSetDebugUtilsObjectTagEXT_r = nullptr;
} VKDebuggingTools;
bool init_vk_callbacks(VKContext *ctx, PFN_vkGetInstanceProcAddr instload);
we should remove vk from the function names it is redundant in these cases
@ -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);
push_marker
/pop_marker
/set_marker
@ -57,6 +61,7 @@ VKContext::VKContext(void *ghost_window, void *ghost_context)
VKContext::~VKContext()
{
vmaDestroyAllocator(mem_allocator_);
debug::destroy_callbacks(this);
This break is as a chain on the device. It has no direct relationship with VKContext.
@ -9,1 +9,3 @@
#ifdef _WIN32
# define IMATH_DLL
#endif
I added it because an error occurred in the build of windows. I don't know if it's the best.
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.
Thanks for pointing this one out. This has been added to the main branch. Next time you sync you can remove these lines.
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)
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)
obj
isn't very useful. would useobject_handle
@ -57,0 +128,4 @@
}
void object_label(VKContext *context,
VkObjectType objType,
objType
->vk_object_type
@ -57,0 +142,4 @@
}
}
void push_marker(VKContext *context, VkCommandBuffer cmd, const char *name)
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)
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)
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);
Also update the definitions to match the parameter names of the implementation.
@ -146,3 +146,3 @@
/* Construct VBO. */
static GPUVertFormat format = {0};
GPUVertFormat format = {0};
The OpenGL test increments the vertex buffer attribute length, causing a subsequent Vulkan test to run-time check failure #2.
You're right. Will change in main branch.
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.