Remove busy loops in SDL_GPUFence #14547
Conversation
SDL_GPUFence is implemented as just a busy loop on an atomic int, a SDL_GPUCondition gives the cpu some rest
removed bad casts
|
Will defer to @thatcosmonaut here. All of this logic is shared between Vulkan, D3D12, and Metal, so whatever strategy we use here should apply to all drivers. |
|
I think Metal is kind of unique here, because if I recall correctly it doesn't provide an explicit wait mechanism. Vulkan and D3D12 provide waits in the spec. |
|
is nobody going to merge? seems pretty serious. I think when the user calls wait for the gpu work to finish, they don't expect it to busy loop on apple devices, destroying the battery life |
src/gpu/metal/SDL_gpu_metal.m
Outdated
| // Notify the fence when the command buffer has completed | ||
| [metalCommandBuffer->handle addCompletedHandler:^(id<MTLCommandBuffer> buffer) { | ||
| SDL_AtomicIncRef(&metalCommandBuffer->fence->complete); | ||
| SDL_LockMutex(metalCommandBuffer->fence->mutex); |
There was a problem hiding this comment.
Again, create a local variable for the fence instead of indirecting it several times in a row.
|
METAL_INTERNAL_FenceOpen() seems like an odd choice for a function name. What is it opening? Would METAL_INTERNAL_FenceComplete() be more appropriate? |
|
This code feels very much like something thrown together very quickly. If you clean it up and @thatcosmonaut approves, we can potentially merge this for 3.4, but it's more likely to need more testing and go in for 3.6. |
semantic fixes Co-authored-by: Sam Lantinga <slouken@libsdl.org>
more semantic changes
|
ok, good I just want people to be aware that these issues exist in the metal implementation and it won't pass app store review for example. The max frames in flight, waiting on a command buffer, waiting for a swapchain texture, etc. is done through a busy loop and I hope the pull requests or other fixes will be merged in the foreseeable future so I can ship a game |
src/gpu/metal/SDL_gpu_metal.m
Outdated
| while(!((MetalFence *)fences[i])->complete) { | ||
| SDL_WaitCondition(((MetalFence *)fences[i])->condition, ((MetalFence *)fences[i])->mutex); |
There was a problem hiding this comment.
| while(!((MetalFence *)fences[i])->complete) { | |
| SDL_WaitCondition(((MetalFence *)fences[i])->condition, ((MetalFence *)fences[i])->mutex); | |
| while(!fence->complete) { | |
| SDL_WaitCondition(fence->condition, fence->mutex); |
src/gpu/metal/SDL_gpu_metal.m
Outdated
| while(!((MetalFence *)fences[i])->complete) { | ||
| SDL_WaitCondition(((MetalFence *)fences[i])->condition, ((MetalFence *)fences[i])->mutex); | ||
| } | ||
| SDL_UnlockMutex(((MetalFence *)fences[i])->mutex); |
There was a problem hiding this comment.
| SDL_UnlockMutex(((MetalFence *)fences[i])->mutex); | |
| SDL_UnlockMutex(fence->mutex); |
|
yeah, I'm going to be honest. syntax highlighting doesn't work in .mm files on clion so I don't see anything |
syntactic changes
fix missing bracket
src/gpu/metal/SDL_gpu_metal.m
Outdated
| if (windowData->inFlightFences[windowData->frameCounter] != NULL) { | ||
| if (!METAL_WaitForFences( | ||
| driverData, | ||
| (SDL_GPURenderer *)driverData, |
src/gpu/metal/SDL_gpu_metal.m
Outdated
| while (!SDL_GetAtomicInt(&renderer->submittedCommandBuffers[i]->fence->complete)) { | ||
| // Spin! | ||
| } | ||
| METAL_WaitForFences((SDL_GPURenderer *)renderer, true, (SDL_GPUFence **)&renderer->submittedCommandBuffers[i]->fence, 1); |
There was a problem hiding this comment.
Are these casts necessary? You can just pass driverData for the first one.
|
Apologies for the lack of review, I've been very busy and haven't had an opportunity to test this.
At least one app has already passed App Store review on iOS and tvOS with the current implementation. Busy-waiting may not be optimal but it's not prohibitive for shipping games, so I agree with Sam that we should hold off on this until 3.6. |
ok, well that's because max frames in flight also currently does nothing so that issue silences this issue for now, it waits when you call next drawable on CAMetalLayer* after maxing out drawables, so it never ends up waiting on the gpu fence even when frames in flight=1, and the developer didn't use the wait for swapchain api or wait for command buffers in his app |
Co-authored-by: Sam Lantinga <slouken@libsdl.org>
removed redundant casts
4 participants
SDL_GPUFence is implemented as just a busy loop on an atomic int, a SDL_GPUCondition gives the cpu some rest