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

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

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
@SRSaunders
Copy link
Contributor Author

I don't plan to make any more changes here unless there are additional review comments or issues found.

I will submit changes in a separate PR to remove macOS workarounds within the descriptor_indexing sample once improvements are made to MoltenVK in this area (currently in development).

@SaschaWillems SaschaWillems self-requested a review July 14, 2024 17:21
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
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.

None yet

6 participants