RFC: implement commit-timing-v1 and fifo-v1 - #2797
Conversation
ea85282 to
62753b0
Compare
|
We definitely want to get fifo and commit-timing working. Though we do have to be careful here since if we fail to signal a fifo barrier for a particular type of surface in any circumstance, it could hang indefinitely. |
yep. I noticed that your original The main purpose of this PR is to investigate whether the commit-timing implementation has reached a sufficient level of correctness, even though its timing precision still needs improvement. It may also provide a good opportunity to use it as a reference when investigating where it differs from your original approach. |
62753b0 to
fd272c0
Compare
|
Following the suggestion, this no longer needs to depend on updates to smithay. |
08c2751 to
2dafd9d
Compare
|
@ids1024 I’ve updated the |
2db3184 to
af2c2be
Compare
|
commit-timing-v1 requirement:
I implemented a frame-time-based early commit strategy. |
|
Not having an extra place or two where we iterate over all types of surfaces is definitely good. |
| fn init_fifo(&self) { | ||
| self.user_data() | ||
| .insert_if_missing_threadsafe(|| FifoBarriers(Mutex::new(Vec::new()))); | ||
| } |
There was a problem hiding this comment.
Isn't this redundant if fifo_barrier() also calls insert_if_missing_threadsafe()?
| user_data.insert_if_missing_threadsafe(|| FifoBarriers(Mutex::new(Vec::new()))); | ||
| if let Some(barriers) = user_data.get::<FifoBarriers>() |
There was a problem hiding this comment.
insert_if_missing_threadsafe and get can be combining into get_or_insert_threadsafe.
| user_data.insert_if_missing_threadsafe(|| AvgFrameTime(RwLock::new(None))); | ||
| if let Some(avg_frametime) = user_data.get::<AvgFrameTime>() { |
There was a problem hiding this comment.
This likewise can use get_or_insert_threadsafe.
| } | ||
| }); | ||
|
|
||
| add_pre_commit_hook::<Self, _>(surface, move |state, _dh, surface| { |
There was a problem hiding this comment.
It's probably unnecessary to add three different pre-commit hooks to each surface in new_surface().
Maybe a single hook that could invoke helper functions for the different things it does?
|
|
||
| impl State { | ||
| fn schedule_commit_timing(&self, surface: &WlSurface, deadline: Timestamp) { | ||
| const SAFE_MARGE: Duration = Duration::from_millis(1); |
There was a problem hiding this comment.
Is MARGE here a "margin"? If so presumably SAFE_MARGIN would be clearer. Unless I misunderstand this.
Is there any particular logic behind the 1ms value, or is it just an extra margin that seemed to work well based on testing?
There was a problem hiding this comment.
yeah, it should be SAFE_MARGIN.
commit-timing requires us not to present frames early, so this 1 ms is a tolerance designed to respect that requirement. We can improve this part further in the future.
|
Storing the fifos to signal with the output seems good. I see it does signal fifos on output removal, which should cover things; either the output will be presented or it will be removed. If a change of primary scanout output happens after setting the barrier it will still wait on the previous output? But that's probably fine. Commit timing is a protocol I'm a little less familiar with.
So is the testing of commit timing mainly just enabling the frame rate limiter and a frame rate overlay and seeing that it seems to report the frame rate it should? Are there any good clients to test/measure how well it is doing more precisely? I suppose comparing requested commit timings to |
work fine in my test, I think the current state of fifo-v1 looks good.
I'm digging deeper into DXVK's design to better understand client-side behavior. Here's how DXVK describes its present timing feature:
To achieve this, DXVK maintains its own internal timeline algorithm and continuously updates it dynamically. I'm still looking for the most appropriate approach to accommodate this kind of client-side behavior. If you have any suggestions, I'd really appreciate it.
I recommend: https://github.com/Themaister/Granite/blob/master/tests/present_timing.cpp the tool is great. It allows you to toggle VSync (FIFO), enable or disable and configure time requests (commit-timing), adjust GPU load, and toggle VRR. Since this involves frame pacing, it is relevant to #2420 |
Yeah, this version of fifo looks fairly safe. Commit-timing is a little more complicated. I'm not familiar with the details of optimal frame pacing, but I know it can be a complicated matter. Probably this version at least a good start and it can be improved later. Though it would be good to see some specific metrics for how well it is working. |
After integrating #2420, I think commit-timing performs reasonably well in fullscreen mode. the reason I integrate this is that the PR prevents commits from other non-fullscreen surfaces from interfering with the scheduling, which can affect frame pacing. here are some screenshots I can use to explain the current situation: The graph below shows the result with commit-timing and FIFO enabled, VRR disabled, and the commit-timing interval set to 10.417 ms. The average frame time is 12.379 ms, which means the presentation is occurring around every third vblank, since 4.167 ms × 3 ≈ 12.5 ms. The graph below shows the result with VRR enabled and the commit-timing interval set to 14.063 ms. The average frame time is 14.064 ms. If you're interested in testing this as well, I've prepared the branch I used for you to try: https://github.com/skygrango/cosmic-comp/tree/commit-timing_fifo_with_fullscreen_patch |
|
A clarification: |
|
Probably best to keep it separate and update #2420 after this one is merged. |
afb1a39 to
b12c0ae
Compare
|
Rebase and cleanup. I don't have an NVIDIA 20–50 series GPU, which supports the open-source driver, so I haven't been able to test this on those GPUs. I've done extensive testing with my AMD GPU, though, and based on those results, I think the current state is good enough for me. Also, since NVIDIA GPUs are currently forced to use edit: I noticed something a bit strange. Let me investigate it a little further. |
|
I went back and tested FIFO on its own again, and I realized that FIFO wasn't actually making the window's update rate follow the display refresh rate. I had forgotten that we only need to process the current barrier, not pending barrier. Otherwise, we would signal all the barriers at once, which defeats the purpose of FIFO. I've updated the FIFO implementation accordingly. Could you please take another look and verify it? This time, I'm confident that the window's frame rate is actually being limited by the vblank. |


related: #1549
The main purpose of this PR is to verify that the
commit-timing-1andfifo-1functionality work correctly.Test conditions:
wp_commit_timer_v1andwp_fifo_v1Note: if someone want to test DXVK game, you need custon proton: https://github.com/NelloKudo/proton-dev/actions/runs/33871680219 and
PROTON_PRESENT_TIMING=1 PROTON_ENABLE_WAYLAND=1logs sample:
need: Smithay/smithay#2149The purpose of this PR is to serve as a proof of concept (POC) for validating theNo longer need to update smithay.schedule_barrierhandler I added to Smithay.TheNo longer need to lookupOutputSurfaceintroduced in this PR is optional rather than required. It is only used to make it easier for me to iterate on and test the output/window behavior during development, and is not intended to be a fundamental requirement of the approach.