Differential D27788

Implement EGL_KHR_partial_update and EGL_EXT_swap_buffers_with_damage
AbandonedPublic
Actions

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
Lint SkippedExcuse: because yes
Unit
No Unit Test Coverage
Build Status
Buildable 23601
Build 23619: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
apol added a comment.Mar 26 2020, 3:59 PM
In D27788#633425, @davidedmundson wrote:

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
In D27788#633425, @davidedmundson wrote:

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 ↗(On Diff #79009)

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

353–358 ↗(On Diff #79009)

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
davidedmundson added a comment.Apr 23 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 ↗(On Diff #80669)

You must call paintBackground if PAINT_SCREEN_BACKGROUND_FIRST is called.

235 ↗(On Diff #80669)

is set*

240 ↗(On Diff #80669)

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 ↗(On Diff #80669)

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 ↗(On Diff #80669)

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 ↗(On Diff #77245)

s/lastRegion/region/

465 ↗(On Diff #77245)

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 ↗(On Diff #77245)

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

506–518 ↗(On Diff #77245)

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 ↗(On Diff #81469)

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
506–518 ↗(On Diff #77245)

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 ↗(On Diff #77245)

This comment hasn't been addressed.

506–518 ↗(On Diff #77245)

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 ↗(On Diff #81504)

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
In D27788#662608, @bshah wrote:

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 ↗(On Diff #77245)

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 ↗(On Diff #77245)

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 ↗(On Diff #77245)

whitespace before *

444 ↗(On Diff #77245)

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

anthonyfieroni added a subscriber: anthonyfieroni.Jul 1 2020, 5:37 AM

Ping, it should be committed?

nicolasfella added a subscriber: nicolasfella.Jul 23 2020, 4:26 PM

Ping? I tested it the other day on my Pinebook and didn't notice anything unusual

apol added a comment.Jul 23 2020, 4:56 PM

I'd love to merge it.

On the last iteration @bshah said he saw some artifacts on rotation and I decided I'd test it on the device because I couldn't reproduce on the pinebook. If it's working well for you, I think we should merge it.

nicolasfella added a comment.Jul 23 2020, 5:00 PM

It doesn't apply on current master, so my test is a few weeks old

Revision Contents
Changeset List

Diff 77476

View Options

platformsupport/scenes/opengl/abstract_egl_backend.cpp

Loading...
View Options

platformsupport/scenes/opengl/backend.h

Loading...
View Options

plugins/platforms/drm/egl_gbm_backend.h

Loading...
View Options

plugins/platforms/drm/egl_gbm_backend.cpp

Loading...