Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix vulkan fence synchronization #17969

Merged
merged 4 commits into from
May 28, 2024
Merged

Fix vulkan fence synchronization #17969

merged 4 commits into from
May 28, 2024

Conversation

msat-huawei
Copy link
Contributor

What does this PR do?

This fixes issues related to synchronization of VkFence and binary semaphores introduced by #17761 , when a device without timeline semaphore support is used:

  1. Re-added synchronization to fences used in the Vulkan async upload queue. The SignalEvent class of the Fence was formerly owned by the Fence itself. Now it must be supplied from outside the class.
  2. In the case of waiting for a fence that is signaled from the framegraph, we did not wait for the fence itself to be signaled. Only the fences/semaphores it dependent upon. We now also wait for the fence itself to be signaled.

This PR should fix #17890. I cannot test this directly as I don't have access to a Quest. We however managed to fix multiple Vulkan validation warnings related to VkFence and multi threading.

How was this PR tested?

Tested with various atom sample viewer samples, and some custom levels.
I couldn't get the OpenXRTest project to run, as mentioned in #17890. There are many passes/shaders that won't compile in the AssetProcessor. We managed to create Vulkan validation errors in some of the atom sample viewer samples however.

@msat-huawei
Add fence synchronization back to async upload queue fences
01049f8 
Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
@msat-huawei
Fix user awaited fence synchronization
58d9515 
Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
@msat-huawei
Move AttachmentReadback wait to Execute
6501f0b 
Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
@msat-huawei msat-huawei requested review from a team as code owners May 22, 2024 14:16
@msat-huawei msat-huawei requested a review from galibzon May 22, 2024 14:22
@galibzon
Copy link
Contributor

I'm testing this PR now on a Quest Pro.

@galibzon
Copy link
Contributor

I got this compilation error:

C:\GIT\o3de\Gems\Atom\RHI\Vulkan\Code\Source\RHI\AsyncUploadQueue.cpp(107,22): error C2220: the following warning is treated as an error [C:\GIT\o3de-extras\Projects\Op
enXRTest\build\External\Vulkan-4a6b04ec\Code\Atom_RHI_Vulkan.Private.Static.vcxproj]
                  auto signalEvent = AZStd::make_shared<SignalEvent>();
                       ^ (compiling source file '../Vulkan-4a6b04ec/Code/CMakeFiles/Atom_RHI_Vulkan.Private.Static.dir/Unity/unity_1_cxx.cxx')

C:\GIT\o3de\Gems\Atom\RHI\Vulkan\Code\Source\RHI\AsyncUploadQueue.cpp(107,22): warning C4456: declaration of 'signalEvent' hides previous local declaration [C:\GIT\o3de
-extras\Projects\OpenXRTest\build\External\Vulkan-4a6b04ec\Code\Atom_RHI_Vulkan.Private.Static.vcxproj]
                  auto signalEvent = AZStd::make_shared<SignalEvent>();
                       ^ (compiling source file '../Vulkan-4a6b04ec/Code/CMakeFiles/Atom_RHI_Vulkan.Private.Static.dir/Unity/unity_1_cxx.cxx')
  C:\GIT\o3de\Gems\Atom\RHI\Vulkan\Code\Source\RHI\AsyncUploadQueue.cpp(99,18):
  see declaration of 'signalEvent'
              auto signalEvent = AZStd::make_shared<SignalEvent>();

@martinwinter-huawei
PR fix
6c3cd58 
Signed-off-by: Martin Winter <martin.winter@huawei.com>
@galibzon
Copy link
Contributor

Tested with OpenXRTest and it's working. Thanks a lot for taking care of this issue.

@akioCL akioCL merged commit ce7058c into o3de:development May 28, 2024
3 checks passed
jjs-home pushed a commit to jjs-home/o3de that referenced this pull request May 29, 2024
@msat-huawei @martinwinter-huawei @jjs-home
Fix vulkan fence synchronization (o3de#17969)
f525fe8 
* Add fence synchronization back to async upload queue fences
* Fix user awaited fence synchronization
* Move AttachmentReadback wait to Execute

Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
Signed-off-by: Martin Winter <martin.winter@huawei.com>
Co-authored-by: Martin Winter <martin.winter@huawei.com>
@moudgils
Copy link
Contributor

@msat-huawei Can we cherry pick this PR into stabilization branch. Thanks.

msat-huawei added a commit to msat-huawei/o3de that referenced this pull request Jun 3, 2024
@msat-huawei @martinwinter-huawei
Fix vulkan fence synchronization (o3de#17969)
95c1de4 
* Add fence synchronization back to async upload queue fences
* Fix user awaited fence synchronization
* Move AttachmentReadback wait to Execute

Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
Signed-off-by: Martin Winter <martin.winter@huawei.com>
Co-authored-by: Martin Winter <martin.winter@huawei.com>
@msat-huawei msat-huawei mentioned this pull request Jun 3, 2024
Merged
jhmueller-huawei pushed a commit that referenced this pull request Jun 4, 2024
@msat-huawei @martinwinter-huawei
Fix vulkan fence synchronization (#17969) (#18007)
1adecd0 
* Add fence synchronization back to async upload queue fences
* Fix user awaited fence synchronization
* Move AttachmentReadback wait to Execute

Signed-off-by: Martin Sattlecker <martin.sattlecker@huawei.com>
Signed-off-by: Martin Winter <martin.winter@huawei.com>
Co-authored-by: Martin Winter <martin.winter@huawei.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akioCL akioCL akioCL approved these changes

@galibzon galibzon galibzon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Applications Running Natively On Meta Quest Devices Are Deadlocked
5 participants
@msat-huawei @galibzon @moudgils @akioCL @martinwinter-huawei