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

Fix support for --headless. #1090

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

Fix support for --headless. #1090

wants to merge 2 commits into from

Conversation

iagoCL
Copy link

@iagoCL iagoCL commented Jul 8, 2024 •
edited
Loading

Description

The main objective of this PR is to fix the --headless flag. Currently it does not work in the framework, due to not requesting a surface and swap chain.

This PR reworks HeadlessWindow to make it more similar to the other window classes. This will allow us to enable VK_EXT_headless_surface in HeadlessWindow::get_required_surface_extensions like the other windows types. Now we can remove the headless paramater from Instance::Instance and VulkanSample<>::create_instance simplifying the framework.

To keep the samples hello_world and hpp_hello_world as simple as possible, I did not add support for headless. Instead, an assert is run to ensure the are not enabling with headless in those samples.

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

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

Fixes #830
Fixes #314

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@JoseEmilio-ARM JoseEmilio-ARM requested a review from a team July 10, 2024 13:47
@SaschaWillems SaschaWillems self-requested a review July 11, 2024 14:55
SaschaWillems
SaschaWillems requested changes Jul 11, 2024
View reviewed changes
Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This crashes for me when trying to start any sample in headless mode.

The crash happens in getSurfaceSupportKHR in the vulkan hpp header and is coming from get_suitable_gpu in hpp_instance.cpp.

Callstack:

image

It works fine without headless.

System is Windows 11 with an NV RTX 4070 and recent Vulkan drivers. The K_EXT_headless_surface extension is supported.

I also have a switfshader ICD installed to test features not supported by my GPU, but the crash happens when calling getSurfaceSupportKHR for my discrete RTX 4070.

@nv-ahamelin
Copy link

Change tested on:

I have tested 3 different setups, 1. Display connected+GDM3 started, 2. Display connected+GDM3 shutdown and 3. Display disconnected.

  1. Display connected+GDM3 started
    The change appears to be functional on my side, I am able to run multiple samples with the --headless option without encountering errors.

  2. Display connected+GDM3 shutdown
    The samples will fail with the following error message:

[error] Error Message: vk::Device::acquireNextImageKHR: ErrorInitializationFailed
[error] Failed when running application Swapchain Images

A bit of investigation led me to a failure in the wsi-layer where the usage of vkImportSemaphoreFdKHR when found available fails, leading to the error we see above.
I made a local modification to the layer to bypass that usage and instead use the layer fallback path instead in the following file [...]/vulkan-wsi-layer/wsi/swapchain_base.cpp:L434 . With that modification, I am able to run successfully the samples in this configuration.

  1. Display disconnected
    I have observed the same behavior as the one in 2. and required the same local modification to the layer to make it function.

@iagoCL
Copy link
Author

iagoCL commented Aug 7, 2024

My current setup is using Swiftshader https://github.com/google/swiftshader "a high-performance CPU-based implementation of the Vulka 1.3 graphics AP" the headless flag seems to work correctly in all tested examples

I can reproduce SaschaWillems crash. Needs more investigation, but seems to be valid use of the API, and only reproducible if you enable ICDs for Nvidia and SwiftShader. As a workaround you can avoid the crash explicitily selecting the valid GPU using the --gpu flag
I also agreed with SaschaWillems to also add some small improvements to the code and extra documentation to better explain VK_EXT_headless_surface, its intention and how it is diferent from rendering without swapchain.
I have been a bit busy this last month, but hopefully I will submit a new PR with some improvements soon.

@iagoCL iagoCL changed the title Fix support for --headless. WIP: Fix support for --headless. Aug 7, 2024
@iagoCL iagoCL force-pushed the iagoCL/fix_headless_support branch 2 times, most recently from f73c125 to f403630 Compare August 22, 2024 17:32
@iagoCL iagoCL changed the title WIP: Fix support for --headless. Fix support for --headless. Aug 22, 2024
@iagoCL
Copy link
Author

iagoCL commented Aug 22, 2024

Change the name of the flag and add some comments to better explain it.
I also added an explanation in the example in the readme.
Based on some feedback I also added a warning when running headless with multiple GPU candidates.
Tested on Windows and Linux using Swiftshader.

@iagoCL iagoCL force-pushed the iagoCL/fix_headless_support branch 2 times, most recently from 7617d5c to 68fe8fa Compare August 29, 2024 11:15
@iagoCL
Fix support for --headless.
85aa207 
Now --headless will create a swap_chain and a surface, now the feature will work correctly.
Remove headless parameter from Instance::Instance and VulkanSample<>::create_instance.
VK_EXT_HEADLESS_SURFACE_EXTENSION_NAME  added in HeadlessWindow::get_required_surface_extensions like other window types
Added an assert if headless is enabled to hello_triangle and hpp_hello_triangle
@iagoCL
Change flag name to headless_surface.
e477c24 
Use offscreen when speaking about no swapchain
Add warning when using headless on multiple GPUs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SaschaWillems SaschaWillems SaschaWillems requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Headless mode seems to be broken Headless mode crashes with API samples
4 participants
@iagoCL @CLAassistant @nv-ahamelin @SaschaWillems