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

Add support for commit-timing-v1. no wlroots #1230

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

Conversation

sergio-gdr
Copy link

Second WIP attempt at bringing support for commit-timing-v1 to gamescope.
Previous attempt uses wlroots' internal infrastructure.

The protocol offers 2 target stages: presentation and latching. I've ignored the distinction for now and just take the provided timestamp to be intended for "presentation".
I've also not tried to work through how a timestamp request would interact with the current implementation of fifo-v1 (something for a future iteration).

@sergio-gdr sergio-gdr marked this pull request as draft April 7, 2024 00:31
src/wlserver.cpp Outdated

#define NANOS_IN_SEC 1000000000
static uint64_t timer_get_target_present_nsec(uint64_t timestamp_nsec, uint32_t rounding_mode) {
uint64_t refresh_nsec = NANOS_IN_SEC/(GetVBlankTimer().GetRefresh()/1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. We have utilities to do refresh -> cycle already mHzToRefreshCycle

  2. Doesn't all this rounding logic need to actually be where we check the timestamp in steamcompmgr?

Right now this problem will only manifest on hotplug, which isn't a big deal -- but some day we will support multiple displays, so we should move it there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I know that adding stuff to the ResListEntry whatever crap is painful, it is high on my to-do to rework that garbage soon and have wlserver create the commits directly).

Copy link
Author

Choose a reason for hiding this comment

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

Hope it's better now.

src/wlserver.cpp Outdated
target_nsec -= (target_nsec % refresh_nsec);
}
else if (rounding_mode == TIMER_ROUNDING_NOT_BEFORE) {
target_nsec = ((target_nsec+(refresh_nsec-1))/refresh_nsec)*refresh_nsec;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get why we are touching anything for TIMER_ROUNDING_NOT_BEFORE.

There is no world where we need to normalize it to a refresh cycle, and we probably shouldn't because multiple displays.

Copy link
Author

Choose a reason for hiding this comment

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

Moved this logic to steamcompmgr, leaving out this case.

src/wlserver.cpp Outdated

for (const auto& commit : *commits)
{
if (commit.desired_present_time > target_present_nsec)
Copy link
Collaborator

@Joshua-Ashton Joshua-Ashton Apr 18, 2024

Choose a reason for hiding this comment

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

This logic doesn't work because some pending commits have already probably moved to steamcompmgr thread already.

Unfortunately Gamescope is not set up to really allow you to check something like this given it's architecture.
I am happy to just drop the implementation of the protocol error for now.

Copy link
Collaborator

@Joshua-Ashton Joshua-Ashton Apr 18, 2024

Choose a reason for hiding this comment

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

Actually, you could just store the highest used target_present_nsec that gets updated at commit time on the wlserver_wl_surface_info instead of checking every commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also probably do that for your actual wlroots implementation instead of checking over every commit ;)

Copy link
Author

Choose a reason for hiding this comment

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

Huh, nice...
From now on whenever I find myself mindlessly proliferating cpu cycles, I'll ask myself if I could do any better :D

However I think I'll just leave a very trivial check for now, since it appears to be controversial what to actually consider an error in this case.

Signed-off-by: Sergio Gómez <sergio.g.delreal@gmail.com>
There might be other done commits which would be lost otherwise.

Signed-off-by: Sergio Gómez <sergio.g.delreal@gmail.com>
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.

None yet

2 participants