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

RenderingDevice: Wait for present if supported (Vulkan Windows/X11/Wayland - needs testing) #94973

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alvinhochun
Copy link
Contributor

Naive attempt to extend #94960 to support Vulkan using the VK_KHR_present_wait extension. I have no idea how functional this is since I cannot test it correctly. (The only environment I can test this on is Windows + NVIDIA Optimus, which doesn't show any real improvements.)

It needs to be especially tested with multiple windows on separate monitors, with different refresh rates.

To verify that present wait is in use, run with --verbose and check for VK_KHR_present_wait.

@alvinhochun alvinhochun force-pushed the rd-wait-for-present branch from 4902c3e to f04ea48 Compare July 31, 2024 10:06
@alvinhochun alvinhochun changed the title RenderingDevice: Wait for present if supported (Vulkan X11/Wayland - needs testing) RenderingDevice: Wait for present if supported (Vulkan Windows/X11/Wayland - needs testing) Jul 31, 2024
@alvinhochun alvinhochun force-pushed the rd-wait-for-present branch from f04ea48 to dc42a87 Compare July 31, 2024 10:20
return;
}

uint64_t wait_present_id = swap_chain->present_id - latency;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be subtracting latency here? Shouldn't we just use the present id as given? The documentation for vkWaitForPresentKHR says:

vkWaitForPresentKHR waits for the presentId associated with swapchain to be increased in value so that it is at least equal to presentId.

So if wait_present_id is too low, presumably vkWaitForPresentKHR won't perform the wait, thus we won't see an improvement in latency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it uses the latest present id then it will only ever be queueing one present image in the swap chain. I may be wrong here but my understanding is that, when swapchain_image_count is set to, say 3, one is always held as the front buffer, the remaining 2 images can be acquired, drawn onto and queued for present. We want to queue two images, but only wait until the first in queue is presented.

Though this made me realize the code actually need to subtract one from the latency to get the desired effect.

@KeyboardDanni
Copy link
Contributor

Unfortunately it looks like support for VK_KHR_present_wait is even worse than for MAILBOX_KHR. Apparently only nVidia on Windows/Linux will have access to this extension for the most part. At least, for now?

Is there a way we could take the semaphore/fence from vkAcquireNextImageKHR and wait on that instead? Or is that a bad idea? Right now we're waiting on the semaphore just before command queue submission.

@Calinou
Copy link
Member

Calinou commented Jul 31, 2024

Unfortunately it looks like support for VK_KHR_present_wait is even worse than for MAILBOX_KHR. Apparently only nVidia on Windows/Linux will have access to this extension for the most part. At least, for now?

NVIDIA on Linux does not support mailbox V-Sync (at least in my experience on X11), so I'd still appreciate having support for VK_KHR_present_wait.

I get this on 555.58.02:

  drivers/vulkan/rendering_device_driver_vulkan.cpp:2662 - The requested V-Sync mode Mailbox is not available. Falling back to V-Sync mode Enabled.

@KeyboardDanni
Copy link
Contributor

Hmm, yeah, I checked again and it looks like some nVidia configurations support it, and some don't on Linux, even on newer drivers. The ones that do indicate support seem to be GTX 900 series or newer, mostly. I wonder if it's only available in Wayland?

@alvinhochun
Copy link
Contributor Author

There should also be some kind of support on X11 since https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19279, and it seems since https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27275 VK_KHR_present_wait should be also supported by Mesa on most if not all Wayland compositors. But it looks like this extension is currently being hidden behind some kind of dri option vk_khr_present_wait? I have no idea how it is supposed to work.

@alvinhochun
Copy link
Contributor Author

alvinhochun commented Aug 1, 2024

Seems to be working fine here on Intel HD 5500 with a slightly patched Mesa driver, both X11 and Wayland.

@KeyboardDanni
Copy link
Contributor

Can you try this with my latency tester app? https://github.com/KeyboardDanni/godot-latency-tester

@alvinhochun
Copy link
Contributor Author

Can you try this with my latency tester app? https://github.com/KeyboardDanni/godot-latency-tester

Tested it with KWin + KDE Plasma on X11. Mind that I don't know if the cursor is hardware or not.

I am using system Mesa 24.1.4 to test without present wait, and Mesa main branch to test with present wait,

(Doesn't work on Wayland - it doesn't move the cursor.)

Screenshots

KWin (X11) with compositing, windowed, without present wait:

vlcsnap-x11-compositing-no-wait

KWin (X11) with compositing, windowed, with present wait:

vlcsnap-x11-compositing-wait

KWin (X11) without compositing, windowed, without present wait:

vlcsnap-x11-no-compositing-no-wait

KWin (X11) without compositing, windowed, with present wait:

vlcsnap-x11-no-compositing-wait

KWin (X11) without compositing, fullscreen, without present wait:

vlcsnap-x11-no-compositing-fs-no-wait

KWin (X11) without compositing, fullscreen, with present wait:

vlcsnap-x11-no-compositing-fs-wait

@KeyboardDanni
Copy link
Contributor

These are some excellent gains on X11! This should probably also be tested with some amount of CPU + GPU load. I'll see if I can incorporate this into the tester.

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

Successfully merging this pull request may close these issues.

3 participants