[vulkan] Synchronization issue: vkAcquireNextImage uses a semaphore which is in use.

[mcourteaux]

I'm still analyzing this issue, but I wanted to document it a bit better than trying to keep things in my head until I understand it enough to fix it.

To Reproduce

1. Compile bgfx in debug mode with examples.
2. Run example-01-cubesDebug
3. Observe the output:

../../../src/renderer_vk.cpp (683): BGFX ---E-           Semaphore, Validation, 0: Validation Error: [ VUID-vkAcquireNextImageKHR-semaphore-01779 ] Object 0: handle = 0xb3ee8b0000000070, 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.280.0/linux/1.3-extensions/vkspec.html#VUID-vkAcquireNextImageKHR-semaphore-01779)

My interpretation so far After mentally untangling the code for more than an hour now, and reading Vulkan documentation, I sort of conclude the following:

• vkAcquireNextImage() implicitly synchronizes ("waits for") with the point where image N - 2 is presented and ready to be reused for drawing a new frame. It means that it will synchronize with the vkQueuePresent() from two frames ago (assuming double buffering). Once the vkAcquireNextImage() signals the semaphore, new command buffers can be submitted to that frame.
• In a correctly-functioning Vulkan application, the application is supposed to wait for the rendering to frame N be complete, before trying to acquire the next image from the swapchain to produce frame N + 2 (assuming double buffering). The reasons for explicitly waiting for the rendering done fence are this (they all sort of sound the same):
  1. To keep the render loop on CPU in sync with the frames being rendered on GPU.
  2. You actually cannot start on frame N + 2 if frame N is not done, because of all the shared resources.
  3. More specifically, such that you are sure that the vkAcquireNextImage() doesn't try to signal into a semaphore which is already in use, because we are trying to acquire frame N+2, whereas frame N is still waiting on this semaphore before it can start rendering.

If I understand stuff correctly, bgfx has not followed this guideline. bgfx's codepath to vkAcquireNextImage does not seem to wait for anything, hence the issue. More precisely, it seems that it actually has the waiting backwards:

https://github.com/bkaradzic/bgfx/blob/530a558b11afa4375bdb926b398dbe65a5fc6b4b/src/renderer_vk.cpp#L7408-L7474

It first tries to acquire the next image, and THEN waits for the previous frame to finish, by waiting for the backBufferFence of the newly returned frame (which should correspond to frame N - 2). I believe I understand why the original author of this code (@pezcode) has written it this way: the documentation of vkAcquireNextImage() explicitly states:

The order in which images are acquired is implementation-dependent, and may be different than the order the images were presented.

So I believe this non-deterministic ordering of "next image" from the swapchain is why the synchronization is programmed only when it's known which one is the next one.

The funny thing is, kick() submits the command buffer to the queue, and passes a fence to be signaled upon completion: m_completedFence, which is the fence that should be waited for, but actually never is waited for.

https://github.com/bkaradzic/bgfx/blob/530a558b11afa4375bdb926b398dbe65a5fc6b4b/src/renderer_vk.cpp#L7931-L7977

Only if the passed _wait is true, then a fully synchronized submit-and-wait is executed. Otherwise, this fence is never used again.

So, the missing thing in bgfx right now seems to be the wait for this fence upon acquiring the next image. To summarize what synchronization I found by analyzing the code:

1. Context::renderFrame()
   1. RendererContextVK::submit()
      1. RendererContextVK::setFrameBuffer() -> FrameBufferVK::acquire():
         • vkAcquireNextImageKHR(): signal_semaphore: m_lastImageAcquiredSemaphore
         • vkWaitForFences(): wait_fence = m_completedFence
      2. RendererContextVK::kick() -> CommandQueueVK::kick(false)
         • vkQueueSubmit(): wait_semaphore = m_lastImageAcquiredSemaphore; signal_semaphore = m_lastImageRenderedSemaphore; signal_fence = m_completedFence
   2. Context::flip() -> RendererContextVK::flip() -> FrameBufferVK::present() -> SwapChainVK::present()
      • vkQueuePresentKHR(): wait_semaphore = m_lastImageRenderedSemaphore

As you can see, the fence is not waited for before acquiring frame N + 2. Unrolling the above synchronization loop 3 times, and identifying the fences, makes this clearer:

1. [Frame N; frame in flight = 0] Context::renderFrame()
   1. RendererContextVK::submit()
      1. RendererContextVK::setFrameBuffer() -> FrameBufferVK::acquire():
         • vkAcquireNextImageKHR(): signal_semaphore: m_lastImageAcquiredSemaphore (semA 0)
         • vkWaitForFences(): wait_fence = m_completedFence (fenceC 0)
      2. RendererContextVK::kick() -> CommandQueueVK::kick(false)
         • vkQueueSubmit(): wait_semaphore = m_lastImageAcquiredSemaphore (semA 0); signal_semaphore = m_lastImageRenderedSemaphore (semR 0); signal_fence = m_completedFence (fenceC 0)
   2. Context::flip() -> RendererContextVK::flip() -> FrameBufferVK::present() -> SwapChainVK::present()
      • vkQueuePresentKHR(): wait_semaphore = m_lastImageRenderedSemaphore (semR 0)
2. [Frame N + 1; frame in flight = 1] Context::renderFrame()
   1. RendererContextVK::submit()
      1. RendererContextVK::setFrameBuffer() -> FrameBufferVK::acquire():
         • vkAcquireNextImageKHR(): signal_semaphore: m_lastImageAcquiredSemaphore (semA 1)
         • vkWaitForFences(): wait_fence = m_completedFence (fenceC 1)
      2. RendererContextVK::kick() -> CommandQueueVK::kick(false)
         • vkQueueSubmit(): wait_semaphore = m_lastImageAcquiredSemaphore (semA 1); signal_semaphore = m_lastImageRenderedSemaphore (semR 1); signal_fence = m_completedFence (fenceC 1)
   2. Context::flip() -> RendererContextVK::flip() -> FrameBufferVK::present() -> SwapChainVK::present()
      • vkQueuePresentKHR(): wait_semaphore = m_lastImageRenderedSemaphore (semR 1)
3. [Frame N + 2; frame in flight = 0] Context::renderFrame()
   1. RendererContextVK::submit()
      1. RendererContextVK::setFrameBuffer() -> FrameBufferVK::acquire():
         • vkAcquireNextImageKHR(): signal_semaphore: m_lastImageAcquiredSemaphore (semA 0)
         • vkWaitForFences(): wait_fence = m_completedFence (fenceC 0) (waiting for fence from frame N)
      2. RendererContextVK::kick() -> CommandQueueVK::kick(false)
         • vkQueueSubmit(): wait_semaphore = m_lastImageAcquiredSemaphore (semA 0); signal_semaphore = m_lastImageRenderedSemaphore (semR 0); signal_fence = m_completedFence (fenceC 0)
   2. Context::flip() -> RendererContextVK::flip() -> FrameBufferVK::present() -> SwapChainVK::present()
      • vkQueuePresentKHR(): wait_semaphore = m_lastImageRenderedSemaphore (semR 0)

I marked the erroneous vkWaitForFences in bold in frame N+2, where it waits for frame N.

[mcourteaux]

I can confirm the error from Vulkan goes away when explicitly kick(true) instead of kick(false) which causes to immediately synchronize on the submitted command buffer.

[bkaradzic]

cc @pezcode

[mcourteaux]

I managed to make some changes that seems to fix the issue, without breaking anything. Needs more testing. Will open PR if I think it's ready for review.

[pezcode]

As you can see, the fence is not waited for before acquiring frame N + 2

The issue is that (as you mentioned) the image order is not guaranteed. So you can not know the next image, and hence can not know the associated fence before calling vkAcquireNextImageKHR(). Not sure what the correct fix is here, but I feel like the missing synchronization should happen at another point.

Calling kick(true) works because it is essentially vkQueueWaitIdle(), but destroys any kind of performance gains by having multiple frames in flight.