Implement EGL_KHR_partial_update and EGL_EXT_swap_buffers_with_damage
Needs ReviewPublic

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

Details

Reviewers
zzag
bshah
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 27024
Build 27042: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
zzag added a comment.EditedMar 16 2020, 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.Mar 16 2020, 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.Mar 17 2020, 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.Mar 26 2020, 3:56 PM

rebase

apol added a comment.Mar 26 2020, 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.

zzag added a comment.Mar 30 2020, 2:21 PM

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.

This is still not a good reason for pushing the current patch as is. The current API of aboutToStartPainting() suggests that it's called before KWin starts recording rendering commands and after determining what parts of the screen are going to be updated, but it's not entirely true because the scene may call paintBackground() before aboutToStartPainting().

apol added a comment.Mar 30 2020, 2:36 PM
In D27788#638127, @zzag wrote:

This is still not a good reason for pushing the current patch as is. The current API of aboutToStartPainting() suggests that it's called before KWin starts recording rendering commands and after determining what parts of the screen are going to be updated, but it's not entirely true because the scene may call paintBackground() before aboutToStartPainting().

Would you prefer another name? How about prepaintDone?

zzag added a comment.Mar 31 2020, 7:11 AM
In D27788#638130, @apol wrote:

Would you prefer another name? How about prepaintDone?

No, we need to rework the implementation of PAINT_SCREEN_BACKGROUND_FIRST. One way to do it is to introduce a counter (e.g. m_paintScreenInvocationCount) and call paintBackground() in paintGenericScreen() or paintSimpleScreen() when the counter is 1.

apol updated this revision to Diff 79009.Mar 31 2020, 6:57 PM

Included Vlad's patch.

Still works on the Pinebook Pro and Intel.

zzag added inline comments.Apr 16 2020, 11:12 AM
scene.cpp
234–235

So, when the slide effect is active, aboutToStartPainting() may be called up to 4 times. We have to check m_paintScreenCount before calling aboutToStartPainting().

358–363

aboutToStartPainting() should not be called twice.

apol updated this revision to Diff 80669.Apr 20 2020, 5:14 PM

address zzag's comment

apol marked 2 inline comments as done.Apr 20 2020, 5:16 PM

+1

Consider it a +2 if there's no other comments

zzag added inline comments.Apr 23 2020, 5:24 PM
scene.cpp
235

You must call paintBackground if PAINT_SCREEN_BACKGROUND_FIRST is called.

235

is set*

240

In general, an effect is allowed to call paintScreen() multiple times without setting PAINT_SCREEN_BACKGROUND_FIRST.

apol updated this revision to Diff 81120.Apr 24 2020, 5:23 PM

rebased

apol marked an inline comment as done.Apr 24 2020, 5:33 PM
apol added inline comments.
scene.cpp
235

This happens in Scene::paintScreen.

apol updated this revision to Diff 81122.Apr 24 2020, 5:33 PM

Removed assert

zzag requested changes to this revision.Apr 27 2020, 6:57 AM
zzag added inline comments.
scene.cpp
235

But this means that paintBackground() will be called before abotToStartPainting(). The whole purpose of m_paintScreenCount is to ensure that we can move paintBackgroudn() from Scene::paintScreen() to Scene::paintGenericScreen() and Scene::paintSimpleScreen().

This revision now requires changes to proceed.Apr 27 2020, 6:57 AM
apol updated this revision to Diff 81469.Apr 28 2020, 7:39 PM

Address @zzag's comment when on paintGenericScreen and bg needs painting.

apol marked an inline comment as done.Apr 28 2020, 7:39 PM
zzag added inline comments.Apr 29 2020, 5:47 AM
plugins/platforms/drm/egl_gbm_backend.cpp
441

s/lastRegion/region/

465

damagedRegion contains the region that will be repainted in the next frame, so we don't need to accumulate surface damages again.

I suppose we want something like

const QRegion region = damagedRegion & output.output->geometry();
472

Use categorized logging please. Also, it's not a critical error if eglSetDamageRegionKHR() fails. qCWarning(KWIN_DRM) would be a better choice.

539–551

I think we can drop this method now since only EglGbmBackend::prepareRenderingForScreen() needs to accumulate damage.

zzag added inline comments.Apr 29 2020, 5:50 AM
scene.cpp
616

Usually we don't put a semicolon after Q_UNUSED.

apol marked 4 inline comments as done.Apr 29 2020, 11:03 AM
apol added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
539–551

I think next person who reads the code will prefer to see it encapsulated.

apol updated this revision to Diff 81496.Apr 29 2020, 11:04 AM

address @zzag's comments

zzag requested changes to this revision.Apr 29 2020, 11:24 AM
zzag added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
465

This comment hasn't been addressed.

539–551

Currently pendingDamageRegions() is used only in two places - aboutToStartPainting() and prepareRenderingForScreen(). There is no reason to accumulate surface damages _again_ in aboutToStartPainting().

After the second usage of pendingDamageRegions() is dropped, the introduction of pendingDamageRegions() will look unrelated.

This revision now requires changes to proceed.Apr 29 2020, 11:24 AM
apol updated this revision to Diff 81504.Apr 29 2020, 12:59 PM
apol marked an inline comment as done.

Refactor as suggested

zzag accepted this revision.Apr 29 2020, 1:51 PM

Thanks.

Note that I don't have hardware to test your patch, but overall the patch looks correct.

platformsupport/scenes/opengl/backend.cpp
121

You missed this one ;-)

No semicolon after Q_UNUSED.

This revision is now accepted and ready to land.Apr 29 2020, 1:51 PM
bshah requested changes to this revision.May 4 2020, 5:20 AM
bshah added a subscriber: bshah.

It seems to be broken on pinephone, it however wasn't broken on pinebook earlier, which makes me think it's probably related to scaling as pinephone have 2x scaling factor

This revision now requires changes to proceed.May 4 2020, 5:20 AM
zzag added a comment.May 6 2020, 7:38 AM

It seems to be broken on pinephone, it however wasn't broken on pinebook earlier, which makes me think it's probably related to scaling as pinephone have 2x scaling factor

Heh, that's right! We pass the damage region to aboutToStartPainting() in device-independent pixels.

apol updated this revision to Diff 82888.May 14 2020, 11:43 PM

Address scaling

apol added a comment.May 14 2020, 11:46 PM


pinebook pro with 2x

zzag requested changes to this revision.May 15 2020, 6:10 AM
zzag added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
441

The region is in the global screen coordinates, but the buffer damage needs to be in the output local coordinates.

This revision now requires changes to proceed.May 15 2020, 6:10 AM
apol updated this revision to Diff 82923.May 15 2020, 11:00 AM

Make sure the rects are always in the right coordinate space and properly scaled

zzag added inline comments.May 15 2020, 11:48 AM
plugins/platforms/drm/egl_gbm_backend.cpp
444

It would be nice to map rects from the global coordinate space to the output coordinate space and scale them both at the same time to avoid an extra for loop and save a couple of memory allocations.

apol updated this revision to Diff 82939.May 15 2020, 2:01 PM

Do not allocate a full QRegion on every paint.

bshah added a comment.May 19 2020, 7:17 AM

Alright I tested latest version on PlaMo and seems to work perfectly fine now!

@zzag If you are happy with it we can get this merged :)

bshah accepted this revision as: Plasma: Mobile, bshah.May 19 2020, 7:17 AM
zzag added a comment.May 19 2020, 7:43 AM

Alright I tested latest version on PlaMo and seems to work perfectly fine now!

Does it work fine with software screen rotation?

plugins/platforms/drm/egl_gbm_backend.cpp
441

whitespace before *

444

No auto please.

bshah added a comment.May 19 2020, 7:53 AM

Does it work fine with software screen rotation?

Hm seems broken

apol updated this revision to Diff 83059.May 19 2020, 11:02 AM

Account for software rotation

Ping, it should be committed?