blender
/
blender
136
406
Fork 590
Code Issues 6.2k Pull Requests 764 Projects 19 Wiki Activity

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
Conversation 3 Commits 16 Files Changed 1 +35 -20
1 changed files with 35 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit bfb5aede13 - Show all commits

55
intern/ghost/intern/GHOST_ContextVK.cpp
View File

@ -23,7 +23,8 @@
#include <cstdio>
#include <cstring>
#include <iostream>
#include "sys/stat.h"
#include <sstream>
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 11:55:15 +01:00
Outdated
Review
Copy Link

System includes should use <>

System includes should use `<>`
#include <sys/stat.h>
/* Set to 0 to allow devices that do not have the required features.
* This allows development on OSX until we really needs these features. */
#define STRICT_REQUIREMENTS 1
@ -79,21 +80,18 @@ static const char *vulkan_error_as_string(VkResult result)
return "Unknown Error";
}
}
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 11:59:21 +01:00
Outdated
Review
Copy Link

vklayer_config_exists might be a better name.

`vklayer_config_exists` might be a better name.
static bool is_vklayer_exist(const char* vk_extension_config)
static bool vklayer_config_exist(const char* vk_extension_config)
{
const char *ev_val = getenv("VK_LAYER_PATH");
if (ev_val == nullptr) {
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-21 13:09:27 +01:00
Outdated
Review
Copy Link

Best to change this to an early exit.

Best to change this to an early exit.
Kazashi Yoshioka commented 2023-03-21 14:12:32 +01:00
Outdated
Review
Copy Link

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.
return false;
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-21 13:07:36 +01:00
Outdated
Review
Copy Link

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 Outdated
Jeroen Bakker commented 2023-03-21 13:11:32 +01:00
Outdated
Review
Copy Link

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?
Jeroen Bakker commented 2023-03-24 11:53:42 +01:00
Outdated
Review
Copy Link

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

This section is too C. I would use `std::stringstream`.
const size_t size_max = strlen(ev_val) + strlen(vk_extension_config) + 2;
char *filename = (char *)malloc(size_max);
memset(filename, 0, size_max);
strcpy(filename, ev_val);
strcat(filename, "/");
strcat(filename, vk_extension_config);
std::stringstream filename;
filename << ev_val;
filename << "/";
filename << vk_extension_config;
struct stat buffer;
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-21 13:15:46 +01:00
Outdated
Review
Copy Link

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?
Kazashi Yoshioka commented 2023-03-21 15:44:26 +01:00
Outdated
Review
Copy Link

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
bool exists = (stat(filename, &buffer) == 0);
free(filename);
bool exists = (stat(filename.str().c_str(), &buffer) == 0);
return exists;
}
#define __STR(A) "" #A
@ -118,6 +116,10 @@ static bool is_vklayer_exist(const char* vk_extension_config)
/* Triple buffering. */
const int MAX_FRAMES_IN_FLIGHT = 2;
enum class VkDynamicLibraryType : std::uint8_t {
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 14:03:47 +01:00
Outdated
Review
Copy Link

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.
VkValidationLayer = 0,
VkDynamicLibraryAll
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 14:06:27 +01:00
Outdated
Review
Copy Link

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

We should remove this one, until it is actually needed.
};
GHOST_ContextVK::GHOST_ContextVK(bool stereoVisual,
#ifdef _WIN32
@ -417,16 +419,31 @@ static bool checkLayerSupport(vector<VkLayerProperties> &layers_available, const
static void enableLayer(vector<VkLayerProperties> &layers_available,
vector<const char *> &layers_enabled,
const char *layer_name,
const bool debug)
VkDynamicLibraryType library_type,
const bool warning)
{
if (checkLayerSupport(layers_available, layer_name)) {
layers_enabled.push_back(layer_name);
std::string layer_name = "";
std::string config_name = "";
switch (library_type) {
case VkDynamicLibraryType::VkValidationLayer:
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-27 08:35:27 +02:00
Outdated
Review
Copy Link

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.
layer_name = "VK_LAYER_KHRONOS_validation";
config_name = "VkLayer_khronos_validation.json";
break;
case VkDynamicLibraryType::VkDynamicLibraryAll:
default:
return;
}
if (checkLayerSupport(layers_available, "VK_LAYER_KHRONOS_validation") &&
vnapdv marked this conversation as resolved Outdated
Jeroen Bakker commented 2023-03-24 14:02:53 +01:00
Outdated
Review
Copy Link

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.
vklayer_config_exist("VkLayer_khronos_validation.json")) {
layers_enabled.push_back("VK_LAYER_KHRONOS_validation");
}
else if (debug) {
fprintf(
stderr, "Warning: Layer requested, but not supported by the platform. [%s]\n", layer_name);
else if (warning) {
fprintf(stderr,
"Warning: Layer requested, but not supported by the platform. [%s]\n",
layer_name.c_str());
}
}
static bool device_extensions_support(VkPhysicalDevice device, vector<const char *> required_exts)
@ -880,9 +897,7 @@ GHOST_TSuccess GHOST_ContextVK::initializeDrawingContext()
vector<const char *> layers_enabled;
if (m_debug) {
if (is_vklayer_exist("VkLayer_khronos_validation.json")) {
enableLayer(layers_available, layers_enabled, "VK_LAYER_KHRONOS_validation", m_debug);
}
enableLayer(layers_available, layers_enabled, VkDynamicLibraryType::VkValidationLayer,true);
}
vector<const char *> extensions_device;