ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
60.77k stars 10.27k forks source link

Vulkan validation error with SDK 1.3.275 #7236

Open mklefrancois opened 9 months ago

mklefrancois commented 9 months ago

Version/Branch of Dear ImGui:

Version docking

Back-ends:

imgui_impl_vulkan

Compiler, OS:

Windows 11

Full config/build information:

No response

Details:

Running the example_glfw_vulkan will generate validation errors with the latest Vulkan SDK 1.3.275

[vulkan] Debug report from ObjectType: 5
Message: Validation Error: [ VUID-vkAcquireNextImageKHR-semaphore-01779 ] Object 0: handle = 0x967dd1000000000e, 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)

One way to fix this is to move vkWaitForFences before vkAcquireNextImageKHR. Doing this in imgui_impl_vulkan.cpp::ImGui_ImplVulkan_RenderWindow and main.cpp::FrameRender fix the errors.

Another solution would be to create SemaphoreCount = ImageCount+1 and to change wd->SemaphoreIndex = (wd->SemaphoreIndex + 1) % wd->SemaphoreCount;

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

mklefrancois commented 9 months ago

The suggestion to move vkWaitForFences before vkAcquireNextImageKHR will cause stuttering on fast GPU. The easiest fix, without changing the semaphores to Timeline semaphores, seems to make SemaphoreCount = ImageCount+1.

Here is the patch: imgui-vk_error-fix.zip

ocornut commented 9 months ago

Thank you Martin-Karl. Did the fix work for you with multi-viewports enabled?

mklefrancois commented 9 months ago

Yes, it works with docking and multiple viewports.

ocornut commented 9 months ago

Thank you! I have confirmed the issue with latest SDK and confirmed fixes. Pushed fixes 5ddfbb8 and 01b99a9 (docking).

mklefrancois commented 9 months ago

Thanks again for the great work and super fast response!

Tom-Olsen commented 3 months ago

I'm new to vulkan and encountered this issue. I have a follow up question. Instead of using "SemaphoreCount = ImageCount+1" shouldn't the correct way be to use the image count of the swapchain? If my understanding is correct the issue here is that the swapchain can give you the next image in vkAcquireNextImageKHR(...) before we wait for and reset the fence. Thus, if we have one semaphor for each image in the swapchain all should be good, because the swapchain can't give the next image until the next image is actually available.

This question is more for fundamental understanding of the underlying logic here.

InchChou commented 3 months ago

I'm new to vulkan and encountered this issue. I have a follow up question. Instead of using "SemaphoreCount = ImageCount+1" shouldn't the correct way be to use the image count of the swapchain? If my understanding is correct the issue here is that the swapchain can give you the next image in vkAcquireNextImageKHR(...) before we wait for and reset the fence. Thus, if we have one semaphor for each image in the swapchain all should be good, because the swapchain can't give the next image until the next image is actually available.

This question is more for fundamental understanding of the underlying logic here.

Agreed. After reading the spec, I still can't understand why we can't use the image count of the swapchain for semaphores. The fence is used to wait command buffer available and it actually affect the availability of sempahore. My guess is that the wait semaphore is associated with the operations in the command buffer. After the first operation is executed, the wait semaphore will become unsignaled, but the wait semaphore will become available only when all associated operations are completed. So we may need wait fence before vkAcquireNextImageKHR().

ocornut commented 3 months ago

I haven't been able to look at this, but may be good to open a PR (will a link to those explanation) for the records. Thanks!

NostraMagister commented 2 months ago

This topic needs serious thought.

1) Dear ImGui creates unsignaled semaphores (there is no other way) and signaled fences by using a creation flag to ask for initial signaling. This makes a vkWaitForFences before vkAcquireNextImageKHR possible during the first pass after a swapchain rebuild because all fences are signaled, hence also the one in the Dear ImGui frame set array at index 0.

2) The Fence being in the signalled state when entering the rendering function CANNOT be used in vkAcquireNextImageKHR. The specs are EXPLICIT the fence MUST be unsignaled at function call time.

3) Yet, the Dear ImGui render code depends on the signaled state of that fence to be sure that the command buffer may be reused. That would be an argument to vkWaitForFences before vkAcquireNextImageKHR, because ones the fence is/becomes signaled, it could be reset and used in vkAcquireNextImageKHR. However, as demonstrated below it won't work.

4) vkAcquireNextImageKHR() does NOT guarantee sequentially in the returned indices, certainly not if the present mode is MAILBOX. Hence, anticipating the index of the Dear ImGui FrameSet structure array holding the wanted Fence for vkWaitForFences is IMPOSSIBLE. Therefor, waiting before vkAcquireNextImageKHR is not an option in the current design. And by the way, the faster your system (CPU) the bigger the chance that indexes are coming out in non sequential order.

5) vkAcquireNextImageKHR is an ASSYNCHRONEOUS function. Using it the way it is used in Dear ImGui will have performance penalties. vkAcquireNextImageKHR returns when it has an index, and will signal when the acquisition is complete, hence the specs are EXPLICIT, either the semaphore or the fence MUST be non-NULL. They both signal simultaneously. The semaphore should be used to tell the GPU side that acquisition is ready, the fence is to tell the CPU side that it is ready.

6) I found nothing in the Dear ImGui Vulkan backend making me believe that the GPU has a need to know about acquisition ready. The fence blocks the CPU anyways and no new submit will take place without a signaled fence. Therefore, this acquisition semaphore may be futile. Setting info.waitSemaphoreCount = 0; andinfo.pWaitSemaphores = VK_NULL_HANDLE; and not creating this semaphore would do fine. The Vulkan specs make this semaphore option anyways.

7) Dear ImGui has a separate call for the presentation (SwapBuffers) and the presentation info will contain the renderer semaphore. That one needs to be maintained because this is GPU-GPU synchronization. Presentation must be done only when the last submit is completely executed at the GPU side.

8) It is not clear why presentation has been split from the render function. The code starts with uint32_t present_index = wd->FrameIndex;which is, IMO, a subscription for future trouble, certainly in multi-threaded and asynchronous scenarios where the FrameIndex could ALLREADY be overwritten through a call to vkAcquireNextImageKHR. It is safe for now because the Dear ImGui thread calls it without contention, knowing no vkAcquireNextImageKHR can have been called between both functions.

9) The current approach will, IMO, become slower and slower as more viewports are simultaneously used. The reason being that one viewport blocks the activities of all others while it waits on those semaphores in a synchronous way on the Dear ImGui thread.