[Bug 711155] wayland: add wl_drm support to wayland sink

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=711155

--- Comment #61 from Daniel Stone <daniel at fooishbar.org> ---
(In reply to George Kiagiadakis from comment #60)
> I've worked a bit on this recently and I've updated my branch with the split
> sink approach:
> 
> https://git.collabora.com/cgit/user/gkiagia/gst-plugins-bad.git/log/
> ?h=waylandsink-dmabuf

I've not actually looked into the code yet, nor do I have anything to clever to
say about the GSt-specific design, but a few things jumped out at me ...

> Currently, the linux_dmabuf v1 protocol that I am using has no way of
> informing the client of what requirements it has on the dmabuf that it can
> import. It can inform it only about the colour format (although this is not
> implemented at the moment), but it cannot inform it about the alignment
> constraints as well as the potential hardware constraints (i.e. a certain
> device may not be able to share dmabuf with another certain device in some
> platform maybe?).

Sorry, but you'll never get that. Not only do the restrictions vary from format
to format, but to an extent they also often depend on state rather than static
limits.

There was an effort a few years ago to find a common way to express these
limitations, but it never stuck, and hardware has only got more complex since.
Not only that, but you'd then have to convey the restrictions back to the
allocator ... in short, trial and error is the only method that will work for
the forseeable future.

> When I started writing wldmabufsink, I was thinking that I could do this in
> set_caps() in order to check if the negotiated format is OK (since listing
> formats from the compositor is not implemented). However, the problem is
> that at that point the allocator is unknown. One solution would be to write
> a drm dumb-buffers-based allocator in waylandsink, like the one that
> Benjamin has written in his patch, and then allocate a dummy buffer from
> there with the negotiated format and see if it works. But this is wrong in
> many ways.

Listing formats for the compositor is WIP, and the protocol included in the
last revision on Phabricator.

> Apart from adding a dependency to drm and limiting waylandsink
> for use on drm-based systems, it would also not really prove anything about
> the negotiation, because the compositor may accept dumb buffers and reject
> v4l2 buffers for example, or vice versa (in a v4l2{src,dec} ! wldmabufsink
> scenario).

And this may be format-dependent, e.g. for one format/modifier combination
(thinking of tiling and compression in particular), it may only be possible to
use non-dumb buffers.

> Now this doesn't fully solve the problem again, because there is still no
> way to negotiate the format. But this is something that eventually will be
> implemented, I hope, in the wayland linux_dmabuf protocol, so we will be
> able to use normal caps to negotiate the format first and then negotiate the
> allocator using the mechanism described above. Fingers crossed, though, as
> I'm a bit sceptical wrt the reason that this feature is not implemented yet.

The reason it isn't implemented yet is twofold.

Firstly, DRM format modifiers were added as an additional combination to the
format. In this case, the format describes the actual pixel values one-by-one
(e.g. each pixel is x8r8g8b8), but modifiers describe an additional
transformation between the raw buffer and the pixels: tiling, compression, et
al. This is required for quite a few usecases, and replaces the separate
NV12M-style format codes, as well as driver-specific tiling which can only be
ascertained through custom queries (i915, Nouveau). There is currently no
support in the EGL dmabuf-import extension for this, which is WIP, but we need
to think reasonably carefully about how to implement (format, modifier) query
tuples.

Secondly, the EGL extension again does not include support for format queries.
This is a bit of a showstopper: if DRM can support importing NV12, but the EGL
implementation supports neither NV12 via TEXTURE_EXTERNAL nor R8/RG8 via
TEXTURE_2D, then that means we can only ever display that buffer on planes, and
not through the EGL fallback. This is a showstopper for advertising formats,
and is also WIP.

> Regarding the drm dumb buffers allocator, it would be really nice to have it
> around so that something like videotestsrc ! wldmabufsink can work. However,
> as I said above, I do not agree with adding a dependency on drm in
> waylandsink. Thinking about it, this allocator is a lot like the allocator
> that I have done in the "inteldmabufupload" element [1]. The difference is
> that the inteldmabuf upload element allocates buffers from GEM, which are
> GPU buffers meant to be used by GL internally, and uses the intel-specific
> API.

Technically, dumb buffers are also very much GEM, but this is just a nitpick.

> Dumb buffers are in a different memory and are cross-GPU compatible.

No, that's not it. Often they're in the same memory! It's every bit as possible
to use buffers allocated through device-specific API as it is dumb buffers. In
fact, sometimes even more possible.

The difference between dumb and non-dumb buffers is that dumb buffers are
designed for exactly one usecase: software rendering into them whilst mmap()ed.
You are guaranteed to be allowed to be able to mmap() dumb buffers, but this is
explicitly _not_ true of non-dumb buffers, and there is no generic API provided
for doing so. Similarly, non-dumb buffers can be used with hardware
acceleration engines (mainly GPU), whereas dumb buffers are _explicitly_ not
supported for hardware rendering. There are myriad reasons for this, frequently
to do with coherency (not just pipeline/cache coherency, but also things like
compression buffers); some of these are less valid today than they were when
this was created, but not all.

tl;dr: the main difference is mmap

> What I would propose there is to keep wldmabufsink without any allocator and
> instead require the use of "upload" elements, like inteldmabufupload. We can
> have dumbdmabufupload, intel*, radeon*, nouveau*, ion*, whatever, if there
> are use cases for them. Wayland does not really require any specific
> allocator, it is up to the negotiation mechanism to decide what to use.

So the 'upload' element is essentially an allocator, right? In that it will
control allocation of the storage backing the dmabuf in the first place,
mediate access to it (e.g. performing any necessary pipeline/cache coherency
calls), and provide (emulation of) CPU mapping if necessary.

The rest sounds good to me. Thanks for pushing on this!

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.