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).
Details
Details
- Reviewers
zzag bshah - Group Reviewers
KWin Plasma: Mobile
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
Diff Detail
- Repository
- R108 KWin
- Branch
- arcpatch-D27788_3
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 26076 Build 26094: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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().
Comment Actions
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.
| scene.cpp | ||
|---|---|---|
| 235 | This happens in Scene::paintScreen. | |
| 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(). | |
| 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. | |
| 529–541 | I think we can drop this method now since only EglGbmBackend::prepareRenderingForScreen() needs to accumulate damage. | |
| scene.cpp | ||
|---|---|---|
| 616 | Usually we don't put a semicolon after Q_UNUSED. | |
| plugins/platforms/drm/egl_gbm_backend.cpp | ||
|---|---|---|
| 529–541 | I think next person who reads the code will prefer to see it encapsulated. | |
| plugins/platforms/drm/egl_gbm_backend.cpp | ||
|---|---|---|
| 465 | This comment hasn't been addressed. | |
| 529–541 | 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. | |
Comment Actions
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. | |
Comment Actions
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
Comment Actions
Heh, that's right! We pass the damage region to aboutToStartPainting() in device-independent pixels.
| plugins/platforms/drm/egl_gbm_backend.cpp | ||
|---|---|---|
| 441 ↗ | (On Diff #81497) | The region is in the global screen coordinates, but the buffer damage needs to be in the output local coordinates. |
| plugins/platforms/drm/egl_gbm_backend.cpp | ||
|---|---|---|
| 444 ↗ | (On Diff #81497) | 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. |
Comment Actions
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 :)
Comment Actions
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.