D10750: [wayland] Add support for zwp_linux_dmabuf

[Roman Gilg]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R108:6613327a9c3e: [wayland] Add support for zwp_linux_dmabuf 
(authored by romangg).

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=63225&id=63227

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/CMakeLists.txt
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/drm_fourcc.h
  platformsupport/scenes/opengl/linux_dmabuf.cpp
  platformsupport/scenes/opengl/linux_dmabuf.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: [wayland] Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg updated this revision to Diff 63225.
romangg added a comment.


  Rebase on master.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=61915&id=63225

BRANCH
  dmaBuf

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/CMakeLists.txt
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/drm_fourcc.h
  platformsupport/scenes/opengl/linux_dmabuf.cpp
  platformsupport/scenes/opengl/linux_dmabuf.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: [wayland] Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg updated this revision to Diff 61915.
romangg marked 2 inline comments as done.
romangg added a comment.


  Rebase on master.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=60898&id=61915

BRANCH
  dmaBuf

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/CMakeLists.txt
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/drm_fourcc.h
  platformsupport/scenes/opengl/linux_dmabuf.cpp
  platformsupport/scenes/opengl/linux_dmabuf.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
fmonteiro, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, 
jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol, mart

D10750: [wayland] Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg marked an inline comment as done.
romangg added inline comments.

INLINE COMMENTS

> romangg wrote in abstract_egl_backend.cpp:405
> rm

Nope, better not. Overlooked the `m_`.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
fmonteiro, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, 
jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol, mart

D10750: [wayland] Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> abstract_egl_backend.cpp:405
>  if (image != EGL_NO_IMAGE_KHR) {
> -eglDestroyImageKHR(m_backend->eglDisplay(), m_image);
> +if (m_image != EGL_NO_IMAGE_KHR) {
> +eglDestroyImageKHR(m_backend->eglDisplay(), m_image);

rm

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg updated this revision to Diff 60898.
romangg added a comment.


  - Import dma-bufs with multi-plane via modifier

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=60655&id=60898

BRANCH
  dmaBuf

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/CMakeLists.txt
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/drm_fourcc.h
  platformsupport/scenes/opengl/linux_dmabuf.cpp
  platformsupport/scenes/opengl/linux_dmabuf.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added a comment.


  In D10750#486570 , @romangg wrote:
  
  > 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).

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg planned changes to this revision.
romangg added a comment.


  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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg updated this revision to Diff 60655.
romangg added a comment.


  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.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=60482&id=60655

BRANCH
  dmaBuf

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/CMakeLists.txt
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/drm_fourcc.h
  platformsupport/scenes/opengl/linux_dmabuf.cpp
  platformsupport/scenes/opengl/linux_dmabuf.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> fredrik wrote in 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.

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?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added inline comments.

INLINE COMMENTS

> zzag wrote in abstract_egl_backend.cpp:427
> What's holding us from doing 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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> abstract_egl_backend.cpp:438-439
> +EGL_DMA_BUF_PLANE0_PITCH_EXT,   EGLint(planes[0].stride),
> +EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT, EGLint(planes[0].modifier & 
> 0x),
> +EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT, EGLint(planes[0].modifier >> 32),
> +EGL_NONE

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

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> abstract_egl_backend.cpp:427
> +
> +// FIXME: Add support for multi-planar images
> +if (planes.count() != 1)

What's holding us from doing that?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg updated this revision to Diff 60482.
romangg added a comment.


  Rebase Fredrik's dma-buf code on master

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=29953&id=60482

BRANCH
  dmaBuf

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Méven Car]

meven added a comment.


  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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
ericadams, jraleigh, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, 
bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Vlad Zagorodniy]

zzag added a comment.


  Any update on this?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: zzag, romangg, anthonyfieroni, plasma-devel, kwin, mkulinski, ragreen, 
jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik updated this revision to Diff 29953.
fredrik added a comment.


  Fix issues pointed out by romangg.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=29331&id=29953

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added a comment.


  In D10750#224860 , @romangg wrote:
  
  > In D10750#216337 , @fredrik 
wrote:
  >
  > > 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.

INLINE COMMENTS

> romangg wrote in abstract_egl_backend.h:100
> Why QLinkedList? It should be no better than QList for the removeOne call. 
> For this better use QSet.

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

> romangg wrote in abstract_egl_backend.h:135
> Why is it necessary to export?

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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added a comment.


  In D10750#216337 , @fredrik wrote:
  
  > 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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added a comment.


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

INLINE COMMENTS

> abstract_egl_backend.cpp:346
>  
> +void AbstractEglBackend::aboutToDestroy(EglDmabufBuffer *buffer)
> +{

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

Suggestion: `removeDmabufBuffer`

> abstract_egl_backend.h:100
>  QList m_clientExtensions;
> +QLinkedList m_dmabufBuffers;
> +bool m_haveDmabufImport = false;

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

> abstract_egl_backend.h:135
>  
> +class KWIN_EXPORT EglDmabufBuffer : public 
> KWayland::Server::LinuxDmabuf::Buffer
> +{

Why is it necessary to export?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik updated this revision to Diff 29331.
fredrik added a comment.


  Import the context.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=27803&id=29331

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added a comment.


  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 planes;
EGLimage eglImage;
VkImage vulkanImage;
VkImageView imageView;
VkDeviceMemory deviceMemory;
};
  
  Or a SceneBufferData object, created and destroyed by the scene:
  
private:
...
QVector 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.

INLINE COMMENTS

> anthonyfieroni wrote in abstract_egl_backend.cpp:117
> Call delete dmabuf will have same effect.

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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Roman Gilg]

romangg added a task: T8067: Support zwp_linux_dmabuf.
Restricted Application edited projects, added KWin; removed Plasma.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Anthony Fieroni]

anthonyfieroni added inline comments.
Restricted Application edited projects, added Plasma; removed KWin.

INLINE COMMENTS

> abstract_egl_backend.cpp:117
> +for (auto *dmabuf : qAsConst(m_dmabufBuffers)) {
> +dmabuf->destroyImage();
> +}

Call delete dmabuf will have same effect.

> abstract_egl_backend.cpp:738
> +
> +void EglDmabufBuffer::destroyImage()
> +{

This function is useless.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, iodelay, bwowk, hardening

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik edited projects, added Plasma; removed KWin.
fredrik added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  In D10750#211784 , @graesslin 
wrote:
  
  > 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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik created this revision.
fredrik added reviewers: KWin, Plasma, davidedmundson, mart, graesslin.
Restricted Application added a project: KWin.
Restricted Application added subscribers: kwin, plasma-devel.
fredrik requested review of this revision.
Restricted Application edited projects, added Plasma; removed KWin.

REVISION SUMMARY
  This adds support for LinuxDmabufUnstableV1Interface in kwin.

TEST PLAN
  I asked Marco to test it with a driver that supports modifiers, and he 
confirmed that it works.
  I have also checked that kwin still works with drivers that don't support 
modifiers.
  
  Test results before (6 failures):
  
  The following tests FAILED:
  
30 - kwin-testLockScreen (Failed)
39 - kwin-testPointerInput (Failed)
57 - kwin-testSceneOpenGL-waylandonly (Failed)
71 - kwin-testKeyboardLayout (Failed)
74 - kwin-testKeymapCreationFailure-waylandonly (Failed)
88 - kwin-testColorCorrectNightColor-waylandonly (Failed)
  
  Test results after (8 failures):
  
  The following tests FAILED:
  
30 - kwin-testLockScreen (Failed)
39 - kwin-testPointerInput (Failed)
59 - kwin-testSceneOpenGLES-waylandonly (Failed)
60 - kwin-testNoXdgRuntimeDir (Failed)
61 - kwin-testNoXdgRuntimeDir-waylandonly (Failed)
71 - kwin-testKeyboardLayout (Failed)
72 - kwin-testKeyboardLayout-waylandonly (Failed)
74 - kwin-testKeymapCreationFailure-waylandonly (Failed)
  
  I'm not sure what to make of this, but at least some of these failures are 
spurious.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, kwin, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart