Composite without timer on swap events
ClosedPublic

Authored by romangg on Nov 14 2019, 8:16 AM.

Details

Summary

When swap events are available do not delay the next repaint by one frame
through the composite timer but directly repaint on swap event.

Test Plan

i915

Diff Detail

Branch
sync-flips
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18780
Build 18798: arc lint + arc unit
romangg requested review of this revision.Nov 14 2019, 8:16 AM
romangg created this revision.
zzag added a subscriber: zzag.Nov 18 2019, 11:42 AM

in AnimationEffect::postPaintScreen. Why?

Could you please post a backtrace?

Maybe I'm misunderstanding this:

For the period between two frames, the first surface that calls damage will trigger a repaint. We'll perform this immediately, then every other update would get queued till after the swap.

So instead of some things being one frame out, literally everything except the first surface is?

composite.cpp
405

Probably as effects call addRepaintFull in there and then you'll call performCompositing again before you've finished the first paint.

We need something queued and here makes sense.

Maybe I'm misunderstanding this:

For the period between two frames, the first surface that calls damage will trigger a repaint. We'll perform this immediately, then every other update would get queued till after the swap.

So instead of some things being one frame out, literally everything except the first surface is?

We talked about this at Qt Contributor Summit already but for reference here: you are not misunderstanding. When the swap event is received a repaint is triggered if damage was accumulated after the last swap. The result is that we have at min a one frame minus some small vblank induced unspecified fraction of a vblank interval delay to at max two frames delay for damage to show up.

With the previous method of the "reverse" frame timer and the block to vblank we had at max a one frame delay if everything worked right on Nvidia on X11. On Mesa and in general on Wayland with the DRM flip event I can not say what delay we had. I guess it was the same we now have.

For our new method the idea is to introduce later a delay timer, which gets started on the vblank event and has an interval length of refreshIntervall - gap where gap is some value we have to guess. On trigger the next paint is started such that we have a minimum damage to present delay of gap and are bound by refreshIntervall + gap.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 12 2019, 12:48 AM
This revision was automatically updated to reflect the committed changes.
romangg marked an inline comment as done.
Restricted Application added a project: KWin. · View Herald TranscriptDec 12 2019, 12:48 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg added inline comments.Dec 12 2019, 12:48 AM
composite.cpp
405

I have added your explanation to the comment and we can later confirm it.

Urgh, no.

I had an unsubmitteed comment for moving forwards.

This patch takes 1 step backwards in order to eventually go 2 steps forward.
A perfectly sensible strategy, but I don't like having master that goes backwards without a completely solid plan to go forward.
The risk of things not getting completed and us releasing something that makes things worse are too high.

But I don't want to hold this up, so in order to resolve this in a way where we can all keep moving forward, I have a proposal:

  • I'll approve it, but we merge this commit as-is into a feature branch, rather than master
  • We can then then do the next review based on that branch
  • We can work, hack, rebase and submit reviews on that branch (and vlad and I can help run and rebase it and whatever)
  • Master stays "sunny"

Then when we have the vsync timer we can merge this all into master. Everyone wins.

Urgh, no.

I had an unsubmitteed comment for moving forwards.

This patch takes 1 step backwards in order to eventually go 2 steps forward.

With this patch we don't go backwards. This was maybe true for the earlier series at 8d13729031 which got rid of all the super old dysfunctional code and after which frames were pushed without alignment on X11 and always with a full frame timer delay even with swap events. But this was merged also a month ago to master and nobody died. The patch series here does fix these issues.

A perfectly sensible strategy, but I don't like having master that goes backwards without a completely solid plan to go forward.
The risk of things not getting completed and us releasing something that makes things worse are too high.

"Perfect" and "completely solid" are unachievable and converging against it has diminishing return. So we need to find a balance between changes going in without any plan and safeguarding things to the point where everything is cemented in and does not move anymore. I am sure we would be at the same point of incomprehensible and faulty compositing as until only few months ago wouldn't I have been willing to take the risk not having implemented everything perfectly from the start.

I had a plan and this one I outlined publicly in task T11071 and the testing ground D23105. I got some feedback and adapted my plan on what I learned. Then I moved forward without further delays.

But I don't want to hold this up, so in order to resolve this in a way where we can all keep moving forward, I have a proposal:

  • I'll approve it, but we merge this commit as-is into a feature branch, rather than master
  • We can then then do the next review based on that branch
  • We can work, hack, rebase and submit reviews on that branch (and vlad and I can help run and rebase it and whatever)
  • Master stays "sunny"

    Then when we have the vsync timer we can merge this all into master. Everyone wins.

Oh, I definitely would have wanted this in master anyways because then Fredrik can base his Nvidia work on it. Since I don't have such direct communication channels to him as to you and Vlad my assumption is that this would lead to unreasonable delay and/or frustration on his side.

If we want to facilitate coordinated feature branch development aside from master we can talk about it, but currently no such facilities besides I guess some company-internal ones at Blue Systems are in place for such.

I am offering a balance and a way to work forward quicker

Working in a branch is a common thing for every project. It doesn't need any new special infrastructure.
From a phab POV it's no different from making a patch against a stable branch.