Use wlr_frame_scheduler #7780
Use wlr_frame_scheduler #7780
Conversation
| @@ -747,7 +746,6 @@ static void handle_frame(struct wl_listener *listener, void *user_data) { | |||
| if (delay < 1) { | |||
| output_repaint_timer_handler(output); | |||
| } else { | |||
| output->wlr_output->frame_pending = true; | |||
There was a problem hiding this comment.
Draft because dropping this bool flag isn't the right thing to do. We probably want to implement our own custom frame scheduler instead.
emersion
force-pushed
the
frame-scheduler
branch
from
b376596
to
625c7d7
Compare
|
This is impressive! 🙂 I know it's still work in progress but I found some interesting takeaways after comparing it back and forth a couple of times. In comparison to Sway 1.8.1, here's bugs which are not present in the current state of this PR:
Differences:
Problems
Setup:
I was hoping to provide more comprehensive numbers with GPUVIS but I haven't figured out good filters to make sense of the data. |
|
That sounds like unintended consequences. In theory once everything is properly implemented these changes shouldn't change the current behavior at all. |
emersion
force-pushed
the
frame-scheduler
branch
from
625c7d7
to
23df132
Compare
emersion
force-pushed
the
frame-scheduler
branch
4 times, most recently
from
46e8ff4
to
92ad664
Compare
|
This is now feature-complete. |
|
Christmas came early this year! I've been using this for a couple of hours now and I'm very happy with how it works. FeedbackMy setupSpecifications:
Kernel arguments: # AMD (EEP)
'amd_pstate.shared_mem=1'
'amd_pstate=active'
'amd_prefcore=enable'
# GPU (VRR)
'video=DP-1:3840x1600@144'
'amdgpu.freesync_video=1'
# Performance
'mitigations=off'
'gpu_sched.sched_policy=0' # https://gitlab.freedesktop.org/drm/amd/-/issues/2516#note_2119750
'preempt=full'What has changed?In comparison to Sway 1.8.1: Improvements:
Noteworthy:
Problem 1 - new "Failed to page-flip output" (error)
Problem 2 - can't toggle VRR on-the-fly (crash)Toggling VRR causes compositor to crash instantly even though VRR is successfully activated.
Problem 3 - refresh rate isn't always updatedMonitor continues to use the latest refresh rate instead of using the one specified in the config file when following these steps:
|
|
Thanks for testing, however I'd like to reiterate that this PR isn't supposed to improve anything. This PR just replicates the existing Sway behavior with new APIs. IOW, it's a bug if this PR doesn't behave exactly as Sway master. |
emersion
force-pushed
the
frame-scheduler
branch
from
92ad664
to
49a6bb9
Compare
emersion
force-pushed
the
frame-scheduler
branch
from
49a6bb9
to
31c891d
Compare
|
I have been running wlroots master branch with the wlroots MR, and a modified version of this PR on top of sway master for about a week and it works great. I am not using the I haven't really noticed any major issues, and I can confirm that the bug with VRR not working when using direct scanout is fixed. I do not need to start sway/wlroots with any environment variable for it to work. I still encounter the issue where moving the mouse cursor will break VRR, but I assume fixing it requires more work than just changing the frame scheduler. If you still consider the change in behavior a bug, I can try to look into why the new frame scheduler fixes it, although I am not too familiar with the frame scheduling logic nor the drm part. SpecsDistro: GPU: |
|
@Nogesma mouse movement doesn't actually break VFR, sway cursors are redrawn every time the mouse reports movement, which I assume, you with your 360hz monitor, you also have a gaming mouse doing 500hz/1000hz, if you set your mouse to 125hz polling it should drive the display at ~125hz, Sway treats all updates equally, I wrote a patch for wlr and sway to defer cursor updates to some max latency. if you want to try it. patch-info: #6832 (comment) |
|
AMDGPU has some funny quirks where updating cursor position does not seem to drive the display faster and so cursor movements stutter at a lower refresh, but again note that this PR should have no functional changes. |
| struct send_frame_done_data frame_done_data = {0}; | ||
| clock_gettime(CLOCK_MONOTONIC, &frame_done_data.when); | ||
| frame_done_data.msec_until_refresh = get_msec_until_refresh(event); | ||
| send_frame_done(output, &frame_done_data); |
There was a problem hiding this comment.
It seems strange for the compositor's handle_present() to also need to call get_msec_until_refresh() in order to determine the frame callback delay. get_msec_until_refresh() uses output->max_render_time, which is specific to the timed_frame_scheduler implemented here. For a generic frame scheduler, we don't seem to expose how long this delay should be, and assuming output->max_render_time seems wrong.
- Should frame callback timers be set by the scheduler in its present function instead?
- If it makes more sense to continue setting the callbacks on the compositor side, maybe this is better addressed in the frame scheduler MR?
I know this is on the back-burner, and things have changed a bit in the wlr_output events since the last rebase, so feel free to ignore if this no longer makes sense.
There was a problem hiding this comment.
It seems strange for the compositor's handle_present() to also need to call get_msec_until_refresh() in order to determine the frame callback delay
Why do you think so? handle_present notifies us of when presentation occurred some time after the fact and is not representative of the actual time of presentation. We need to do a bit of math to figure out how much time is actually left at this point, using the reported time of presentation and estimated time from there till the next frame.
get_msec_until_refresh()usesoutput->max_render_time
Hmm? It doesn't in the PR as is. output->max_render_time is applied by timed_frame_scheduler itself to offset render from the value reported by get_msec_until_refresh.
For a generic frame scheduler, we don't seem to expose how long this delay should be
I believe the intent was to later implement a "fancy" frame scheduler that dynamically decides on a suitable delay, which would more or less deprecate max_render_time.
There was a problem hiding this comment.
Hmm? It doesn't in the PR as is. output->max_render_time is applied by timed_frame_scheduler itself to offset render from the value reported by get_msec_until_refresh.
Whoops, you're right, output->max_render_time is used by send_frame_done_iterator not in get_msec_until_refresh. That got mixed up at some point between this implementation, master, and my own patch 😅
Same point though, output->max_render_time is used outside of the frame scheduler.
I believe the intent was to later implement a "fancy" frame scheduler that dynamically decides on a suitable delay, which would more or less deprecate max_render_time.
We're on the same page here. There are technically 2 timings a frame scheduler implementation might want to configure separately. We're using next_refresh - max_render_time for both right now (ignoring view->max_render_time), but there is:
- A) the timing of the frame event (which triggers rendering for the compositor)
- B) the timing of the frame callbacks being called (which triggers rendering for clients)
(A) is already being handled entirely within this frame scheduler. My question is about how (B) should be handled by the frame scheduler, since in this implementation it's not.
Edit: Changed "delay from the present event" to "timing"
There was a problem hiding this comment.
Having messed around with implementing a frame scheduler for a bit, I now realize that it doesn't make sense for (B) to be implemented in wlr_frame_scheduler. Even though it is very similar to (A), it is probably more appropriate to handle per-surface, rather than per-output.
So I guess this PR doesn't need to address that, sorry for the noise.
Update for https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4307