Optionally don't call vkDeviceWaitIdle in imgui_impl_vulkan.cpp

[akeley98NV]

Version/Branch of Dear ImGui:

Version: 089ed30
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_vulkan.cpp
Compiler: all supported compilers
Operating System: all supported OS

My Issue/Question:

In the reference Vulkan implementation, we are sometimes implicitly calling vkQueueWaitIdle, sometimes implicitly calling vkDeviceWaitIdle. For example, in ImGui_ImplVulkan_CreateFontsTexture, we call vkQueueWaitIdle before calling ImGui_ImplVulkan_DestroyFontsTexture, but later call vkDeviceWaitIdle after submitting the command buffer. The implicit vkDeviceWaitIdle calls pose difficulties for me as my graphics code is not on the same CPU thread as my async compute code, and the Vulkan spec requires the caller to synchronize CPU access to all queues. So, if I'm doing things by the book, I have to use a mutex to ensure the ImGui code won't run at the same time as my async compute code, even though the two really have nothing to do with each other.

The way we solved this in the nvpro_core framework is to by-default use vkDeviceWaitIdle (for backwards compatibility with code that may somehow be relying on this) but allow the caller to opt-into substituting all vkDeviceWaitIdle calls with vkQueueWaitIdle. Alternatively maybe no one is truly relying on vkDeviceWaitIdle at all and we can unconditionally replace all vkDeviceWaitIdle calls with vkQueueWaitIdle.

Example API: https://github.com/nvpro-samples/nvpro_core/blob/26a5a00282641248755cd874b3e94f32a5bafd74/nvvk/swapchain_vk.hpp#L364

Example implementation (replaced all vkDeviceWaitIdle calls with this): https://github.com/nvpro-samples/nvpro_core/blob/26a5a00282641248755cd874b3e94f32a5bafd74/nvvk/swapchain_vk.hpp#L241C12-L241C12

This is an easy-to-make change on my end, but I make the issue anyway as requested by the header comment.

[ocornut]

Hello,

There are 4 calls to vkDeviceWaitIdle() could you help by going over each of them?
Are you affected by all 4 of them or specifically by the one in _CreateFontsTextures()?

• Could the one in ImGui_ImplVulkan_CreateFontsTexture() unconditionally be simply replaced by vkQueueWaitIdle(v->Queue) since in this case we are already working with a queue? It seems like the main one that could cause problem and HAS an obvious solution and is a mistake in our code?
• ImGui_ImplVulkan_SetMinImageCount() i guess doesn't matter it is an init-only thing.
• ImGui_ImplVulkanH_CreateWindowSwapChain() and ImGui_ImplVulkanH_DestroyWindow() are slightly more annoying because they are technically helpers used by both examples AND by the multi-viewport system.

Alternatively maybe no one is truly relying on vkDeviceWaitIdle at all and we can unconditionally replace all vkDeviceWaitIdle calls with vkQueueWaitIdle

I don't know but I would be interested if you could investigate this? (preferably in docking branch). It might also make sense to move the wait to caller code.

[akeley98NV]

Looking at it more closely it does seem like ImGui_ImplVulkan_CreateFontsTexture is the only vkDeviceWaitIdle that matters (I don't use the ImGui swapchain helpers) and this is due to a recent change:

2023-11-10: BREAKING CHANGE: Removed parameter from ImGui_ImplVulkan_CreateFontsTexture(): backend now creates its own command-buffer to upload fonts.

I'll make a PR later to swap the vkDeviceWaitIdle for vkQueueWaitIdle and leave the others as-is. Thanks!

[ocornut]

swap the vkDeviceWaitIdle for vkQueueWaitIdle and leave the others as-is.

Now pushed that change as 33d4268

[akeley98NV]

Looks good.