Implement zwp_linux_dmabuf_v1
Needs ReviewPublic

Authored by fredrik 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

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

Diff Detail

Repository
R127 KWayland
Lint
Lint Skipped
Unit
Unit Tests Skipped
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
364

is this defined? I can't find it

I'd expect it be used on line 408.

src/server/linuxdmabuf_v1_interface.h
65

doesn't this need exporting?

99

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