Vulkan: Use timeline semaphores for synchronization if possible

[msat-huawei]

What does this PR do?

With this PR the Vulkan RHI backend uses timeline semaphores instead of binary semaphores to synchronize execution across queue. Binary semaphores are still used for swapchains.
The synchronization still supports binary semaphores for synchronization if timeline semaphores are not available for a device.

What does the PR actually do?

1. Use timeline semaphores where possible (everywhere but swapchains)
2. Add a new synchronization construct. Binary semaphores have the limitations that the wait operation must be submitted to the queue before the signal operation is. Timeline semaphores don't have this limitation, but when mixing timeline semaphores with binary semaphores we still need to synchronize the queue operations. When enqueuing a binary semaphore wait operation, all dependent semaphore (including timeline semaphores) wait operations must have been submitted already. See here and here. For binary semaphores this was accomplished by a Signal/Wait on the CPU that ensured the right submit order. We no longer call this functions. Instead we introduce a SemaphoreTracker class that counts how many semaphores are on the framegraph before a swapchain, and how many semaphores have already been submitted to the queue. We don't track which specific semaphores have been signaled, just how many semaphores a swapchain depends on have been signaled.
3. A way to tell the framegraph that an external (Fence) semaphore has been signalled. This is accomplished by the FenceTracker class. The user of a Fence is responsible to call the FenceTracker::SignalUserSemaphore when calling the Fence::SignalOnCpu function.

Why do we need this?

In a previous PR (#17569) we introduced timeline semaphores for user defined Fences. We found that signalling these from the CPU lead to a deadlock somewhere in the graphics driver/kernel, which is explained by the Vulkan specification linked above.
Signalling Fences from the CPU will be used in MultiGPU contexts. See this RFC

In addition to fixing the problem above, this may also lead to a slight performance gain, when semaphores are used for syncing between queues. The queue submits no longer need to wait for each other, except for the swapchain.

How was this PR tested?

Tested with multiple levels and atom sample viewer samples (async compute, multi pipeline, multi scene, mesh) on Windows with Vulkan.
I cannot test this on a machine without timeline semaphore support because I don't have access to hardware. I did deactivate the m_signalFenceFromCPU manually however to test this codepath.
This was also tested with the MultiGPU RPI sample in atom sampler viewer in this branch: https://github.com/jhmueller-huawei/o3de/tree/multi-device-copy-pass-test

[jhmueller-huawei]

Looks good, great job! Just minor comments from my side, but I'd like to get this reviewed by @moudgils as well.

[moudgils]

Recommend waiting for @akioCL approval before merging. I will also try to find time to go through it this week.

[akioCL]

I'm going through the review but I feel like this change increased the complexity of semaphores a lot. This is starting to look like spaghetti code. Maybe this is a signal that a deeper refactor is need in order to accommodate binary and timeline semaphores. For example, almost all functions of the semaphore class have a different behavior for binary and timeline semaphore. Maybe we should separate into two classes with a common interface? I'm having a hard time following the semaphore logic in the Vulkan RHI. What do you guys think?

[msat-huawei]

I'm going through the review but I feel like this change increased the complexity of semaphores a lot. This is starting to look like spaghetti code. Maybe this is a signal that a deeper refactor is need in order to accommodate binary and timeline semaphores. For example, almost all functions of the semaphore class have a different behavior for binary and timeline semaphore. Maybe we should separate into two classes with a common interface? I'm having a hard time following the semaphore logic in the Vulkan RHI. What do you guys think?

I refactored the code a bit so I could remove the FenceTracker class from the public RHI API. The SemaphoreTracker classes are not signalled and waited-for through the Fence/Semaphore classes. This removes a lot of the new code I added.

Do you mean the implementation in the Semaphore/Fence with Spaghetti code? Should I split them into a timeline semaphore and binary semaphore/VkFence specific class?

[msat-huawei]

I'm gettings some validation errors like:

vkDebugMessage: [ERROR][Validation] Validation Error: [ UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout ] Object 0: handle = 0x20582dd94b0, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4dae5635 | vkQueueSubmit(): pSubmits[0].pCommandBuffers[0] command buffer VkCommandBuffer 0x20582dd94b0[] expects VkImage 0x9484f20000012180[LuminanceMapAttachmentId] (subresource: aspectMask 0x1 array layer 0, mip level 0) to be in layout VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL--instead, current layout is VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL.
vkDebugMessage: [ERROR][Validation] Validation Error: [ UNASSIGNED-VkImageMemoryBarrier-image-00004 ] Object 0: handle = 0x20582dd94b0, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4acfa767 | vkQueueSubmit(): in submitted command buffer VkImageMemoryBarrier acquiring ownership of VkImage (VkImage 0x35807000000d02e[]), from srcQueueFamilyIndex 0 to dstQueueFamilyIndex 2 has no matching release barrier queued for execution.
[ERROR][Validation] Validation Error: [ UNASSIGNED-VkImageMemoryBarrier-image-00004 ] Object 0: handle = 0x2060a142060, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4acfa767 | vkQueueSubmit(): in submitted command buffer VkImageMemoryBarrier acquiring ownership of VkImage (VkImage 0x35807000000d02e[]), from srcQueueFamilyIndex 2 to dstQueueFamilyIndex 0 has no matching release barrier queued for execution.

This are likely false positives as discussed here and here.

After the refactoring the PR does not longer work for the MultiGPU sample in this branch. I will mark the PR as draft until I fixed that

[akioCL]

I'm going through the review but I feel like this change increased the complexity of semaphores a lot. This is starting to look like spaghetti code. Maybe this is a signal that a deeper refactor is need in order to accommodate binary and timeline semaphores. For example, almost all functions of the semaphore class have a different behavior for binary and timeline semaphore. Maybe we should separate into two classes with a common interface? I'm having a hard time following the semaphore logic in the Vulkan RHI. What do you guys think?

I refactored the code a bit so I could remove the FenceTracker class from the public RHI API. The SemaphoreTracker classes are not signalled and waited-for through the Fence/Semaphore classes. This removes a lot of the new code I added.

Do you mean the implementation in the Semaphore/Fence with Spaghetti code? Should I split them into a timeline semaphore and binary semaphore/VkFence specific class?

I do think we should split the fence and semaphore classes into a normal/timeline_semaphore fence, and a binary/timeline semaphore class. It would make the code easier to read and understand.
Also, after re-reading the standard, I think we have a problem with binary semaphores in general, independent if we are using timeline semaphores or not. Currently, binary semaphores wait on CPU before the signal operation for the SAME semaphore has been submitted. But, according to the standard, it has to wait for all dependencies (not only for it's own signal operation), so we are missing dependencies. We need to do this for ALL binary semaphores, it doesn't matter if we are using timeline semaphores or not.

As for a solution, I'm not a fan of the SemaphoreTracker, SemaphoreTrackerCollection and SemaphoreTrackerHandle. I think we can reuse what we already have. Currently, the binary semaphore has a condition_variable that's signal once, and wait once. We should refactor the Vulkan::SignalEvent class to accept multiple signal calls. We would share a SignalEvent between the dependent semaphores (similar to the shared SemaphoreTrackerHandle). So for all binary semaphores, they would need to wait on CPU until ALL previous dependent semaphore have signal the SignalEvent. We cannot use the "count" approach, because now the Binary semaphore is not the last one, so we need to know who is signaling (maybe a bitset in the SignalEvent class). We can get the dependent semaphores information from the Framegraph. You are currently doing it in the FrameExecuter, but I think it should go at the end of the FrameGraphCompiler::CompileInternal, after all barriers and semaphores have been added to the scopes.

Since Timeline semaphores don't care about out of order signal/wait operations, they would only signal the SignalEvent, but they wouldn't waste time waiting on CPU. Only Binary semaphores would waste time waiting until all dependent semaphores have submit their signal into a queue (it also includes itself).

What do you think?

[msat-huawei]

I don't think we have a problem when only binary semaphores are present.
When we submit a wait operation for a semaphore we wait for the signal operation to finish.
Before the signal operation is submitted, we wait for the signal operations of the directly dependent semaphores to be enqueued.
The dependent semaphores wait on their dependencies and so on.
Or maybe I don't understand something there.

I will split the Semaphore/Fence classes into normal/timeline semaphore classes.

Your approach also makes sense. Moving it to the FrameGraphCompiler also makes sense. I will implement it.

[msat-huawei]

@akioCL I refactored the synchronization code according to your suggestion, and split the Fence/Semaphore into multiple classes. Can you have a look at it again?

[akioCL]

I think it looks much better. I'm still going through the review but I added a comment about the Factory::CreateFence function. Will publish the rest of my comments on Monday.

[akioCL]

We don't need to change the public interface of the Factory to have 2 different type of fence implementations. What I would do is have a Vulkan::Fence class, and inside that class a member that is the Fence Implementation (m_fenceImpl) using the FenceBase interface. Then, during Initialization, we create the proper Fence Implementation depending on the device properties, and we funnel the calls of the Vulkan::Fence to the FenceBase class.

[akioCL]

Some minor comments, but I think after addressing them it should be good to go.

[martinwinter-huawei]

@akioCL we did another refactor, what do you think about this?

[akioCL]

Looks good! Thanks for making the changes