[platforms/drm] Subclass DrmBuffer and fix cleanup after page flip

Authored by romangg on Jan 8 2017, 7:13 PM.


Group Reviewers

DrmBuffers behave very differently if created from a GBM surface or as a dumb QImage buffer. Because of that split it up in subclasses to increase clarity and minimize conditions in the rendering loop. Also this will help, when we later introduce direct scanouts from wayland buffers without rendering on planes (just another subclass).

Also the buffer deletion was wrong in my last work on Atomic Mode Setting. QPainter buffers shouldn't be deleted between page flips, because they reuse the buffers.

Test Plan

Tested it on Wayland session with OpenGl and QPainter, with and without Atomic Mode Setting.

Diff Detail

R108 KWin
Lint Skipped
Unit Tests Skipped
romangg updated this revision to Diff 9873.Jan 8 2017, 7:13 PM
romangg retitled this revision from to [platforms/drm] Subclass DrmBuffer and fix cleanup after page flip.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added a reviewer: KWin.
romangg set the repository for this revision to R108 KWin.
romangg added a project: KWin.
Restricted Application added a subscriber: kwin. ยท View Herald TranscriptJan 8 2017, 7:13 PM
sebas added a subscriber: sebas.

I'm not confident enough to give this one a ship it, Martin, could you have a look?

graesslin added inline comments.Jan 9 2017, 8:32 PM

no need to change the type. It's still a DrmBuffer*


same here


shouldn't we move that into the HAVE_GBM part?


maybe ifdef the complete DrmSurfaceBuffer based on HAVE_GBM? Might simplify the ifdef logic in other areas.

Or: move the DrmSurfaceBuffer into a dedicated .h and .cpp file and only include the .cpp if we have gbm. Then we don't need any ifdefs at all.


why change here? From what I see only the DrmBuffer API is used, isn't it?

romangg added inline comments.Jan 9 2017, 8:35 PM

What happens in the other case? Does it fallback to QPainter when Platform::createOpenGLBackend() is called?

romangg added inline comments.Jan 10 2017, 9:00 AM

The cursor is always a dumb buffer. So I'm not really seeing the advantage of using the abstract type (using DrmDumbBuffer in contrast gives this information directly to the reader). Also in showCursor(..) we call the function DrmDumbBuffer::handle() only known to DrmDumbBuffer. So we would either need to define this function also in DrmBuffer or do a dynamic_cast in showCursor(..).

romangg added inline comments.Jan 17 2017, 1:46 PM

See my comment https://phabricator.kde.org/D4026#inline-16024

handle() is not known to DrmBuffer right now, because it's not needed for a DrmSurfaceBuffer. Also doesn't it make more sense to specify the argument as much as possible so it's easier to read the code? A cursor buffer is always a dumb buffer.

graesslin added inline comments.Jan 19 2017, 6:21 PM

it more or less terminates. Without the else part calls the Platform default impl which returns a nullptr. If createOpenGLBackend returns a nullptr, the Scene cannot be created which means compositing is not possible and KWin terminates.

graesslin accepted this revision.Apr 18 2017, 6:54 PM
This revision is now accepted and ready to land.Apr 18 2017, 6:54 PM
romangg abandoned this revision.Apr 19 2017, 3:59 PM

The changes from this patch had been incorporated instead in the patch series starting with D5118.