Implement EGL_KHR_partial_update and EGL_EXT_swap_buffers_with_damage
Needs ReviewPublic

Authored by apol on Mon, Mar 2, 4:30 PM.

Details

Reviewers
None
Group Reviewers
KWin
Plasma: Mobile
Summary

Notify the driver about the parts of the screen that will be repainted.
In some cases this can be benefitial. This is especially useful on lima
and panfrost devices (e.g. pinephone, pinebook, pinebook pro).

Test Plan

Tested on a pinebook pro with a late mesa version.
Basically I implemented it, then it didn't work and I fixed it.
Maybe next step we want to look into our damage algorithm.

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D27788
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24276
Build 24294: arc lint + arc unit
apol created this revision.Mon, Mar 2, 4:30 PM
Restricted Application added a project: KWin. · View Herald TranscriptMon, Mar 2, 4:30 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
apol requested review of this revision.Mon, Mar 2, 4:30 PM

This assumes double buffered, is this always the case?
Otherwise you need to collect sum of all damage across the buffer age.

apol added a comment.Mon, Mar 2, 7:13 PM

This assumes double buffered, is this always the case?
Otherwise you need to collect sum of all damage across the buffer age.

That's what the for loop in egl_gbm_backend.cpp:498 is doing, AFAIU.

apol updated this revision to Diff 76877.Tue, Mar 3, 7:07 PM

Also implement EGL_EXT_swap_buffers_with_damage.

It's also implemented on the lima@pinephone although not in panfrost@pinebookpro.
It's implemented in intel though, and it keeps working fine there, hard to tell if it works any better.

zzag added a subscriber: zzag.Tue, Mar 3, 8:58 PM
zzag added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
558

This is the region in the buffer which must be repaired, i.e. the damage from the previous frames. Shouldn't we also include the current damage region?

561

IIRC, it is okay to set n_rects to 0 so we probably can drop this check.

zzag added inline comments.Tue, Mar 3, 9:00 PM
plugins/platforms/drm/egl_gbm_backend.h
78

The name of this method is confusing. What age?

apol updated this revision to Diff 76896.Wed, Mar 4, 12:38 AM

Properly define the rectangles

apol updated this revision to Diff 76897.Wed, Mar 4, 3:17 AM

Addressed some things, found a way where it doesn't work.
Needs more work.

mwolff added a subscriber: mwolff.Wed, Mar 4, 3:21 PM
mwolff added inline comments.
platformsupport/scenes/opengl/backend.h
331

nit while browsing this interesting patch:

init this member with false too, otherwise this may be left uninitialized (e.g. if initBufferAge isn't called)?

apol updated this revision to Diff 76954.Wed, Mar 4, 4:53 PM

properly calculate rects >.<

apol updated this revision to Diff 76955.Wed, Mar 4, 4:55 PM
apol marked an inline comment as done.

address milian's comment

apol added a comment.Wed, Mar 4, 4:56 PM

addressed

apol added a comment.Wed, Mar 4, 4:57 PM

Now it's working for sure and it's ready to merge.

apol retitled this revision from Implement EGL_KHR_partial_update to Implement EGL_KHR_partial_update and EGL_EXT_swap_buffers_with_damage.Thu, Mar 5, 1:13 AM
zzag added a comment.Thu, Mar 5, 7:11 AM

I am not sure that the region passed to eglSetDamageRegionKHR is correct. The specified region indicates the area to which all rendering commands must be restricted. So, let's say that we need to repaint region A to repair the back buffer and region B is the damage region for the current(next?) frame. So, the resulting damage region which will be passed to eglSetDamageRegionKHR is defined as the union of region A and region B. Currently, we pass only the region that specifies what parts of the back buffer must be repaired. Am I missing something?

I also would like to point out that we don't have control over how many buffers gbm allocates. So, we can't ignore buffer age when determining the area that must be repaired.

apol updated this revision to Diff 77244.Mon, Mar 9, 12:29 AM

upload the version I tested on the pinebook pro

apol updated this revision to Diff 77245.Mon, Mar 9, 1:19 AM

actually use the correct version, arc is doing weird things :/

apol added a comment.Mon, Mar 9, 1:23 AM
In D27788#622423, @zzag wrote:

I am not sure that the region passed to eglSetDamageRegionKHR is correct. The specified region indicates the area to which all rendering commands must be restricted. So, let's say that we need to repaint region A to repair the back buffer and region B is the damage region for the current(next?) frame. So, the resulting damage region which will be passed to eglSetDamageRegionKHR is defined as the union of region A and region B. Currently, we pass only the region that specifies what parts of the back buffer must be repaired. Am I missing something?

I also would like to point out that we don't have control over how many buffers gbm allocates. So, we can't ignore buffer age when determining the area that must be repaired.

You were right, I misused arc somehow and submitted a half-baked last version of the patch. I only realised now when I looked at it and saw that it still had code I had removed.

Sorry about that.

zzag added a comment.Wed, Mar 11, 7:46 AM

I still have concerns about the specified damage region. We only pass the region which must be repainted to repair the back buffer. We probably need to run prePaintScreen() and prePaintWindow() before calling eglSwapBuffersWithDamageEXT() in order to find out what region will be repainted in the current/next frame. Unfortunately, this would require some refactoring.

Please notice that effects such as blur may expand the painted area so we have to run prePaint hooks.

apol updated this revision to Diff 77476.Thu, Mar 12, 2:54 AM

Report the eglSetDamageRegionKHR() at presentOutput so we have information from all effects.
Fixed off by one error.

apol added a comment.Thu, Mar 12, 2:57 AM
In D27788#625614, @zzag wrote:

I still have concerns about the specified damage region. We only pass the region which must be repainted to repair the back buffer. We probably need to run prePaintScreen() and prePaintWindow() before calling eglSwapBuffersWithDamageEXT() in order to find out what region will be repainted in the current/next frame. Unfortunately, this would require some refactoring.

Please notice that effects such as blur may expand the painted area so we have to run prePaint hooks.

I think this is solved with this iteration, thanks for pointing it out, it's something I could see when running a full plasma session.

zzag added a comment.Thu, Mar 12, 7:56 AM

Report the eglSetDamageRegionKHR() at presentOutput so we have information from all effects.

Hmm, the spec says that we should call it before issuing draw commands, i.e. before calling Scene::paintScreen():

After posting the back buffer, the damage region is set to the full dimensions of the surface. The damage region can only be changed by the application before any client API commands that draw to the surface have been made. After this, the damage region is frozen until the back buffer is posted again.

apol updated this revision to Diff 77518.Thu, Mar 12, 7:40 PM

Offer the right damage information and call eglSetDamageRegionEXT() before starting to render things.

zzag added inline comments.Fri, Mar 13, 7:18 AM
plugins/scenes/opengl/scene_opengl.h
88

Stray whitespace after &, please remove it.

scene.cpp
205–206

In general it shouldn't matter, but I wonder whether paintBackground() must be moved down so we don't call glClear() before eglSetDamageRegionKHR().

apol marked 2 inline comments as done.Fri, Mar 13, 12:06 PM
apol updated this revision to Diff 77550.Fri, Mar 13, 12:06 PM

Polishing

zzag added a comment.Fri, Mar 13, 12:18 PM

Overall, looks pretty good to me, but I still have some minor nitpicks.

platformsupport/scenes/opengl/backend.cpp
119

Style: add whitespace before &

platformsupport/scenes/opengl/backend.h
82

Style: remove whitespace after &

plugins/platforms/drm/egl_gbm_backend.cpp
447

I suggest to use (rect.y() + rect.height()) instead of rect.bottom() + 1 because QRect::bottom() deviates from the true bottom border. QRect::right() and QRect::bottom() are good when you need to snap the right or the bottom border of one rectangle to another, e.g.

rect.moveRight(anotherRect.right());

otherwise, it is better to stick with x() + width() and y() + height() to avoid off by one errors.

468

According to the spec, eglSetDamageRegionKHR() handles the case where n_rects is 0, so we can drop rects.isEmpty() check and thus simplify code a bit more.

zzag added inline comments.Fri, Mar 13, 12:44 PM
scene.cpp
144–146

Urgh, this paintBackground() may render something to the back buffer before eglSetDamageRegionKHR() is called.

apol updated this revision to Diff 77553.Fri, Mar 13, 12:49 PM

Calculate differently the rects
Run clang-format

zzag added a comment.Fri, Mar 13, 1:16 PM

Uh, there is a problem with calling aboutToStartPainting() from paintGenericScreen(). Namely, effects such as Desktop Grid and Slide may call EffectsHandler::paintScreen() more than once to render each individual virtual desktop.

apol added inline comments.Fri, Mar 13, 2:52 PM
scene.cpp
144–146

Tried adding an aboutToStartPainting call here but it errors out constantly.

Also expose effects work just fine as is.

zzag added a comment.EditedMon, Mar 16, 8:33 AM

The key point about EGL_KHR_partial_update is that the compositor knows what parts of the back buffer are going to be updated. So the compositor could specify the damage region _beforehand_ recording command buffers. If the compositor doesn't have a sophisticated effects pipeline, e.g. weston, then it's pretty trivial to determine the buffer damage, but that's not the case for us, unfortunately.

Ideally, a compositing cycle must be split into several phases - pre paint, paint, and maybe post paint. In the pre paint phase, the compositor would do necessary bookkeeping so we know what the damage region for the next frame is.

I really like the idea of having a hook that is called when the scene is about to start recording a command buffer for the next frame, but given the current design of the effects pipeline I am starting to think that we cannot support EGL_KHR_partial_update at the moment. The dirty region for the next frame is computed too late and I don't see how we can fix it because effects have the final say in what area of the screen will be repainted. :/

I would love to be proven wrong, though.

apol added a comment.Mon, Mar 16, 3:41 PM
In D27788#628077, @zzag wrote:

I really like the idea of having a hook that is called when the scene is about to start recording a command buffer for the next frame, but given the current design of the effects pipeline I am starting to think that we cannot support EGL_KHR_partial_update at the moment. The dirty region for the next frame is computed too late and I don't see how we can fix it because effects have the final say in what area of the screen will be repainted. :/

Why too late? everything works, at least in practice.
We might want to add some API to make sure effects don't paint before they're allowed to but just resigning to have a worse throughput to be able to paint on prepaint feels wrong.

I would love to be proven wrong, though.

I don't know what proof looks here.

zzag added a comment.Tue, Mar 17, 8:21 AM
In D27788#628469, @apol wrote:

Why too late? everything works, at least in practice.

Yes, but it's possible to call aboutToStartPainting() after rendering something, which conceptually doesn't seem right.

Ideally, we would determine the damage region for the next frame by just running prePaintScreen() hooks, but we can't really do that because we need also to run prePaintWindow() hooks, which means we need to run some parts of paintSimpleScreen() and paintGenericScreen().

We might want to add some API to make sure effects don't paint before they're allowed to but just resigning to have a worse throughput to be able to paint on prepaint feels wrong.

No, we don't need such an API. When a pre-paint hook is called, an effect is allowed only to schedule repaints or update its internal state.

I don't know what proof looks here.

Heh, it would be a patch where we call eglSetDamageRegionKHR() before any rendering starts and after we ran prePaintScreen() and prePaintWindow() hooks. :-)

he dirty region for the next frame is computed too late and I don't see how we can fix it because effects have the final say in what area of the screen will be repainted. :/

Those effects will set the transform flag which means we'll be in paintGenericScreen which marks the whole screen as dirty anyway.
As long as that happens, we don't need them to precalculate the correct geometry.

apol updated this revision to Diff 78564.Thu, Mar 26, 3:56 PM

rebase

apol added a comment.Thu, Mar 26, 3:59 PM

he dirty region for the next frame is computed too late and I don't see how we can fix it because effects have the final say in what area of the screen will be repainted. :/

Those effects will set the transform flag which means we'll be in paintGenericScreen which marks the whole screen as dirty anyway.
As long as that happens, we don't need them to precalculate the correct geometry.

That would explain why we haven't seen any artefacts coming from these effects.