Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vk ext shader object optional layer #780

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Vk ext shader object optional layer #780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
89d6fe8
Add VK_EXT_shader_object sample
Apr 17, 2023
c76194e
add an ability to have optional vulkan layers. If it's an optional l…
gpx1000 Aug 17, 2023
2b0356d
Merge remote-tracking branch 'upstream/main'
gpx1000 Aug 17, 2023
53482ab
Merge branch 'main' into VK_EXT_shader_object_optional_layer
gpx1000 Aug 17, 2023
a86f7d2
fix the formatting and copyright breaks.
gpx1000 Aug 17, 2023
fb01dcf
get full_screen_exclusive to build.
gpx1000 Aug 17, 2023
33bdc15
clang format locally wants it formatted differently than CI.
gpx1000 Aug 17, 2023
ad8e78c
if there's an error in clang-format then save off the diff output so …
gpx1000 Aug 17, 2023
9290b5f
attempt to fix clang-format
gpx1000 Aug 17, 2023
d1cb82d
Merge remote-tracking branch 'refs/remotes/upstream/main' into VK_EXT…
gpx1000 May 16, 2024
892f289
Refactor validation layers to use vector instead of map
gpx1000 May 16, 2024
48d8b14
revert the change in check.yml
gpx1000 May 16, 2024
e92bdb0
fix the copyright and formating and doxygen problems.
gpx1000 May 16, 2024
3b73db8
revert vulkan and volk submodule changes.
gpx1000 May 17, 2024
d2b132c
clang format
gpx1000 May 17, 2024
27e35ba
update the clang-format
gpx1000 May 17, 2024
709d7de
revert full_screen_exclusive.cpp and hello_triangle.cpp
gpx1000 May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions framework/core/hpp_instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,41 @@ static VKAPI_ATTR VkBool32 VKAPI_CALL debug_callback(VkDebugReportFlagsEXT flags
}
#endif

bool validate_layers(std::unordered_map<const char *, bool> &required,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required layers here actually are not required (they're allowed to be optional), but requested. Maybe rename the argument?

const std::vector<vk::LayerProperties> &available)
{
std::vector<const char *> remove_vec;
for (auto layer : required)
{
bool found = false;
for (auto &available_layer : available)
{
if (strcmp(available_layer.layerName, layer.first) == 0)
{
found = true;
break;
}
}

if (!found)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can replace the loop over the availables with this check:
if (std::none_of(available.begin(), available.end(), [&layer](vk::LayerProperties const &lp) { return lp.layerName == layer.first; }))

{
if (!layer.second)
{
LOGW("Optional Layer {} not found, removing it", layer.first)
remove_vec.push_back(layer.first);
continue;
}
LOGE("Validation Layer {} not found", layer.first)
return false;
}
}
for (auto &rem : remove_vec)
{
required.erase(rem);
}
return true;
}

bool validate_layers(const std::vector<const char *> &required,
const std::vector<vk::LayerProperties> &available)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case, you want to change a little more, this version of validate_layers could look like this:

	auto requiredButNotAvailableIt =
	    std::find_if(required.begin(),
	                 required.end(),
	                 [&available](char const *layer) {
		                 return std::none_of(available.begin(), available.end(), [&layer](vk::LayerProperties const &lp) { return lp.layerName == layer; });
	                 });
	if (requiredButNotAvailableIt != required.end())
	{
		LOGE("Validation Layer {} not found", *requiredButNotAvailableIt);
		return false;
	}
	return true;

Expand Down Expand Up @@ -186,6 +221,7 @@ bool enable_all_extensions(const std::vector<const char *> required_
HPPInstance::HPPInstance(const std::string &application_name,
const std::unordered_map<const char *, bool> &required_extensions,
const std::vector<const char *> &required_validation_layers,
const std::unordered_map<const char *, bool> &requested_layers,
bool headless,
uint32_t api_version)
{
Expand Down Expand Up @@ -299,6 +335,20 @@ HPPInstance::HPPInstance(const std::string &applicati
throw std::runtime_error("Required validation layers are missing.");
}

std::unordered_map<const char *, bool> layers = (std::unordered_map<const char *, bool>) (requested_layers);
Copy link
Contributor

@asuessenbach asuessenbach Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.
Never cast the const away.
If you would handle the requested_layers the same way as the required_extensions (which should be named requested_extensions!), you would not even need the new validate_layers version.
And, by the way, the layers are never used here!

if (validate_layers(layers, supported_validation_layers))
{
LOGI("Enabled Validation Layers:")
for (const auto &layer : layers)
{
LOGI(" \t{}", layer.first);
}
}
else
{
throw std::runtime_error("Required validation layers are missing.");
}

vk::ApplicationInfo app_info(application_name.c_str(), 0, "Vulkan Samples", 0, api_version);

vk::InstanceCreateInfo instance_info({}, &app_info, requested_validation_layers, enabled_extensions);
Expand Down
4 changes: 3 additions & 1 deletion framework/core/hpp_instance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2022-2023, NVIDIA CORPORATION. All rights reserved.
/* Copyright (c) 2022-2024, NVIDIA CORPORATION. All rights reserved.
*
* SPDX-License-Identifier: Apache-2.0
*
Expand Down Expand Up @@ -53,13 +53,15 @@ class HPPInstance
* @param application_name The name of the application
* @param required_extensions The extensions requested to be enabled
* @param required_validation_layers The validation layers to be enabled
* @param requested_layers The layers that are requested to be enabled (second parameter in unordered map is if required).
* @param headless Whether the application is requesting a headless setup or not
* @param api_version The Vulkan API version that the instance will be using
* @throws runtime_error if the required extensions and validation layers are not found
*/
HPPInstance(const std::string &application_name,
const std::unordered_map<const char *, bool> &required_extensions = {},
const std::vector<const char *> &required_validation_layers = {},
const std::unordered_map<const char *, bool> &requested_layers = {},
bool headless = false,
uint32_t api_version = VK_API_VERSION_1_0);

Expand Down
50 changes: 50 additions & 0 deletions framework/core/instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,41 @@ static VKAPI_ATTR VkBool32 VKAPI_CALL debug_callback(VkDebugReportFlagsEXT flags
}
#endif

bool validate_layers(std::unordered_map<const char *, bool> &required,
const std::vector<VkLayerProperties> &available)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comments as for hpp_instance.cpp hold here.

{
std::vector<const char *> remove_vec;
for (auto layer : required)
{
bool found = false;
for (auto &available_layer : available)
{
if (strcmp(available_layer.layerName, layer.first) == 0)
{
found = true;
break;
}
}

if (!found)
{
if (!layer.second)
{
LOGW("Optional Layer {} not found, removing it", layer.first)
remove_vec.push_back(layer.first);
continue;
}
LOGE("Validation Layer {} not found", layer.first)
return false;
}
}
for (auto &rem : remove_vec)
{
required.erase(rem);
}
return true;
}

bool validate_layers(const std::vector<const char *> &required,
const std::vector<VkLayerProperties> &available)
{
Expand Down Expand Up @@ -186,6 +221,7 @@ bool enable_all_extensions(const std::vector<const char *> required_ex
Instance::Instance(const std::string &application_name,
const std::unordered_map<const char *, bool> &required_extensions,
const std::vector<const char *> &required_validation_layers,
const std::unordered_map<const char *, bool> &requested_layers,
bool headless,
uint32_t api_version)
{
Expand Down Expand Up @@ -311,6 +347,20 @@ Instance::Instance(const std::string &application_nam
throw std::runtime_error("Required validation layers are missing.");
}

std::unordered_map<const char *, bool> layers = (std::unordered_map<const char *, bool>) (requested_layers);
if (validate_layers(layers, supported_validation_layers))
{
LOGI("Enabled Validation Layers:")
for (const auto &layer : layers)
{
LOGI(" \t{}", layer.first);
}
}
else
{
throw std::runtime_error("Required validation layers are missing.");
}

VkApplicationInfo app_info{VK_STRUCTURE_TYPE_APPLICATION_INFO};

app_info.pApplicationName = application_name.c_str();
Expand Down
2 changes: 2 additions & 0 deletions framework/core/instance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,15 @@ class Instance
* @param application_name The name of the application
* @param required_extensions The extensions requested to be enabled
* @param required_validation_layers The validation layers to be enabled
* @param requested_layers The layers that are requested to be enabled (second parameter in unordered map is if required).
* @param headless Whether the application is requesting a headless setup or not
* @param api_version The Vulkan API version that the instance will be using
* @throws runtime_error if the required extensions and validation layers are not found
*/
Instance(const std::string &application_name,
const std::unordered_map<const char *, bool> &required_extensions = {},
const std::vector<const char *> &required_validation_layers = {},
const std::unordered_map<const char *, bool> &requested_layers = {},
bool headless = false,
uint32_t api_version = VK_API_VERSION_1_0);

Expand Down
25 changes: 21 additions & 4 deletions framework/vulkan_sample.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include "scene_graph/scripts/animation.h"

#if defined(PLATFORM__MACOS)
#include <TargetConditionals.h>
# include <TargetConditionals.h>
#endif

namespace vkb
Expand Down Expand Up @@ -250,6 +250,8 @@ class VulkanSample : public vkb::Application
*/
void add_instance_extension(const char *extension, bool optional = false);

void add_layer(const char *layer, bool optional = false);

void create_gui(const Window &window, StatsType const *stats = nullptr, const float font_size = 21.0f, bool explicit_update = false);

/**
Expand Down Expand Up @@ -358,6 +360,8 @@ class VulkanSample : public vkb::Application
*/
std::unordered_map<const char *, bool> const &get_instance_extensions() const;

std::unordered_map<const char *, bool> const &get_layers() const;

/// <summary>
/// PRIVATE MEMBERS
/// </summary>
Expand Down Expand Up @@ -415,6 +419,7 @@ class VulkanSample : public vkb::Application

/** @brief Set of instance extensions to be enabled for this example and whether they are optional (must be set in the derived constructor) */
std::unordered_map<const char *, bool> instance_extensions;
std::unordered_map<const char *, bool> instance_layers;

/** @brief The Vulkan API version to request for this sample at instance creation time */
uint32_t api_version = VK_API_VERSION_1_0;
Expand Down Expand Up @@ -459,6 +464,12 @@ inline void VulkanSample<bindingType>::add_instance_extension(const char *extens
instance_extensions[extension] = optional;
}

template <vkb::BindingType bindingType>
inline void VulkanSample<bindingType>::add_layer(const char *layer, bool optional)
{
instance_layers[layer] = optional;
}

template <vkb::BindingType bindingType>
inline std::unique_ptr<typename VulkanSample<bindingType>::DeviceType> VulkanSample<bindingType>::create_device(PhysicalDeviceType &gpu)
{
Expand All @@ -478,7 +489,7 @@ inline std::unique_ptr<typename VulkanSample<bindingType>::DeviceType> VulkanSam
template <vkb::BindingType bindingType>
inline std::unique_ptr<typename VulkanSample<bindingType>::InstanceType> VulkanSample<bindingType>::create_instance(bool headless)
{
return std::make_unique<InstanceType>(get_name(), get_instance_extensions(), get_validation_layers(), headless, api_version);
return std::make_unique<InstanceType>(get_name(), get_instance_extensions(), get_validation_layers(), get_layers(), headless, api_version);
}

template <vkb::BindingType bindingType>
Expand Down Expand Up @@ -764,6 +775,12 @@ inline std::unordered_map<const char *, bool> const &VulkanSample<bindingType>::
return instance_extensions;
}

template <vkb::BindingType bindingType>
inline std::unordered_map<const char *, bool> const &VulkanSample<bindingType>::get_layers() const
{
return instance_layers;
}

template <vkb::BindingType bindingType>
inline typename VulkanSample<bindingType>::RenderContextType const &VulkanSample<bindingType>::get_render_context() const
{
Expand Down Expand Up @@ -957,9 +974,9 @@ inline bool VulkanSample<bindingType>::prepare(const ApplicationOptions &options

// initialize C++-Bindings default dispatcher, first step
#if TARGET_OS_IPHONE
static vk::DynamicLoader dl("vulkan.framework/vulkan");
static vk::DynamicLoader dl("vulkan.framework/vulkan");
#else
static vk::DynamicLoader dl;
static vk::DynamicLoader dl;
#endif
VULKAN_HPP_DEFAULT_DISPATCHER.init(dl.getProcAddress<PFN_vkGetInstanceProcAddr>("vkGetInstanceProcAddr"));

Expand Down
8 changes: 4 additions & 4 deletions samples/api/hello_triangle/hello_triangle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ void HelloTriangle::init_instance(Context &context,
{
LOGI("Initializing vulkan instance.");

if (volkInitialize())
{
throw std::runtime_error("Failed to initialize volk.");
}
if (volkInitialize())
{
throw std::runtime_error("Failed to initialize volk.");
}

uint32_t instance_extension_count;
VK_CHECK(vkEnumerateInstanceExtensionProperties(nullptr, &instance_extension_count, nullptr));
Expand Down
10 changes: 6 additions & 4 deletions samples/extensions/shader_object/README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
////
- Copyright 2023 Nintendo
- Copyright 2024 Nintendo
-
- Licensed under the Apache License, Version 2.0 (the "License");
- you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -436,13 +436,15 @@ The layer can be shipped with your application, and it will disable itself if a
The emulation layer can be enabled by adding `VK_LAYER_KHRONOS_shader_object` to `ppEnabledLayerNames` in `VkDeviceCreateInfo`.

The sample framework already has an existing abstraction normally used for enabling the validation layer.
This sample repurposes this mechanism to instead load the emulation layer.
This
Copy link
Contributor

@asuessenbach asuessenbach Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline here?

Besides that, is that description still valid? We now have some add_layer and get_layers. None of them explicitly mentions validation layers.

sample repurposes this mechanism to instead load the emulation layer. NB: the layer is set to be optional as that's
what the false does.

[,CPP]
----
const std::vector<const char *> ShaderObject::get_validation_layers()
const std::unordered_map<const char *, bool> ShaderObject::get_validation_layers()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no ShaderObject::get_validation_layers anymore.

{
return {"VK_LAYER_KHRONOS_shader_object"};
return {"VK_LAYER_KHRONOS_shader_object", false};
}
----

Expand Down
21 changes: 13 additions & 8 deletions samples/extensions/shader_object/shader_object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ ShaderObject::ShaderObject()

// Enable extensions for sample
add_device_extension(VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME);

// Enable the shader object layer if it's available. Its optional.
add_layer("VK_LAYER_KHRONOS_shader_object", false);
}

ShaderObject::~ShaderObject()
Expand Down Expand Up @@ -105,13 +108,6 @@ ShaderObject::~ShaderObject()
}
}

// Currently the sample calls through this function in order to get the list of any instance layers, not just validation layers.
// This is not suitable for a real application implementation using the layer, the layer will need to be shipped with the application.
const std::vector<const char *> ShaderObject::get_validation_layers()
{
return {"VK_LAYER_KHRONOS_shader_object"};
}

bool ShaderObject::resize(const uint32_t _width, const uint32_t _height)
{
if (!has_device())
Expand Down Expand Up @@ -229,6 +225,11 @@ void ShaderObject::setup_framebuffer()
// Create render pass for UI drawing
void ShaderObject::setup_render_pass()
{
// delete existing render pass
if (render_pass != VK_NULL_HANDLE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setup_render_pass called multiple times?
If so, you would need to initialize render_pass with VK_NULL_HANDLE to make this check work in debug mode. If not, you don't need to destroy the render pass here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @asuessenbach , per discussion today, can you open an issue so we don't lose the context? We're going to merge this one soon w/o capturing this - but want to circle back later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed the meeting where this was discussed...
Why should this issue not be resolved right here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to fix this after this PR has been merged. The PR is kinda urgent, and the line you highlighted is not new to the PR but already present in the base sample.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SaschaWillems so it seems, you know about the issue to be resolved later. Would you be so kind and file that issue, instead of me guessing what's meant to be done here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was under the impression that this change was already present beforehand. But as this code is new to this PR you're right to mention this and it indeed needs fixing in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this change because renderpass was being created by the vulkansample in the framework automatically. I can take this change out, but then validation layers correctly identifies a render_pass object not getting released. The rest of the changes are updated to conform with your requests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. We should keep that change then :)

{
vkDestroyRenderPass(get_device().get_handle(), render_pass, VK_NULL_HANDLE);
}
VkAttachmentDescription color_attachment{};

// Color attachment set to load color and ignore stencil
Expand Down Expand Up @@ -414,6 +415,10 @@ void ShaderObject::load_assets()

VkSamplerCreateInfo sampler_create_info = vkb::initializers::sampler_create_info();

// destroy created sampler before re-creating
vkDestroySampler(get_device().get_handle(), heightmap_texture.sampler, nullptr);
vkDestroySampler(get_device().get_handle(), terrain_array_textures.sampler, nullptr);

// Setup a mirroring sampler for the height map
vkDestroySampler(get_device().get_handle(), heightmap_texture.sampler, nullptr);
sampler_create_info.magFilter = filter;
Expand Down Expand Up @@ -1950,7 +1955,7 @@ void ShaderObject::build_linked_shaders(VkDevice device, ShaderObject::Shader *v
shader_create.flags |= VK_SHADER_CREATE_LINK_STAGE_BIT_EXT;
}

VkShaderEXT shaderEXTs[2];
VkShaderEXT shaderEXTs[2] = {VK_NULL_HANDLE, VK_NULL_HANDLE};

// Create the shader objects
VkResult result = vkCreateShadersEXT(device,
Expand Down
2 changes: 0 additions & 2 deletions samples/extensions/shader_object/shader_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,6 @@ class ShaderObject : public ApiVulkanSample
ShaderObject();
~ShaderObject() override;

const std::vector<const char *> get_validation_layers() override;

bool resize(const uint32_t width, const uint32_t height) override;
bool prepare(const vkb::ApplicationOptions &options) override;
void setup_framebuffer() override;
Expand Down
Loading