KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.8k stars 468 forks source link

Still not possible to safely destroy semaphore used with present (without mandatory extension use) #2007

Open krOoze opened 1 year ago

krOoze commented 1 year ago

As per #152, without use of extensions it is still not possible to safely destroy (or reuse) wait semaphores used with vkQueuePresentKHR, because there is no way to "retire" the semaphores and even things like vk*WaitIdle are specified as not covering the case.

This results in a situation where applications and implementations are forced to hallucinate unsupported ad-hoc "solutions" with the hope their code is correct™ (i.e. hoping it will always work and keep working).

The problematic parts to cover are:

ANGLE materials mention one such set of "workarounds":

So those are all cases any new drivers might need to honor (even if specification does not say so).

Additionally, the new extensions seem to strongly imply it was not safe to destroy the Swapchain itself:

and can destroy the swapchain when the fences associated with all past presentation requests have signaled.

But vkDestroySwapchainKHR specification seems to say it is fine dealing with outstanding images:

All uses of presentable images acquired from swapchain must have completed execution

The memory of each VkImage will only be freed after that image is no longer used by the presentation engine. For example, if one image of the swapchain is being displayed in a window, the memory for that image may not be freed until the window is destroyed, or another swapchain is created for the window.

cubanismo commented 1 year ago

But vkDestroySwapchainKHR specification seems to say it is fine dealing with outstanding images:

It does not. The image isn't in use by the presentation engine until the queue operation portion of vkQueuePresentKHR() is complete. The queue operation is not complete until the present fence says it is. As noted in #152, "All uses of presentable images" includes outstanding queue operations resulting from vkQueuePresentKHR().

krOoze commented 1 year ago

But as I quoted, destruction only requires the use of acquired images to be complete. Presented images are no longer acquired. But I will give image lifetime spec another look. Maybe it deserves a separate Issue if you are really sure it is supposed to work the way you say (hm, I vaguely remember there already might have been one created...).

cubanismo commented 1 year ago

Presented images are no longer acquired.

They are acquired until the queue operations are done. Otherwise, the work done between when vkQueuePresentKHR() was called and completion of the queue operations performing the pre-present semaphore waits would be touching non-acquired images, which would be invalid usage. Since there is no way to differentiate between the various queue operations (semaphore waits, undefined work on the images related to the queue present), they all must be treated as one block of work, and hence images are not un-acquired until the queue operation portion of vkQueuePresentKHR() is complete.

krOoze commented 1 year ago

I think spec very much strongly implies images are unaquired as soon as vkPresent() returns a success code, but as I said I will give it a look again. It is kinda other way around: present guarantees it won't touch the image until the semaphore is signaled, i.e. what you say is not really a concern. Analogously image is acquired as soon as vkAcquire returns successfully.

cubanismo commented 1 year ago

The entire WSI section is long overdue for a substantial rewrite to better express this IMHO, so I wouldn't put too much thought into the language nuances in there right now. There was originally a concept of image ownership being transferred back and forth from the presentation engine in concert with the synchronization primitives passed in to queue present and out of acquire next image, with that happening on a parallel timeline to the CPU operations themselves. This all got scrubbed when the term "ownership" unfortunately conflicted with some formal terms used in the synchronization chapter, and the language replacing it never correctly captured all these nuances IMHO, but was added while I was on or near one of my paternity leaves IIRC.

krOoze commented 1 year ago

The entire WSI section is long overdue for a substantial rewrite to better express this IMHO, so I wouldn't put too much thought into the language nuances in there right now.

That's not great to hear, but good to know. These kinds of foundations should be settled sooner than later. It won't get better by itself after another five years when 420 new extensions are piled on top of those.

HansKristian-Work commented 1 year ago

In the short term, I'll try to write a more extensive NOTE explaining the situation and what the status quo is for a pre-EXT world. The problems are more theoretical than practical in nature imo.

krOoze commented 1 year ago

Well, in theory there's no difference in theory and practice. In practice there might be. Most people are probably paranoid, so it just gives them peace of mind that they are already doing the right thing or better. But some might be lax and surprised. Besides, the image of us knowing what we are doing (or at least own mistakes and plug the holes retrospectively) is good for impressionable and confusable newcomers. Having a specification is all about weeding out the heresy and giving no space for doubt and divergent implementations. 😅

helviett commented 1 year ago

In the short term, I'll try to write a more extensive NOTE explaining the situation and what the status quo is for a pre-EXT world. The problems are more theoretical than practical in nature imo.

@HansKristian-Work Did you write a note like that? If yes, can you share the link? It would be great to know "good" workarounds to consider for different vendor to make application shutdown errorless.