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.

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

Add Layer Settings API to framework, improve batch mode error recovery, fix macOS issues #1084

Merged
merged 30 commits into from
Aug 18, 2024

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Jul 2, 2024

Description

This PR provides solutions to #1080, #1081, #1082, and #1083.

  1. Fixes defects in CMakeLists.txt find dxc logic. Supports system-level Vulkan SDK installations that include dxc.
  2. Adds layer settings capability to framework with new add_layer_setting(VkLayerSettingEXT layerSetting), and get_layer_settings() API calls in vulkan_sample.h. Modified Instance() and HPPInstance() parameter lists to include required_layer_settings so they can be activated during instance creation. Protected all changes with #if defined(VK_EXT_layer_settings) for backwards compatibility with Vulkan SDKs earlier than 1.3.272 where the extension may not be defined.
  3. Modified shader_debugprintf sample to override get_validation_layers() and use layer settings to activate the Validation Layer's VK_VALIDATION_FEATURE_ENABLE_DEBUG_PRINTF_EXT feature using the standard framework. The sample's custom create_instance() is now only used if VK_EXT_layer_settings is not available at runtime.
  4. Modified descriptor_indexing sample to use the layer settings API on macOS to activate MoltenVK's MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS feature which is necessary to increase the available descriptors for the sample. Also worked around a macOS/MoltenVK limitation in the variable descriptor count feature. This may be fixed in future versions of MoltenVK, but the workaround will function for past, current and future MoltenVK versions.
  5. Enabled VK_ENABLE_BETA_EXTENSIONS on Apple platforms for access to the VK_KHR_portability_subset extension and features. Activating VK_KHR_portability_subset is required on all portability implementations where the extension is defined and available. Also activated the mutableComparisonSamplers portability feature for the async_compute and multithreading_render_pass samples.
  6. Improved batch mode error recovery by: a) using throw std::runtime_error() vs. ExitCode::FatalError in main_loop() and main_loop_frame(), b) modifying VK_CHECK to use throw std::runtime_error() to gracefully recover from more subtle issues such as pipeline creation problems (e.g. SPIR-V to Metal cross-compilation errors due to feature limitations) as well as queue submission errors, c) resetting the shader compiler's target environment settings prior to each sample to avoid stickiness from earlier samples in batch mode, d) allowing VK_SUBOPTIMAL_KHR which avoids early termination of some samples when exiting while in batch mode, and e) fixing specific issues in async_compute, texture_mipmap_generation, and fragment_shader_barycentric samples to permit graceful failure and continuation while in batch mode.
  7. Changed packaging to build a command line macOS executable (not an app bundle) to match the existing online build/run instructions on the web site (i.e. use ./build/mac/app/bin/Release/<x86_64|arm64>/vulkan_samples vs. ./build/mac/app/bin/Release/<x86_64|arm64>/vulkan_samples.app/Contents/MacOS/vulkan_samples). App bundles are needed for iOS, but get in the way on macOS for command line applications like vulkan_samples.
  8. Added .DS_Store to the .gitignore file for git cleanliness on Apple.

I have other improvements pending for Xcode project generation for macOS/iOS, running on iOS and the iOS simulator (x86_64 and arm64 hosts), and standalone MoltenVK support (i.e. without the vulkan loader, useful when working on the MoltenVK dev stream), but will submit those in a separate PR.

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

I have tested using batch runs on Windows and macOS. Depending on CI for now on linux - will test later Batch mode testing on Manjaro now complete. Cannot test on Android - don't have the environment. I have also tested on iOS but have additional changes to add in a separate PR.

If this PR contains framework changes:

  • I did a full batch run using the batch command line argument to make sure all samples still work properly

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR. Very much appreciated 👍🏻

I did a quick first look, but with so many changes crammed into one PR it may take some time to properly review this ;)

Though I won't be able to test any Mac OS / iOS related changes due to a lack of such hardware.

CMakeLists.txt Outdated Show resolved Hide resolved
framework/common/error.h Show resolved Hide resolved
samples/extensions/shader_debugprintf/CMakeLists.txt Outdated Show resolved Hide resolved
samples/performance/async_compute/async_compute.cpp Outdated Show resolved Hide resolved
@SRSaunders
Copy link
Contributor Author

SRSaunders commented Jul 4, 2024

I added commit 7c0195a to fix the Doxygen and Copyright date issues found by CI.

I am not sure what to do with clang-issues.diff as it does not appear to be properly formatted git patch file.

I have also added commit 8caca90 to fix clang-format issues found by CI.

@SRSaunders
Copy link
Contributor Author

The remaing CI issues (Doxygen, clang-format) appear to be upstream issues, not related to this PR.

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

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

The VulkanSample class is one of the few classes (by now) that are unified with regards to C- and C++-bindings of vulkan. To keep it consistent, some changes to your PR would be needed, that you can find in the attached patch.
0001-Adjust-LayerSetting-handling-in-VulkanSample-class.patch

framework/core/hpp_instance.cpp Outdated Show resolved Hide resolved
framework/core/hpp_instance.cpp Outdated Show resolved Hide resolved
framework/core/instance.cpp Outdated Show resolved Hide resolved
samples/extensions/shader_debugprintf/CMakeLists.txt Outdated Show resolved Hide resolved
samples/performance/async_compute/async_compute.cpp Outdated Show resolved Hide resolved
samples/performance/pipeline_cache/pipeline_cache.cpp Outdated Show resolved Hide resolved
samples/performance/pipeline_cache/pipeline_cache.cpp Outdated Show resolved Hide resolved
asuessenbach
asuessenbach previously approved these changes Jul 15, 2024
@SRSaunders
Copy link
Contributor Author

Hopefully all issues raised by @asuessenbach and @SaschaWillems are now resolved. Again, I don't plan any more updates and re-review of recent changes / resolutions can now proceed.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
app/CMakeLists.txt Show resolved Hide resolved
bldsys/cmake/global_options.cmake Show resolved Hide resolved
@SRSaunders
Copy link
Contributor Author

@SaschaWillems are there any further changes needed to proceed with this PR? I have tried to respond to your review comments with resolutions but have not heard back. If there is anything else please let me know. I would like to submit some iOS fixes and Xcode improvements (in a new PR) that are dependent on this one. Thanks.

@SaschaWillems
Copy link
Collaborator

Two things: I'm currently lacking time for PRs and I don't own any Apple device so I can't actually test if this works. We discussed this on our recent call (how we'll review Apple related PRs in the future), so hopefully someone else can green light this.

@SRSaunders
Copy link
Contributor Author

We discussed this on our recent call (how we'll review Apple related PRs in the future), so hopefully someone else can green light this.

Given your constraints, if you could nominate or assign an Apple-knowledgeable reviewer that would be great. Thanks.

@SaschaWillems
Copy link
Collaborator

We are already working on that and hopefully have another official Mac reviewer soon ;)

Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

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

It compiles and works fine for me (did a full batch run) on Win 11 with an RT 4070, and I'm fine with the code changes. I don't want to block this, so I'll approve it from a code reviewers point-of-view.

@SRSaunders
Copy link
Contributor Author

Thanks @SaschaWillems for approving.

After this is merged, I may update the logic for picking the API version for shader_debugprintf. Given that the VVL is now fixed and will be picked up in the next SDK, the API version should be set to 1.1 starting with the next SDK release. I can add a bit of logic to detect this at runtime and dynamically set the correct API version based on the running SDK instance. I would put this change in a supplementary PR. What do you think?

@gpx1000
Copy link
Collaborator

gpx1000 commented Aug 18, 2024

@marty-johnson59 ready to merge here.

@SRSaunders we typically try to be careful about updating the API so older versions which might still be out there in the wild continue to work. However, I certainly am on board with considering an update. Ideally we try to keep it to just the iPhone or Mac platforms.
Lemme know if you need any help with it. I do Mac development and can be reliable for any help you might need.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Aug 18, 2024

@gpx1000 thanks for approving the merge.

Regarding the shader_debugprintf sample, there has been a platform-independent bug in the Vulkan Validation Layers that manifests itself as an-almost unusable performance issue when using the debugPrintfEXT feature with the correct Vulkan API 1.1 setting. This happens on all platforms, not just macOS. See defect KhronosGroup/Vulkan-ValidationLayers#7562. This PR changes the sample to Vulkan API 1.0 as a temporary workaround, but it should be moved back to Vulkan 1.1 when the fixed VVL ships, which should be in the next SDK version. I was just offering to make a follow-on change to this PR that recognizes this and allows the shader_debugprintf sample code to adapt to the SDK instance it is running. Note the VVL fix came in after this PR was frozen for review so I did not change it here.

Your offer to assist with macOS issues is very welcome - thank you! In fact, I do have a pending fix that I would like to get feedback on - relating to swapchain recreation with MoltenVK. I will open a separate issue and ask for your feedback there. Thanks again.

@marty-johnson59 marty-johnson59 merged commit fdce530 into KhronosGroup:main Aug 18, 2024
23 checks passed
@JoseEmilio-ARM
Copy link
Collaborator

Hi, I missed this PR, but recently I needed to test the settings extension and I have been trying to use the helper function add_layer_setting from here (thanks for this!), and I think I might have found a bug.

I was trying to enable the best practice validation, by simply adding these lines to the constructor of a different sample:

	const char*    layer_name = "VK_LAYER_KHRONOS_validation";
	const VkBool32 value      = VK_TRUE;

	add_layer_setting({layer_name, "validate_best_practices", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &value});

However, to make it work, I had to remove the requirement that the VK_EXT_layer_settings is enabled as an Instance extension. As it has been discussed here before, this is actually not a requirement, since the extension is implemented by the validation layer itself.

If I remove these lines from instance.cpp, then the settings work and I can see the validation warnings as expected:

--- a/framework/core/instance.cpp
+++ b/framework/core/instance.cpp
@@ -383,13 +383,14 @@ Instance::Instance(const std::string                            &application_nam
        VkLayerSettingsCreateInfoEXT layerSettingsCreateInfo{VK_STRUCTURE_TYPE_LAYER_SETTINGS_CREATE_INFO_EXT};

        // If layer settings extension enabled by sample, then activate layer settings during instance creation
-       if (std::find(enabled_extensions.begin(), enabled_extensions.end(), VK_EXT_LAYER_SETTINGS_EXTENSION_NAME) != enabled_extensions.end())
-       {
                layerSettingsCreateInfo.settingCount = static_cast<uint32_t>(required_layer_settings.size());
                layerSettingsCreateInfo.pSettings    = required_layer_settings.data();
                layerSettingsCreateInfo.pNext        = instance_info.pNext;
                instance_info.pNext                  = &layerSettingsCreateInfo;
-       }

Enabling the extension from the sample as suggested in the comment above does not work for me, as the instance keeps reporting the extension as not supported.

(Update: is this the same bug as the one being addressed in #1187 ?)

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Nov 18, 2024

However, to make it work, I had to remove the requirement that the VK_EXT_layer_settings is enabled as an Instance extension. As it has been discussed here before, this is actually not a requirement, since the extension is implemented by the validation layer itself.

@JoseEmilio-ARM you are completely correct. In the process of solving the issues in #1187, and with the help of VVL engineers, I came to realize that the VK_EXT_layer_settings extension is typically provided by the layer, and only sometimes provided by the driver (e.g. MoltenVK). As a result I made a change in that PR to do the following in instance.cpp and hpp_instance.cpp:

// If layer settings are defined, then activate the sample's required layer settings during instance creation
if (required_layer_settings.size() > 0)
{
	layerSettingsCreateInfo.settingCount = static_cast<uint32_t>(required_layer_settings.size());
	layerSettingsCreateInfo.pSettings    = required_layer_settings.data();
	layerSettingsCreateInfo.pNext        = instance_info.pNext;
	instance_info.pNext                  = &layerSettingsCreateInfo;
}

If you could retest after applying #1187 that would be great (note I have just merged in main to correct a recent conflict caused by changes to headless mode). Please let me know if this works for you.

Hopefully this PR can be reviewed and merged relatively soon, as it fixes this general layer settings issue as well as the shader_debugprintf sample.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants