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

Fixed swapchain sync with present. #15

Merged
merged 2 commits into from
Feb 17, 2024
Merged

Conversation

Huntlier
Copy link
Member

Proper present handling with multiple frames in flight

Usage of m_PresentQueue.waitIdle(); with enabled validation layers (m_DeviceParams.enableDebugRuntime) shadows validation errors due to incorrect swapchain handling over frames. For example, overriding Validation Layers for release builds via Vulkan Configurator without enabling debug runtime internally, to prevent unnecessary waitIdle() calls in present queue, leads to constant validation errors:

VUID-vkAcquireNextImageKHR-semaphore-01779(ERROR / SPEC): msgNum: 1461184347 - Validation Error: [ VUID-vkAcquireNextImageKHR-semaphore-01779 ] Object 0: handle = 0xee647e0000000009, type = VK_OBJECT_TYPE_SEMAPHORE; | MessageID = 0x5717e75b | vkAcquireNextImageKHR(): Semaphore must not have any pending operations. The Vulkan spec states: If semaphore is not VK_NULL_HANDLE it must not have any uncompleted signal or wait operations pending (https://vulkan.lunarg.com/doc/view/1.3.275.0/windows/1.3-extensions/vkspec.html#VUID-vkAcquireNextImageKHR-semaphore-01779)
Objects: 1
[0] 0xee647e0000000009, type: 5, name: NULL

Thus, using only one semaphore for handling multiple "frames-in-flight" is not enough. Each "frame-in-flight" must have its own semaphore at least.
Best way is to create 'm_DeviceParams.maxFramesInFlight + 1' semaphores and use them for each "frame-in-flight".


No need to wait idle in present queue

With debug runtime enabled calls like m_PresentQueue.waitIdle() aren't needed at all. Information on vulkan-tutorial.com already changed and validation layers are doing well.
This change also boost perfomance in DEBUG builds. For example: 'rt_bindless' example from 'donut_examples' runs in debug without this changes at around 200 FPS and at around 300 FPS with this changes on NVIDIA RTX 3060 Laptop GPU.

- Per frame in flight semaphores for correct API usage
- Removed unnecessary waitIdle() in present queue
Copy link

github-actions bot commented Jan 24, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Huntlier
Copy link
Member Author

I have read the CLA Document and I hereby sign the CLA

@Huntlier Huntlier marked this pull request as draft February 15, 2024 12:12
Ensuring that number of frames-in-flight is lower than maxFramesInFlight before adding additional event for waiting later.
@Huntlier
Copy link
Member Author

Huntlier commented Feb 15, 2024

After some investigation, I discovered an additional problem related to the loop after sending image to present queue. This loop waits for events until the number of frames-in-flight will be equals m_DeviceParams.maxFramesInFlight.
Right after this loop, a new event is added to the queue, so by the end of DeviceManager_VK::Present() the actual number of frames-in-flight already exceeds m_DeviceParams.maxFramesInFlight.
In GPU-bound scenarios we will encounter a situation where we try to request an image with a semaphore that is still has any uncompleted signal or wait operations pending.

If in the loop we replace the comparison with greater than or equal to, then the actual number of frames in flight by the end of the DeviceManager_VK::Present() guaranteed will be no more than m_DeviceParams.maxFramesInFlight.

@Huntlier Huntlier marked this pull request as ready for review February 15, 2024 21:15
@apanteleev apanteleev merged commit 7946491 into NVIDIAGameWorks:main Feb 17, 2024
1 check passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2024
@apanteleev
Copy link
Contributor

Merged, thank you!

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

Successfully merging this pull request may close these issues.

2 participants