Fix : Added prerequisite checks for using VK_Layer_Validation #105922
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#105922
Loading…
Reference in New Issue
No description provided.
Delete Branch ":vk_debug_break_down"
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?
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.
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.
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.
@ -83,0 +83,4 @@
{
const char *ev_val = getenv("VK_LAYER_PATH");
bool exists = false;
if (ev_val != nullptr) {
Best to change this to an early exit.
@ -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";
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
_
.@ -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());
Not sure we need to open the file. can we use file stats?
@ -83,0 +90,4 @@
}
if (!exists) {
#if defined(_WIN32)
fprintf(stderr,
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?
@ -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__)
Just use #else.
@ -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.");
Seems to be an error.
@ -83,0 +83,4 @@
{
const char *ev_val = getenv("VK_LAYER_PATH");
bool exists = false;
if (ev_val != nullptr) {
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.
@ -83,0 +90,4 @@
}
if (!exists) {
#if defined(_WIN32)
fprintf(stderr,
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
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.
WIP: Fix : Added prerequisite checks for using VK_Layer_Validationto Fix : Added prerequisite checks for using VK_Layer_ValidationI think we are almost there!
@ -24,3 +24,3 @@
#include <cstring>
#include <iostream>
#include "sys/stat.h"
System includes should use
<>
@ -80,3 +80,3 @@
}
}
static bool is_vklayer_exist(const char* vk_extension_config)
vklayer_config_exists
might be a better name.@ -83,0 +85,4 @@
if (ev_val == nullptr) {
return false;
}
const size_t size_max = strlen(ev_val) + strlen(vk_extension_config) + 2;
This section is too C. I would use
std::stringstream
.@ -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")) {
I would combine both functions as these should be called together all the time. Just add the
vk_extension_config
parameter to theenableLayer
function and call thevklayer_config_exists
function from insideenableLayer
.@ -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 {
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.@ -104,1 +118,4 @@
const int MAX_FRAMES_IN_FLIGHT = 2;
enum class VkDynamicLibraryType : std::uint8_t {
VkValidationLayer = 0,
VkDynamicLibraryAll
We should remove this one, until it is actually needed.
@ -409,0 +434,4 @@
return;
}
if (checkLayerSupport(layers_available, "VK_LAYER_KHRONOS_validation") &&
I think you wanted to refer to
layer_name
andconfig_name
here.Think something went wrong with your latest push. Might be EOL character difference.
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>
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.
@ -413,0 +425,4 @@
vklayer_config_exist("VkLayer_khronos_validation.json")) {
layers_enabled.push_back(layer_name);
}
else if (warning) {
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 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.
@blender-bot build