Implement zwp_linux_dmabuf_v1
ClosedPublic

Authored by romangg on Feb 22 2018, 2:27 PM.

Details

Summary

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

Test Plan

Works with weston-simple-dmabuf-drm and weston-simple-dmabuf-egl in KWin.

Diff Detail

Repository
R127 KWayland
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fredrik created this revision.Feb 22 2018, 2:27 PM
Restricted Application added projects: Plasma on Wayland, Frameworks. · View Herald TranscriptFeb 22 2018, 2:27 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
fredrik requested review of this revision.Feb 22 2018, 2:27 PM
Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptFeb 22 2018, 2:27 PM
mart added a comment.Feb 22 2018, 2:56 PM

+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 :)

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptFeb 22 2018, 2:56 PM
fredrik added inline comments.Feb 22 2018, 3:07 PM
src/server/linuxdmabuf_v1_interface.h
40

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

66

Should the Buffer class use a d-pointer?

108

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.

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptFeb 22 2018, 3:07 PM
Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptFeb 23 2018, 8:29 PM
romangg added a subscriber: romangg.Mar 7 2018, 6:49 PM

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

fredrik updated this revision to Diff 28986.Mar 7 2018, 11:06 PM

Import the context.

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?

src/server/linuxdmabuf_v1_interface.h
40

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

66

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

108

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.

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.

src/server/linuxdmabuf_v1_interface.h
108

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.

src/server/linuxdmabuf_v1_interface.cpp
365

is this defined? I can't find it

I'd expect it be used on line 408.

src/server/linuxdmabuf_v1_interface.h
66

doesn't this need exporting?

100

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)

108

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.

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

Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptJun 1 2018, 3:01 PM
romangg commandeered this revision.Jun 22 2019, 3:21 PM
romangg added a reviewer: fredrik.

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

romangg updated this revision to Diff 60360.Jun 22 2019, 3:25 PM

Rebase on master.

romangg updated this revision to Diff 60364.Jun 22 2019, 3:32 PM
  • Revert drm_fourcc.h whitespace changes
romangg updated this revision to Diff 60366.Jun 22 2019, 3:36 PM
  • Minor style changes
romangg updated this revision to Diff 60429.Jun 23 2019, 9:46 AM
  • Update protocol xml to wayland-protocols master
romangg updated this revision to Diff 60654.Jun 25 2019, 3:38 PM
  • 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
romangg edited the test plan for this revision. (Show Details)Jun 25 2019, 3:40 PM
zzag added a subscriber: zzag.Jun 29 2019, 10:58 AM
This comment was removed by zzag.
romangg updated this revision to Diff 61912.Jul 17 2019, 1:49 PM
  • Rebase on master
  • Use packaged dma-buf xml

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?

romangg updated this revision to Diff 63224.Tue, Aug 6, 7:36 PM

Rebase on master.

romangg edited the test plan for this revision. (Show Details)Tue, Aug 6, 7:40 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Aug 6, 8:03 PM
This revision was automatically updated to reflect the committed changes.