D10747: Implement zwp_linux_dmabuf_v1

[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 R127:1dd57d909165: Implement zwp_linux_dmabuf_v1 (authored by 
romangg).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=63224=63228

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

AFFECTED FILES
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, 
jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg edited the test plan for this revision.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, 
jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  Rebase on master.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=61912=63224

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, 
jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg added a comment.


  If there are no objections I would push this soon.
  
  There is one remaining annoyance I have with the current version: the name of 
the class inside the interface the compositor has to subclass for interfacing 
with KWayland. I changed the name from Bridge to Impl since Bridge can be 
anything and as we use the name Impl in KWin already for stuff like this. But 
there it's used for the subclass and not for the abstract implementation base 
class. Any other idea for a good name?

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, fmonteiro, 
sbergeron, hannahk, jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  - Rebase on master
  - Use packaged dma-buf xml

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=60654=61912

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, fmonteiro, 
sbergeron, hannahk, jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Vlad Zagorodniy]

zzag added a comment.


  > Update protocol xml to wayland-protocols master
  
  OT: there's a package called wayland-protocols. We could use it in order to 
avoid copying protocol specs.
  
  name=FindWaylandProtocols.cmake
#.rst:
# FindWaylandProtocols
# ---
#
# Try to find wayland-protocols on a Unix system.
#
# This will define the following variables:
#
# ``WaylandProtocols_FOUND``
# True if (the requested version of) wayland-protocols is available
# ``WaylandProtocols_VERSION``
# The version of wayland-protocols
# ``WaylandProtocols_DATADIR``
# The wayland protocols data directory

find_package(PkgConfig)
pkg_check_modules(PKG_wayland_protocols QUIET wayland-protocols)

set(WaylandProtocols_VERSION ${PKG_wayland_protocols_VERSION})
pkg_get_variable(WaylandProtocols_DATADIR wayland-protocols pkgdatadir)

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(WaylandProtocols
FOUND_VAR WaylandProtocols_FOUND
REQUIRED_VARS WaylandProtocols_DATADIR
VERSION_VAR WaylandProtocols_VERSION
)

include(FeatureSummary)
set_package_properties(WaylandProtocols PROPERTIES
URL "https://wayland.freedesktop.org/;
DESCRIPTION "Specifications of extended Wayland protocols"
)
  
  and then use it as follows
  
find_package(WaylandProtocols 1.8 REQUIRED)


${WaylandProtocols_DATADIR}/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, 
jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg added a dependent revision: D10750: wayland: Add support for 
zwp_linux_dmabuf.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg edited the test plan for this revision.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  - D-pointer DmaBufBuffer
  - Only allow one create request in any case
  - Params in private interface class
  - typedef for LinuxDmabufUnstableV1Interface
  - Small cleanup
  - Set buffer privates and delete impl on interface destroy

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=60429=60654

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  - Update protocol xml to wayland-protocols master

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=60366=60429

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  - Minor style changes

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=60364=60366

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  - Revert drm_fourcc.h whitespace changes

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=60360=60364

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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


  Rebase on master.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=28986=60360

BRANCH
  dmaBuf

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg commandeered this revision.
romangg added a reviewer: fredrik.
romangg added a comment.


  Thanks @fredrik for initiating this. I'll try to finish the patches up.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, jraleigh, 
sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg added a comment.
Restricted Application edited subscribers, added: kde-frameworks-devel; 
removed: Frameworks.


  What's the current state of this patch and the KWin one?

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: kde-frameworks-devel, romangg, plasma-devel, ragreen, Pitel, schernikov, 
michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, #frameworks

D10747: Implement zwp_linux_dmabuf_v1

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> linuxdmabuf_v1_interface.cpp:364
> +
> +static void unbind(wl_client *client, wl_resource *resource);
> +static void createParamsCallback(wl_client *client, wl_resource 
> *resource, uint32_t id);

is this defined? I can't find it

I'd expect it be used on line 408.

> linuxdmabuf_v1_interface.h:65
> + */
> +class Buffer {
> +public:

doesn't this need exporting?

> linuxdmabuf_v1_interface.h:99
> + */
> +class KWAYLANDSERVER_EXPORT LinuxDmabufUnstableV1Interface : public Global
> +{

One of kwayland's functions is to act as an abstraction layer

Generally all exported class names aren't called with UnstableV1 or whatever.
This would be LinuxDmabufInterface and then we'd handle the V1 stuff in the 
private implementation.

(Personally, I think it's far more effort than it's worth to abstract something 
that isn't guaranteed to be compatiable, and would be ok for you argue that 
it's deliberate)

> fredrik wrote in linuxdmabuf_v1_interface.h:107
> Is this the solution we want for interfacing with the compositor?
> 
> My preference would be to use std::function callbacks, with setters in 
> LinuxDmabufUnstableV1Interface. Setting up the interface could then look like 
> this:
> 
>   m_linuxDmabuf = m_display->createLinuxDmabufInterface(m_display);
>   m_linuxDmabuf->setQuerySupportedFormats([]{ return 
> Compositor::self()->scene()->supportedDrmFormats(); });
>   ...
>   m_linuxDmabuf->create();
> 
> This can also be extended without breaking binary compatibility. But I don't 
> think we can use std::function in frameworks. There are also BIC issues when 
> mixing different STL implementations, which we may or may not care about.

I don't think I fully understand the issue.

I assume the problem we're solving is that we need to provide supportedFormats 
on client bind, and as per the spec we need to do that immediately but we don't 
have that information before the scene is created which comes after we create 
the global?

Looking at the current kwin patch we'd just return wrong values / even crash if 
we were called before the scene was created. Given that's currently the case, 
why can't we just have the compositor call  a normal setSupportedFormats(...) 
when the scene is first set up.

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik added a comment.


  In D10747#237235 , @romangg wrote:
  
  > Regarding the "drm_fourcc.h" file: do we want to copy it in KWayland's code 
base or could we use the system one? It's only available on Linux? In this case 
could we include it as a dummy only on non-Linunx systems? This way we could 
use the system one on Linux.
  
  
  I don't know if it's available on all platforms we support, and copying it 
was the path of least resistance.
  
  > Should LinuxDmabufParams subclass Resource?
  
  I considered doing that, but Resource is designed in such a way that it 
requires the use of a d-pointer, and LinuxDmabufParams is a private class.

INLINE COMMENTS

> romangg wrote in linuxdmabuf_v1_interface.h:107
> I'm not sure what's the canonical way in KWayland to do this. I assume the 
> supported formats and modifiers could be saved in a field of the interface's 
> Private class on creation.
> 
> The buffer could be imported through a signal to the compositor and a 
> function in the interface to be called by the compositor afterwards to 
> proceed.

I considered that, but it seems ugly to have the compositor call back into 
KWayland from the slot. It would also be necessary to pass a pointer to the 
LinuxDmabufParams object in a signal parameter, just so the compositor can pass 
it back to KWayland.

A slightly less ugly option would to make the LinuxDmabufParams class public, 
and emit the signal from the params object. But that would require the 
compositor to track the creation of those objects so it can connect the signal 
to a slot.

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg added a comment.


  Regarding the "drm_fourcc.h" file: do we want to copy it in KWayland's code 
base or could we use the system one? It's only available on Linux? In this case 
could we include it as a dummy only on non-Linunx systems? This way we could 
use the system one on Linux.
  
  Should LinuxDmabufParams subclass Resource?

INLINE COMMENTS

> fredrik wrote in linuxdmabuf_v1_interface.h:39
> Do we want these nested namespaces? We could have LinuxDmabufFlags, 
> LinuxDmabufBuffer etc. instead.

Since there are not yet any namespaces in KWayland below the client/server 
level, I would prefer it without the namespace.

> fredrik wrote in linuxdmabuf_v1_interface.h:65
> Should the Buffer class use a d-pointer?

I think yes. Together with a Private class implemented in the cpp file.

> fredrik wrote in linuxdmabuf_v1_interface.h:107
> Is this the solution we want for interfacing with the compositor?
> 
> My preference would be to use std::function callbacks, with setters in 
> LinuxDmabufUnstableV1Interface. Setting up the interface could then look like 
> this:
> 
>   m_linuxDmabuf = m_display->createLinuxDmabufInterface(m_display);
>   m_linuxDmabuf->setQuerySupportedFormats([]{ return 
> Compositor::self()->scene()->supportedDrmFormats(); });
>   ...
>   m_linuxDmabuf->create();
> 
> This can also be extended without breaking binary compatibility. But I don't 
> think we can use std::function in frameworks. There are also BIC issues when 
> mixing different STL implementations, which we may or may not care about.

I'm not sure what's the canonical way in KWayland to do this. I assume the 
supported formats and modifiers could be saved in a field of the interface's 
Private class on creation.

The buffer could be imported through a signal to the compositor and a function 
in the interface to be called by the compositor afterwards to proceed.

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, ragreen, schernikov, michaelh, ZrenBot, 
ngraham, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

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


  Import the context.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=27735=28986

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, schernikov, michaelh, ZrenBot, 
alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

romangg added a comment.


  Please import the context. This makes reviews more easy. Either via -U9 
flag to git diff or (for future diffs) by using arc directly on a feature 
branch.

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, schernikov, michaelh, ZrenBot, 
alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Roman Gilg]

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

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: plasma-devel, #frameworks, schernikov, michaelh, ZrenBot, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

INLINE COMMENTS

> linuxdmabuf_v1_interface.h:39
> +
> +namespace LinuxDmabuf
> +{

Do we want these nested namespaces? We could have LinuxDmabufFlags, 
LinuxDmabufBuffer etc. instead.

> linuxdmabuf_v1_interface.h:65
> + */
> +class Buffer {
> +public:

Should the Buffer class use a d-pointer?

> linuxdmabuf_v1_interface.h:107
> + */
> +class Bridge {
> +public:

Is this the solution we want for interfacing with the compositor?

My preference would be to use std::function callbacks, with setters in 
LinuxDmabufUnstableV1Interface. Setting up the interface could then look like 
this:

  m_linuxDmabuf = m_display->createLinuxDmabufInterface(m_display);
  m_linuxDmabuf->setQuerySupportedFormats([]{ return 
Compositor::self()->scene()->supportedDrmFormats(); });
  ...
  m_linuxDmabuf->create();

This can also be extended without breaking binary compatibility. But I don't 
think we can use std::function in frameworks. There are also BIC issues when 
mixing different STL implementations, which we may or may not care about.

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, schernikov, alexeymin, eliasp, hein

D10747: Implement zwp_linux_dmabuf_v1

[Marco Martin]

mart added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  +1 on my machine opengl apps seems to work well on wayland (after the first 
revision in which they were flipped upside down, now they are ok)
  would need a final ship it by someone more familiar with that area tough :)

REPOSITORY
  R127 KWayland

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

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: plasma-devel, #frameworks, schernikov, michaelh, ZrenBot, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

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

REVISION SUMMARY
  This interface provides a way for clients to create generic dmabuf-based 
wl_buffers.

TEST PLAN
  I asked Marco to test it with a driver that supports modifiers, and he says 
that it works.

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

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