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.

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

Introduce semaphores and wait on submit #4946

Closed
wants to merge 1 commit into from

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Dec 31, 2023

Connections
#4689, #4775, #4919

bevyengine/bevy#10792

Description
This is a WIP PR to gather feedback. I've yet to port all the examples or other backends than Vulkan to use semaphores and fill out the SemaphoreDescriptor fully.

This introduces user-facing semaphores and allows one to be used to synchronize when a surface texture becomes available during submission.

It was suggested in #4775 that the fence blocking might be a driver bug. I don't necessarily think that's the case. The fence used is only required to trigger at some point in the future when the surface texture becomes available. Vulkan on Android for example (I can't recall where I read this) seems to hold onto the surface texture until a new frame is available from a call to submit. In that scenario it's understandable why the fence never signals in user facing code prior to that.

This might imply that wait semaphores are the only "correct" way to perform this synchronization when using vkAcquireNextImageKHR. So it might be a good idea to make this a user facing feature which is what this PR does.

Alternative designs

It might be possible to keep track of the semaphores internally. I'm just not sure how that can be done right now while not limiting functionality.

TODO

  • Implement changes to other backends.
  • Fix examples to use semaphores.
  • Should we support more than one semaphore during submit? One might use more than one acquired image in the pipeline.

@udoprog udoprog requested a review from a team as a code owner December 31, 2023 05:35
@udoprog udoprog marked this pull request as draft December 31, 2023 05:36
@cwfitzgerald
Copy link
Member

Before you get too far down this path - we unfortunately can't accept this as it is. The semaphore/fence is a decision completely internal to wgpu-hal's vulkan backend, and it definitely doesn't need to go all the way out to the user.

You definitely can set up semaphores completely internally to the vulkan backend, I had a branch which did exactly that. It made no difference on all the drivers I tested, so it never went anywhere. I believe you need a semaphore per frame in the swapchain. Each submit waits on the previous frame's submission semaphore, and the acquire semaphore for that image. If you're interested in implementing this, let's chat on the matrix.

Fence is definitely correct, but it doesn't seem common, which could perhaps cause problems with drivers.

@udoprog
Copy link
Contributor Author

udoprog commented Jan 3, 2024

Thanks! And don't worry, this is a draft because I just wanted to garner feedback. I never expected it to be merged like this. Implementing this was a decent excuse to get a feel for how wgpu manages and tracks resources.

I'll poke you about the other parts on Matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants