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 new sample for shader debugprintf #945

Merged
merged 25 commits into from
Mar 25, 2024

Conversation

SaschaWillems
Copy link
Collaborator

@SaschaWillems SaschaWillems commented Feb 25, 2024

Description

This PR adds a new sample showing how to use debugprintf in shaders. It also includes a tutorial.

Note: This also does some changes to the framework required for samples that do custom instance creation.

Tested on Windows 11 with an NVIDIA RTX 4070

Fixes #926
Fixes #985

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

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

  • I have tested the sample on at least one compliant Vulkan implementation
  • If the sample is vendor-specific, I have tagged it appropriately
  • I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
  • Any dependent assets have been merged and published in downstream modules
  • For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
  • For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
  • For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

@SaschaWillems SaschaWillems marked this pull request as draft February 25, 2024 16:30
@SaschaWillems SaschaWillems added the sample This is relevant to a sample label Feb 25, 2024
Copy link
Contributor

@gary-sweet gary-sweet left a comment

Choose a reason for hiding this comment

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

We don't support debugPrintfEXT but I'm still seeing a couple of issues when I run this PR.

I get a validation error at the start:
VUID-vkCreateInstance-ppEnabledExtensionNames-01388(ERROR / SPEC): msgNum: -437968512 - Validation Error: [ VUID-vkCreateInstance-ppEnabledExtensionNames-01388 ] | MessageID = 0xe5e52180 | vkCreateInstance(): pCreateInfo->ppEnabledExtensionNames[0] Missing extension required by the instance extension VK_KHR_display: VK_KHR_surface. The Vulkan spec states: All required extensions for each extension in the VkInstanceCreateInfo::ppEnabledExtensionNames list must also be present in that list (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkCreateInstance-ppEnabledExtensionNames-01388)

It terminates with an error about VK_KHR_get_physical_device_properties2:

[error] [framework/platform/platform.cpp:169] Error Message: Couldn't request feature from device as VK_KHR_get_physical_device_properties2 isn't enabled!
[error] [framework/platform/platform.cpp:170] Failed when running application shader_debugprintf
Segmentation fault

and then segfaults in vkDestroySurfaceKHR() called from VulkanSample::~VulkanSample()

@SaschaWillems
Copy link
Collaborator Author

Thanks for noticing. The framework seems to add VK_KHR_surface explicitly and via glfw (which looks wrong on e.g. Windows as it shows up twice). I guess on your platform the implicit addition via glfw doesn't happen, hence it's missing. Will fix that.

@SaschaWillems SaschaWillems marked this pull request as ready for review March 6, 2024 19:34
@SaschaWillems
Copy link
Collaborator Author

I have added a tutorial for this sample. It's now ready for review.

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.

Great description on how to use it.

For some unknown reason, this sample "runs" incredibly slow on my machine (multiple seconds per frame). But when the Vulkan Configurator is running as well, I don't get any output anymore and the speed is as expected. Any idea on that?

samples/extensions/shader_debugprintf/README.adoc Outdated Show resolved Hide resolved
samples/extensions/shader_debugprintf/README.adoc Outdated Show resolved Hide resolved
samples/extensions/shader_debugprintf/README.adoc Outdated Show resolved Hide resolved
samples/extensions/shader_debugprintf/README.adoc Outdated Show resolved Hide resolved
samples/extensions/shader_debugprintf/README.adoc Outdated Show resolved Hide resolved
@SaschaWillems
Copy link
Collaborator Author

Great description on how to use it.

For some unknown reason, this sample "runs" incredibly slow on my machine (multiple seconds per frame). But when the Vulkan Configurator is running as well, I don't get any output anymore and the speed is as expected. Any idea on that?

That seems to be a bug with the validation layers. I noticed this too, but couldn't reproduce :(

@gary-sweet
Copy link
Contributor

This no longer crashes for me, but I'm still seeing:

[error] [framework/platform/platform.cpp:175] Error Message: Couldn't request feature from device as VK_KHR_get_physical_device_properties2 isn't enabled!
[error] [framework/platform/platform.cpp:176] Failed when running application shader_debugprintf
    71:05:42.547 nxserverlib: Vulkan Display(0x7e5c000ee0) unregistered

@SaschaWillems
Copy link
Collaborator Author

Thank you very much for testing :)

I guess that error is caused by #938

Currently the framework will always enable all features of an extension at the time it's requested. I'll see if I can do a workaround for that.

@asuessenbach
Copy link
Contributor

I think, @gary-sweet's issue here is, that VK_KHR_get_physical_device_properties2 is not enabled (is it supported, at all?), even though it has been added in ShaderDebugPrintf::create_instance! Should have failed there, then.

@gary-sweet
Copy link
Contributor

I think, @gary-sweet's issue here is, that VK_KHR_get_physical_device_properties2 is not enabled (is it supported, at all?), even though it has been added in ShaderDebugPrintf::create_instance! Should have failed there, then.

We do support VK_KHR_get_physical_device_properties2, so it's not obvious what the issue is. I'll do some degugging.

@gary-sweet
Copy link
Contributor

I'll do some debugging.

It looks like this sample is creating its own VkInstance which it then constructs the Instance() object from. This appears to bypass a bunch of the extension enabling that happens in the Instance main constructor (all the enable_extension() stuff). The end result is that the Instance object doesn't think the extension is enabled, even though it actually is in the contained VkInstance.

@SaschaWillems
Copy link
Collaborator Author

SaschaWillems commented Mar 11, 2024

Yeah, it's similar to the profiles example. The framework assumes some stuff, even if you create your own instance, which in turn causes problems like these. It also tries to enable all features of an ext structure (see #928). But since I don't want to change anything about the framework in this PR (would be out of it's scope) I'll see if I can workaround this.

@SaschaWillems
Copy link
Collaborator Author

SaschaWillems commented Mar 11, 2024

This is kinda hard to debug, and I feel like this is platform-specific. The only place where this error is thrown is in (hpp_)phyiscal_device.h and at least on Windows I never even enter that code when running my sample. So we probably do something different/specific when running on your platform that causes the assert to trigger. I'll somehow try to reproduce this on Windows.

I guess it's caused by this part of device creation: https://github.com/KhronosGroup/Vulkan-Samples/blob/main/framework/core/device.cpp#L109

It'll implicitly enable some extensions for all samples if they're supported.

Sometimes I wish our framework wouldn't do so much implicit stuff. Often feels like the exact opposite of Vulkan :/

@SaschaWillems
Copy link
Collaborator Author

Sorry, this currently fails after merging main. Lots of framework changes and at least something missing for me to continue working on this, see #985,

…ed in a sample will be marked as enabled in the instance class

This is required to make other parts of the framework properly work with samples that do custom instance creation
Copy link
Contributor

@gary-sweet gary-sweet left a comment

Choose a reason for hiding this comment

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

I believe this is now missing an entry in samples/CMakeLists.txt. This means the sample isn't currently runnable and doesn't appear in the samples lists in the help.

@SaschaWillems
Copy link
Collaborator Author

Welp, probably happened while merging with main. Thanks for letting me know, will fix.

@SaschaWillems
Copy link
Collaborator Author

Can you check again? Afair the list in that cmake file is purely optional. I'm actually not sure why that list of samples in there even exist. CMake should do a folder traversal and adds all samples it can find, no matter if the sample is explicitly added in that list or not.

@gary-sweet
Copy link
Contributor

Can you check again? Afair the list in that cmake file is purely optional. I'm actually not sure why that list of samples in there even exist. CMake should do a folder traversal and adds all samples it can find, no matter if the sample is explicitly added in that list or not.

Well that's odd then, because the sample didn't exist for me unless I hacked it into that file.

@SaschaWillems
Copy link
Collaborator Author

That's interesting. I just reran cmake and deleted the cache beforehand and the sample is somehow picked up. Will have to investigate.

@SaschaWillems
Copy link
Collaborator Author

Added it to the CMake file anyway. Can't hurt ;)

@gary-sweet
Copy link
Contributor

That's interesting. I just reran cmake and deleted the cache beforehand and the sample is somehow picked up. Will have to investigate.

Hmm. So I trashed my build folders completely and rebuilt and then it appeared, without the entry in cmake. It's possible that the cmake generate hadn't run, which is what I was seeing.

Copy link
Contributor

@gary-sweet gary-sweet left a comment

Choose a reason for hiding this comment

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

The fact that shader-printf 'just works' (provided that you support VK_KHR_shader_non_semantic_info) had kind of passed me by and I had to scratch my head for a little bit when I saw it working with my driver to figure out why!

What a very useful thing, and an excellent sample.

[If I had an extra button to auto-rotate the model it would be even better (since I have no other way to rotate API samples), but I can always add one later on.]

@marty-johnson59 marty-johnson59 merged commit 22c6a8a into KhronosGroup:main Mar 25, 2024
25 checks passed
jeroenbakker-atmind pushed a commit to jeroenbakker-atmind/Vulkan-Samples that referenced this pull request Mar 27, 2024
* Added new shader debzgprrintf sample
Work-in-progress

* Note on descriptor size
Minor cleanup

* Display shaderdebutprintf output in UI

* Proper extension structure chaining

* Restructure code
Use VK 1.2 to avoid a bug with recent Vulkan SDKs

* Minor adjustments

* Use VK 1.1, clean up messages for UI display

* Enable instance extensions

* Started working on documentation/tutorial

* Fixed link

* Minor reordering
Easier to document

* Tutorial fpr shader printf

* Update tutorial

* Code adjustments based on review

* Clang-format...

* Add new argument to custom instance creation so that extensions enabled in a sample will be marked as enabled in the instance class
This is required to make other parts of the framework properly work with samples that do custom instance creation

* Add function to check if sample has a valid instance
Fixes KhronosGroup#985

* Adjust to recent framework changes

* Doc fix

* Typo

* Add sample to cmake ordered list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sample This is relevant to a sample
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to access instance from samples anymore Add sample for shader debug printf
4 participants