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

wayland_vk: use FIFO if commit-timing and fifo protocols are available #15056

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

Conversation

Dudemanguy
Copy link
Member

A very long time annoyance with wayland was compositors indefinitely blocking our vo thread if the surface gets occluded in some way. We've worked around this by using mailbox and our own custom vsync function. Thankfully it looks like people are finally solving this and with these two protocols it should be possible to guarantee forward progress on vulkan which means all the workarounds we do shouldn't be needed. So we can just request fifo in this case as a default since all we want is standard vsync blocking.

It's not tested at the moment, but SDL essentially does the same thing.

@llyyr
Copy link
Contributor

llyyr commented Oct 11, 2024

We should probably wait a few months after a new Mesa release is cut with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/26150

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Oct 11, 2024

I'm hoping compositor implementations would only signal clients if it knows the drivers can actually use the protocols. But I didn't really look at how it was implemented. We still have to wait for the next wayland-protocols release anyway.

@Dudemanguy Dudemanguy force-pushed the wayland-fifo branch 3 times, most recently from 043de5b to 3113c9a Compare October 12, 2024 04:49
@mahkoh
Copy link
Contributor

mahkoh commented Oct 12, 2024

I'm hoping compositor implementations would only signal clients if it knows the drivers can actually use the protocols.

There is no way for compositors to do this. I've ranted at length about this here: libsdl-org/SDL#10937 (comment)

I also don't see why the compositor should be in charge of telling an application what features a library supports. If application A wants to know what features library B supports, then it should ask library B instead of compositor C.

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Oct 12, 2024

Hmm that's unfortunate. I guess that means this will just have to wait some arbitrary time after the next mesa release. I guess the semantics of --wayland-disable-vsync should change a bit so there's a "force on" option as a fallback in case someone is on some really weird configuration like newish compositor version but old mesa.

@Dudemanguy Dudemanguy force-pushed the wayland-fifo branch 2 times, most recently from f8234a9 to e84aa5e Compare October 12, 2024 15:50
@Dudemanguy
Copy link
Member Author

Dudemanguy commented Oct 12, 2024

Reworked --wayland-disable-vsync to a --wayland-internal-vsync option so users have full control of the vsync behavior. Since 1.38 is out now, I bumped that as well so this is just blocked on the next mesa release as far as I'm concerned. We'll see what the final implementation will be but for now I also made using fifo depend on having wp_presentation_v2.

Copy link

github-actions bot commented Oct 12, 2024

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy Dudemanguy force-pushed the wayland-fifo branch 2 times, most recently from a8a0070 to 9133df2 Compare October 14, 2024 23:43
With the upcoming fixes to FIFO in wayland, it should be preferable to
use FIFO instead of our own hacky heuristic. This means
--wayland-disable-vsync should become a tristate option with an "auto"
behavior (not implemented in this commit). The semantics have to
slightly change so introduce --wayland-internal-vsync and deprecate
--wayland-disable-vsync.
@Dudemanguy Dudemanguy force-pushed the wayland-fifo branch 2 times, most recently from 6471b74 to 0841139 Compare October 15, 2024 23:07
@Dudemanguy
Copy link
Member Author

It looks like the mesa implementation will work with presentation time v1 so I loosened up the restriction a bit.

@llyyr
Copy link
Contributor

llyyr commented Oct 16, 2024

It looks like the mesa implementation will work with presentation time v1 so I loosened up the restriction a bit.

it'll work but only for fixed refresh rate outputs. It'll go back to the old behavior as soon as VRR is enabled. See: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/26150/diffs?commit_id=e7aa09688bf20a5b4e664437f302762be606c783#8b9326d5e797a10c9d7bf33ddb5420c06e700768_2135_2270

implementing v2 is trivial for any compositor that'll go through the trouble of implementing fifo-v1 and commit-timing-v1, so I don't see any issue with requiring all 3

@Dudemanguy
Copy link
Member Author

Added it back in.

* if the surface is no longer visible. This means using FIFO would block
* the entire vo thread which is just not good. Use MAILBOX for those
* compositors to avoid indefinite blocking. */
p->use_fifo = wl->has_commit_timing && wl->has_fifo && wl->present_v2 && wl->use_present;
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually segfaults because wl is still null here

Copy link
Contributor

@llyyr llyyr Oct 21, 2024

Choose a reason for hiding this comment

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

This is still not sufficient because use_present is false until the first frame callback, which is too late for this check

A very long time annoyance with wayland was compositors indefinitely
blocking our vo thread if the surface gets occluded in some way. We've
worked around this by using mailbox and our own custom vsync function.
Thankfully it looks like people are finally solving this and with these
two protocols it should be possible to guarantee forward progress on
vulkan which means all the workarounds we do shouldn't be needed. So we
can just request fifo in this case as a default since all we want is
standard vsync blocking.
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.

4 participants