Fix : Added prerequisite checks for using VK_Layer_Validation #105922

Merged
Jeroen Bakker merged 16 commits from :vk_debug_break_down into main 2023-03-28 10:45:56 +02:00
Contributor

This PR is for the GPU Vulkan Module to determine the prerequisites for using the validation layer.

The validation flow of the validation layer that I thought of is as follows.

  • For those who use the validation layer, run blender with the --debug-gpu flag.
  • And vulkanSDK 1.2.198.1 must be downloaded.
  • Additionally, the environment variable VK_LAYER_PATH should be set correctly.
    If the above conditions are not met, the validation layer will not be used, but will still enter runtime.

This is the explicit way to use VK_LAYER_PATH.

vulkanSDK can be placed in any directory.

Should VulkanLayer libraries (parts of VulkanSDK) be Blender Standard 3rd party?

There is no plan, but it would like to do it if there are the opportunities.

The required files are as follows.

VkLayer_khronos_validation.json
VkLayer_khronos_validation.dll(.so)

And the folder containing the files must be set in VK_LAYER_PATH.
It's a way to occupy VK_LAYER_PATH, so it's up to the user to manage.

So the implication of this commit is that if VK_LAYER_PATH is not set, disable its use.

### This PR is for the GPU Vulkan Module to determine the prerequisites for using the validation layer. #### The validation flow of the validation layer that I thought of is as follows. - For those who use the validation layer, run blender with the --debug-gpu flag. - And vulkanSDK 1.2.198.1 must be downloaded. - Additionally, the environment variable VK_LAYER_PATH should be set correctly. If the above conditions are not met, the validation layer will not be used, but will still enter runtime. #### This is the explicit way to use VK_LAYER_PATH. vulkanSDK can be placed in any directory. #### Should VulkanLayer libraries (parts of VulkanSDK) be Blender Standard 3rd party? There is no plan, but it would like to do it if there are the opportunities. The required files are as follows. > VkLayer_khronos_validation.json > VkLayer_khronos_validation.dll(.so) And the folder containing the files must be set in VK_LAYER_PATH. It's a way to occupy VK_LAYER_PATH, so it's up to the user to manage. So the implication of this commit is that if VK_LAYER_PATH is not set, disable its use.
Kazashi Yoshioka added 1 commit 2023-03-20 15:32:25 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-20 15:32:55 +01:00
Jeroen Bakker requested changes 2023-03-21 13:15:51 +01:00
@ -83,0 +83,4 @@
{
const char *ev_val = getenv("VK_LAYER_PATH");
bool exists = false;
if (ev_val != nullptr) {
Member

Best to change this to an early exit.

Best to change this to an early exit.
vnapdv marked this conversation as resolved
@ -83,0 +84,4 @@
const char *ev_val = getenv("VK_LAYER_PATH");
bool exists = false;
if (ev_val != nullptr) {
std::string _json = std::string(ev_val) + "/VkLayer_khronos_validation.json";
Member

The filename should be a parameter. In the future we might want to enable other layers as well. This way it is clear to the developer where to change it.

Not sure concatinating strings is the best solution. would rather use a pathseq

Variables should not start with an _.

The filename should be a parameter. In the future we might want to enable other layers as well. This way it is clear to the developer where to change it. Not sure concatinating strings is the best solution. would rather use a pathseq Variables should not start with an `_`.
vnapdv marked this conversation as resolved
@ -83,0 +85,4 @@
bool exists = false;
if (ev_val != nullptr) {
std::string _json = std::string(ev_val) + "/VkLayer_khronos_validation.json";
std::ifstream infile(_json.c_str());
Member

Not sure we need to open the file. can we use file stats?

Not sure we need to open the file. can we use file stats?
vnapdv marked this conversation as resolved
@ -83,0 +90,4 @@
}
if (!exists) {
#if defined(_WIN32)
fprintf(stderr,
Member

Not sure we should print any message to the user. --debug-gpu is also used by users when reporting bugs. This message will confuse them and ask questions. I rather see this fail silently.

So perhaps we just want to change the messahe in enableLayer (supporter=>available).

But then, what is the value of this PR?

Not sure we should print any message to the user. `--debug-gpu` is also used by users when reporting bugs. This message will confuse them and ask questions. I rather see this fail silently. So perhaps we just want to change the messahe in `enableLayer` (supporter=>available). But then, what is the value of this PR?
vnapdv marked this conversation as resolved
@ -83,0 +95,4 @@
"\n Set the path ..VulkanSDK\1.2.198.1\Bin of VulkanSDK (version1.2.198.1) to the "
"environment variable VK_LAYER_PATH.\nSee more details "
"https://vulkan.lunarg.com/doc/sdk/1.3.239.0/windows/layer_configuration.html.");
#elif !defined(__APPLE__)
Member

Just use #else.

Just use #else.
vnapdv marked this conversation as resolved
@ -83,0 +99,4 @@
fprintf(stderr,
"Warning: Layer requested, we are trying to use the VulkanValidationLayer explicitly. "
"\n Set the path ..vulkan/explicit_layer.d of VulkanSDK (version1.2.198.1) to the "
"environment variable VK_LAYER_PATH.\n"See more details https://vulkan.lunarg.com/doc/sdk/1.3.239.0/linux/layer_configuration.html.");
Member

Seems to be an error.

Seems to be an error.
vnapdv marked this conversation as resolved
Kazashi Yoshioka reviewed 2023-03-21 14:12:49 +01:00
@ -83,0 +83,4 @@
{
const char *ev_val = getenv("VK_LAYER_PATH");
bool exists = false;
if (ev_val != nullptr) {
Author
Contributor

If you return when ev_val is nullptr, you can't print the warnings. It will check if there is no VK_LAYER_PATH and if there is no json file, so it will not return early.

If you return when ev_val is nullptr, you can't print the warnings. It will check if there is no VK_LAYER_PATH and if there is no json file, so it will not return early.
vnapdv marked this conversation as resolved
Kazashi Yoshioka reviewed 2023-03-21 15:44:32 +01:00
@ -83,0 +90,4 @@
}
if (!exists) {
#if defined(_WIN32)
fprintf(stderr,
Author
Contributor

What we check with enableLayer is not whether it's actually ready for dynamic linking.
If the dll or so files are not properly configured, vkCreateInstance will fail.
This is a check to avoid that error.


Vulkan Error : GHOST_ContextVK.cpp:937 : vkCreateInstance(&create_info, 0, &m_instance) failled with VK_ERROR_LAYER_NOT_PRESENT

What we check with `enableLayer` is not whether it's actually ready for dynamic linking. If the dll or so files are not properly configured, vkCreateInstance will fail. This is a check to avoid that error. ----- Vulkan Error : GHOST_ContextVK.cpp:937 : vkCreateInstance(&create_info, 0, &m_instance) failled with VK_ERROR_LAYER_NOT_PRESENT
vnapdv marked this conversation as resolved
Author
Contributor

Current vulkan is 1.3 and blender locks the version to 1.2.
There are situations where it cannot be said that conflicts regarding extensions will not occur, so I think that VK_LAYER_PATH should be used to clarify the policy of dynamic linking.

Current vulkan is 1.3 and blender locks the version to 1.2. There are situations where it cannot be said that conflicts regarding extensions will not occur, so I think that VK_LAYER_PATH should be used to clarify the policy of dynamic linking.
Kazashi Yoshioka added 1 commit 2023-03-21 16:00:34 +01:00
Kazashi Yoshioka added 1 commit 2023-03-21 16:32:12 +01:00
Kazashi Yoshioka changed title from WIP: Fix : Added prerequisite checks for using VK_Layer_Validation to Fix : Added prerequisite checks for using VK_Layer_Validation 2023-03-21 16:33:29 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-21 16:33:35 +01:00
Kazashi Yoshioka added 1 commit 2023-03-21 17:41:57 +01:00
Kazashi Yoshioka added 2 commits 2023-03-23 17:19:29 +01:00
Kazashi Yoshioka added 1 commit 2023-03-23 17:49:37 +01:00
Jeroen Bakker requested changes 2023-03-24 11:59:38 +01:00
Jeroen Bakker left a comment
Member

I think we are almost there!

I think we are almost there!
@ -24,3 +24,3 @@
#include <cstring>
#include <iostream>
#include "sys/stat.h"
Member

System includes should use <>

System includes should use `<>`
vnapdv marked this conversation as resolved
@ -80,3 +80,3 @@
}
}
static bool is_vklayer_exist(const char* vk_extension_config)
Member

vklayer_config_exists might be a better name.

`vklayer_config_exists` might be a better name.
vnapdv marked this conversation as resolved
@ -83,0 +85,4 @@
if (ev_val == nullptr) {
return false;
}
const size_t size_max = strlen(ev_val) + strlen(vk_extension_config) + 2;
Member

This section is too C. I would use std::stringstream.

This section is too C. I would use `std::stringstream`.
vnapdv marked this conversation as resolved
@ -865,3 +881,3 @@
vector<const char *> layers_enabled;
if (m_debug) {
enableLayer(layers_available, layers_enabled, "VK_LAYER_KHRONOS_validation", m_debug);
if (is_vklayer_exist("VkLayer_khronos_validation.json")) {
Member

I would combine both functions as these should be called together all the time. Just add the vk_extension_config parameter to the enableLayer function and call the vklayer_config_exists function from inside enableLayer.

I would combine both functions as these should be called together all the time. Just add the `vk_extension_config` parameter to the `enableLayer` function and call the `vklayer_config_exists` function from inside `enableLayer`.
vnapdv marked this conversation as resolved
Jeroen Bakker added the
Interest
Vulkan
label 2023-03-24 11:59:49 +01:00
Jeroen Bakker added this to the 3.6 LTS milestone 2023-03-24 11:59:55 +01:00
Jeroen Bakker added this to the EEVEE & Viewport project 2023-03-24 12:00:00 +01:00
Kazashi Yoshioka added 1 commit 2023-03-24 13:53:08 +01:00
Jeroen Bakker requested changes 2023-03-24 14:06:42 +01:00
@ -102,6 +116,10 @@ static const char *vulkan_error_as_string(VkResult result)
/* Triple buffering. */
const int MAX_FRAMES_IN_FLIGHT = 2;
enum class VkDynamicLibraryType : std::uint8_t {
Member

Don't specify the size of the enumeration, Let the compiler chose the optimal one for you.

Use a shorter name DynamicLibrary should be enough.

Don't specify the size of the enumeration, Let the compiler chose the optimal one for you. Use a shorter name `DynamicLibrary` should be enough.
vnapdv marked this conversation as resolved
@ -104,1 +118,4 @@
const int MAX_FRAMES_IN_FLIGHT = 2;
enum class VkDynamicLibraryType : std::uint8_t {
VkValidationLayer = 0,
VkDynamicLibraryAll
Member

We should remove this one, until it is actually needed.

We should remove this one, until it is actually needed.
vnapdv marked this conversation as resolved
@ -409,0 +434,4 @@
return;
}
if (checkLayerSupport(layers_available, "VK_LAYER_KHRONOS_validation") &&
Member

I think you wanted to refer to layer_name and config_name here.

I think you wanted to refer to `layer_name` and `config_name` here.
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2023-03-24 14:42:19 +01:00
Kazashi Yoshioka added 1 commit 2023-03-24 14:45:56 +01:00
Member

Think something went wrong with your latest push. Might be EOL character difference.

Think something went wrong with your latest push. Might be EOL character difference.
Kazashi Yoshioka added 1 commit 2023-03-24 15:06:59 +01:00
Kazashi Yoshioka added 1 commit 2023-03-24 15:15:19 +01:00
Kazashi Yoshioka added 1 commit 2023-03-24 15:18:35 +01:00
Kazashi Yoshioka added 1 commit 2023-03-24 15:53:12 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-24 16:13:32 +01:00
Jeroen Bakker requested changes 2023-03-27 08:41:53 +02:00
Jeroen Bakker left a comment
Member

In a previous iteration you used an enum class I do think this might be better than having an API matching strings/names. I prefer that one as that will create better maintainable code.

So the interface of would become
static void enableLayer(vector<VkLayerProperties> &layers_available, vector<const char *> &layers_enabled, const eLayer *layer, const bool display_warning)

The enum eLayer will for now only contain a single item, But is easier to extend as adding a second item will automatically show the places that have to be adapted.

in enableLayer use a switch statement without a default.
keep track with a boolean if a message should be displayed.
Check together with display_warning just before displaying (outside the switch statement).

In a previous iteration you used an `enum class` I do think this might be better than having an API matching strings/names. I prefer that one as that will create better maintainable code. So the interface of would become `static void enableLayer(vector<VkLayerProperties> &layers_available, vector<const char *> &layers_enabled, const eLayer *layer, const bool display_warning)` The enum `eLayer` will for now only contain a single item, But is easier to extend as adding a second item will automatically show the places that have to be adapted. in `enableLayer` use a switch statement without a default. keep track with a boolean if a message should be displayed. Check together with display_warning just before displaying (outside the switch statement).
@ -19,10 +19,12 @@
#include <vector>
#include <sys/stat.h>
Member

Please run make format when push and configure clang-format to your IDE. This file has several issues where it doesn't match our code style.

Please run make format when push and configure clang-format to your IDE. This file has several issues where it doesn't match our code style.
vnapdv marked this conversation as resolved
@ -413,0 +425,4 @@
vklayer_config_exist("VkLayer_khronos_validation.json")) {
layers_enabled.push_back(layer_name);
}
else if (warning) {
Member

I think the pattern you're using here will be different when adding the second extension. You don't want to copy the warning for each extension.

I think the pattern you're using here will be different when adding the second extension. You don't want to copy the warning for each extension.
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2023-03-27 19:48:34 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
69ac0203d4
FIX:Added VkLayerEnum
Kazashi Yoshioka requested review from Jeroen Bakker 2023-03-27 19:54:33 +02:00
Kazashi Yoshioka reviewed 2023-03-27 20:02:23 +02:00
Author
Contributor

Please run make format when push and configure clang-format to your IDE. This file has several issues where it doesn't match our code style.

I checked blender's clang-format. It seems that there was a slight conflict in the settings of visual studio. I will make sure to check again with visual code.

> Please run make format when push and configure clang-format to your IDE. This file has several issues where it doesn't match our code style. I checked blender's clang-format. It seems that there was a slight conflict in the settings of visual studio. I will make sure to check again with visual code.
Member

@blender-bot build

@blender-bot build
Jeroen Bakker approved these changes 2023-03-28 08:52:39 +02:00
Kazashi Yoshioka added 1 commit 2023-03-28 10:20:08 +02:00
Jeroen Bakker merged commit a64877f045 into main 2023-03-28 10:45:56 +02:00
Kazashi Yoshioka deleted branch vk_debug_break_down 2023-03-28 10:46:47 +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#105922
No description provided.