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

Fix synchronization in the ash-examples #979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vadim-s-kovy
Copy link

@vadim-s-kovy vadim-s-kovy commented Jan 14, 2025

What is this PR doing?

#917 ERROR: VALIDATION on examples
When ash-examples is executed with Vulkan 1.3.290.0, it generates error messages.

cargo run -p ash-examples --bin triangle

ERROR:
VALIDATION [VUID-vkAcquireNextImageKHR-semaphore-01779 (1461184347)] : Validation Error: [ VUID-vkAcquireNextImageKHR-semaphore-01779 ] Object 0: handle = 0xd5b26f0000000010, 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.290.0/mac/1.3-extensions/vkspec.html#VUID-vkAcquireNextImageKHR-semaphore-01779)

The PR is fixing this synchronization by including draw_commands_reuse_fence into the render_loop.

@koplas
Copy link

koplas commented Jan 14, 2025

I also created a pull request to fix this issue; see #941.
I avoid waiting for the same fence twice in this pull request, as this is unnecessary. The function record_submit_commandbuffer already waits for the draw_commands_reuser_fence, but this is done too late.

As the examples also need a complete overhaul, I would close my pull request in favor of this if the issue (albeit not optimal) will be resolved.

@vadim-s-kovy
Copy link
Author

vadim-s-kovy commented Jan 18, 2025

... waiting for the same fence twice in this pull request, as this is unnecessary. The function record_submit_commandbuffer already waits for the draw_commands_reuser_fence, but this is done too late.

We actually don't wait for that fence second time, since it's not reset after actual waiting. However, this logic is a bit convoluted and implicit: we are waiting for a fence and not reseting it, expecting it will be reset later in record_submit_commandbuffer. A cleaner approach would be to wait and explicitly reset immediately before acquire_next_image and not do it inside record_submit_commandbuffer. Please, let me know if that approach makes sense.

@koplas
Copy link

koplas commented Jan 19, 2025

... waiting for the same fence twice in this pull request, as this is unnecessary. The function record_submit_commandbuffer already waits for the draw_commands_reuser_fence, but this is done too late.

We actually don't wait for that fence second time, since it's not reset after actual waiting. However, this logic is a bit convoluted and implicit: we are waiting for a fence and not reseting it, expecting it will be reset later in record_submit_commandbuffer. A cleaner approach would be to wait and explicitly reset immediately before acquire_next_image and not do it inside record_submit_commandbuffer. Please, let me know if that approach makes sense.

Yes, waiting and then resetting the fence before acquire_next_image would be the clean approach. This is also done in the official Vulcan cube demo: https://github.com/KhronosGroup/Vulkan-Tools/blob/26c7bde34a4a253004af427d92b27656e386effc/cube/cube.c#L1146-L1147.
And yes, calling wait_for_fences on a fence that was already signaled returns immediately; my point was that this causes unnecessary API overhead and does not scream best practices (Sorry, I was a bit inaccurate in my description).

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.

2 participants