[wayland] Add support for zwp_linux_dmabuf
Needs ReviewPublic

Authored by romangg on Feb 22 2018, 5:40 PM.

Details

Summary

This adds support for LinuxDmabufUnstableV1Interface in kwin.

Test Plan

Session starts. weston-simple-dmabuf-egl and weston-simple-dmabuf-drm execute without errors.

Diff Detail

Repository
R108 KWin
Branch
dmaBuf
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13219
Build 13237: arc lint + arc unit
fredrik created this revision.Feb 22 2018, 5:40 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 22 2018, 5:40 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
fredrik requested review of this revision.Feb 22 2018, 5:40 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 22 2018, 5:40 PM

Concerning the tests: the ones requiring OpenGL work best if module vgem is loaded. That normally makes them pass. The tests regarding keyboard layout need env variable XDG_DEFAULT_LAYOUT being unset or on us.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 22 2018, 6:58 PM
fredrik edited projects, added Plasma; removed KWin.Feb 23 2018, 3:36 PM

Concerning the tests: the ones requiring OpenGL work best if module vgem is loaded. That normally makes them pass. The tests regarding keyboard layout need env variable XDG_DEFAULT_LAYOUT being unset or on us.

Those results are with vgem loaded, and XDG_DEFAULT_LAYOUT is not set.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 23 2018, 3:36 PM
anthonyfieroni added inline comments.
platformsupport/scenes/opengl/abstract_egl_backend.cpp
117

Call delete dmabuf will have same effect.

738

This function is useless.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 23 2018, 6:11 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 23 2018, 8:29 PM

An issue that this patch does not fully address is switching compositing backends at runtime.

Buffers are imported by the scene. The scene allocates and returns a subclassed LinuxDmabuf::Buffer that contains the file descriptors and backend specific objects, such as EGL images. When the compositing backend is destroyed, the EGL images are also destroyed, but the buffers survive. The new backend could use the file descriptors to re-import the buffers, if the buffer class wasn't specific to the old backend.

A possible solution to this is a shared Buffer class (in platformsupport/scenes/common?):

    ...
private:
    ...
    QVector<Plane> planes;
    EGLimage eglImage;
    VkImage vulkanImage;
    VkImageView imageView;
    VkDeviceMemory deviceMemory;
};

Or a SceneBufferData object, created and destroyed by the scene:

private:
    ...
    QVector<Plane> planes;
    SceneBufferData *sceneData;
};

But even with a shared class there are no guarantees that a re-import is possible, since the new backend might not support the same formats and/or modifiers. So I don't know if runtime switching is something that should be expected to work.

platformsupport/scenes/opengl/abstract_egl_backend.cpp
117

No, it will not. It will leave a dangling pointer in the wl_resource, which will result in a double-free when the client deletes the buffer. Or a segfault the next time it tries to attach the buffer to a surface.

I will expand on this in a separate comment.

fredrik updated this revision to Diff 29331.Mar 12 2018, 2:07 PM

Import the context.

Do typedefs for KWayland::Server::LinuxDmabuf in files where you use it more than once.

platformsupport/scenes/opengl/abstract_egl_backend.cpp
378

Name should be more descriptive in relation to functionality, also it's not a signal, so "aboutTo" imo not recommended.

Suggestion: removeDmabufBuffer

platformsupport/scenes/opengl/abstract_egl_backend.h
100

Why QLinkedList? It should be no better than QList for the removeOne call. For this better use QSet.

135

Why is it necessary to export?

An issue that this patch does not fully address is switching compositing backends at runtime.

Do we support that at all? The backend is set at startup. Don't think you can change this later on.

An issue that this patch does not fully address is switching compositing backends at runtime.

Do we support that at all? The backend is set at startup. Don't think you can change this later on.

That is the question I'm asking.

platformsupport/scenes/opengl/abstract_egl_backend.h
100

I believe the use of QList is discouraged in new code, but you are right about QSet being a better choice in this case.

135

I guess it isn't. I was thinking at the time that someone might want to use this class in a class derived from AbstractEglBackend or AbstractEglTexture.

fredrik updated this revision to Diff 29953.Mar 19 2018, 11:47 PM

Fix issues pointed out by romangg.

fredrik marked 3 inline comments as done.Mar 19 2018, 11:49 PM
zzag added a subscriber: zzag.Sep 17 2018, 8:08 AM

Any update on this?

meven added a subscriber: meven.Jun 13 2019, 7:59 AM
In D10750#327333, @zzag wrote:

Any update on this?

I am wondering as well.
This seems like good progress was already done and what is left is some test fixes and polish.
I can only encourage @fredrik and good KWin folks in their endeavor.

romangg commandeered this revision.Sun, Jun 23, 12:52 PM
romangg added a reviewer: fredrik.
romangg updated this revision to Diff 60482.Sun, Jun 23, 12:53 PM

Rebase Fredrik's dma-buf code on master

zzag added inline comments.Sun, Jun 23, 3:05 PM
platformsupport/scenes/opengl/abstract_egl_backend.cpp
427

What's holding us from doing that?

zzag added inline comments.Sun, Jun 23, 3:21 PM
platformsupport/scenes/opengl/abstract_egl_backend.cpp
438–439

Perhaps we need to add modifier only if EGL_EXT_image_dma_buf_import_modifiers is present.

fredrik added inline comments.Sun, Jun 23, 10:07 PM
platformsupport/scenes/opengl/abstract_egl_backend.cpp
427

We would need to create a separate EGL image and a separate texture for each plane.
The scene would need to bind each of those textures to separate texture binding points, and we would need to generate and use a shader that samples texels from each plane and performs YUV to RGB conversion.

romangg added inline comments.Mon, Jun 24, 8:38 AM
platformsupport/scenes/opengl/abstract_egl_backend.cpp
427

Thanks for the explanation @fredrik. It wasn't clear to me how multi-planes buffers are handled (also that it's the same notion as for hw-planes is annoying!).

Until we support multi-plane buffers shouldn't we then filter out all multi-plane formats in the supportedFormats call such that the client does not even try to use these instead of just failing with null here?

romangg updated this revision to Diff 60655.Tue, Jun 25, 3:46 PM

Refactor

Bind the interface on abstract-egl level. Then we can remove the interface
stubs throughout the stack.

The interface is not created when EGL format extensions are not available
or we don't use EGL.

Let the impl be the main class creating the interface, the impl is then
destroyed from the interface on destruction of the abstract-egl-backend.

Set supported formats and modifiers once in the beginning and remember them
in the KWayland interface private part. The according KWayland interface
has been adapted.

This code is still missing support for multi-plane buffers. We need it because
while we can filter out multi-plane formats modifiers might add additional
planes (for example i915 Y_TILED_CCS) to otherwise single-plane formats.

romangg planned changes to this revision.Tue, Jun 25, 3:48 PM

Multi-plane support is needed. See @fredrik's inline comment on how to do that:

We would need to create a separate EGL image and a separate texture for each plane.
The scene would need to bind each of those textures to separate texture binding points, and we would need to generate and use a shader that samples texels from each plane and performs YUV to RGB conversion.

Multi-plane support is needed. See @fredrik's inline comment on how to do that:

We would need to create a separate EGL image and a separate texture for each plane.
The scene would need to bind each of those textures to separate texture binding points, and we would need to generate and use a shader that samples texels from each plane and performs YUV to RGB conversion.

Going through Weston's code this seems only true for original multi-plane formats, but not for dma-bufs with multiple planes via modifier.
See: https://gitlab.freedesktop.org/wayland/weston/blob/5d7877adfd31956bdad98da4a937059260da1f69/libweston/renderer-gl/gl-renderer.c#L2291

While allowing multi-plane for the former would involve some more fundamental changes to KWin since we when need to save multiple textures per WindowPixmap, doing the modifier multi-plane it seems we just receive a single EGLImage which we can then texture from. At least the following revision works with both weston-simple-dmabuf-egl and weston-simple-dmabuf-drm. And session starts including XWayland (didn't work before).

romangg updated this revision to Diff 60898.Sun, Jun 30, 11:27 PM
  • Import dma-bufs with multi-plane via modifier
romangg retitled this revision from wayland: Add support for zwp_linux_dmabuf to [wayland] Add support for zwp_linux_dmabuf.Sun, Jun 30, 11:38 PM
romangg edited the test plan for this revision. (Show Details)
romangg added inline comments.Mon, Jul 1, 7:59 AM
platformsupport/scenes/opengl/abstract_egl_backend.cpp
538–540

rm

romangg marked an inline comment as done.Wed, Jul 17, 2:04 PM
romangg added inline comments.
platformsupport/scenes/opengl/abstract_egl_backend.cpp
538–540

Nope, better not. Overlooked the m_.

romangg updated this revision to Diff 61915.Wed, Jul 17, 2:04 PM
romangg marked 2 inline comments as done.

Rebase on master.