[PATCH 1/7] protocol: add linux_dmabuf extension RFCv1

[Pekka Paalanen]

On Thu, 18 Dec 2014 10:25:09 +0100
Daniel Vetter <daniel at ffwll.ch> wrote:

> On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis
> Ratté-Boulianne wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > An experimental (hence the 'z' prefix) linux_dmabuf Wayland protocol
> > extension for creating dmabuf-based wl_buffers in a generic manner.
> > 
> > This does not include proper dmabuf metadata negotiation because
> > there is no way to communicate all dmabuf constraints from the
> > compositor to a client before-hand. The client has to create a
> > wl_buffer wrapping one or more dmabuf buffers and then listen at
> > the feedback object returned to know if the operation was
> > successful.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>
> 
> So I have no idea about how wayland protos work, so please take that
> into account ;-)

Hi Daniel,

very much appreciated anyway!

> Generally I think we should try to follow what the drm addfb2 ioctl
> does in drm, since in the end we need to be able to handle pretty
> much all the same issues. Well wayland needs to solve a few more
> since it also must cope with buffer layouts which can only be used
> for rendering but not scanned out.

Yes. The current proposal was written based on
https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_import.txt
which has some differences compared to addfb2, like having only 3
planes instead of 4.

> Wrt tiling layouts and compressed buffers and similar vendor specific
> things: The current proposal is to add a new uint64_t layout
> qualifier to addfb with opaque #defines (probably a new header), with
> an 8:56 bit split between vendor identifier and opaque vendor
> specified content. It will also be per-buffer (like pitches/offsets).
> The abi isn't locked down yet, but definitely something to keep in
> mind.

Yup, I've seen it.

> It will definitely complicate the protocol though since the specific
> layout modifiers which are acceptable change dynamically at runtime:
> Some scanout formats become invalid when we run out of fifo space,
> some can only be used on specific planes and ofc this all depends
> upon whether gl compositing or hw planes are used to pick the right
> one.

From the Wayland compositor point of view, things would get awfully
hard if an existing wl_buffer (handle to a dmabuf object) could
suddenly become completely unusable. That is why we try to make sure
during the wl_buffer creation phase (when a client initially shares the
dmabuf with the compositor) that the buffer is always usable for at
least compositing (i.e. as a GL texture).

If the buffer is also usable for direct scanout, that's a nice bonus.
It's no problem to fall back to compositing if a buffer suddenly or
temporarily turns out to be not scanout-able.

We just need to guarantee that the compositing path always works. I
don't think we can say to a client "oops, go make a different kind of
buffer, this no longer works".

Do you think these expectations are realistic?

> 
> Some more random comments below.
> 
> Cheers, Daniel
> 
> > ---
> >  Makefile.am               |   7 +-
> >  protocol/linux-dmabuf.xml | 224
> > ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 229
> > insertions(+), 2 deletions(-) create mode 100644
> > protocol/linux-dmabuf.xml
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 494266d..0462fdd 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -91,7 +91,9 @@ nodist_weston_SOURCES
> > =					\
> > protocol/presentation_timing-protocol.c		\
> > protocol/presentation_timing-server-protocol.h	\
> > protocol/scaler-protocol.c			\
> > -	protocol/scaler-server-protocol.h
> > +	protocol/scaler-server-protocol.h		\
> > +	protocol/linux-dmabuf-protocol.c		\
> > +	protocol/linux-dmabuf-server-protocol.h
> >  
> >  BUILT_SOURCES += $(nodist_weston_SOURCES)
> >  
> > @@ -1101,7 +1103,8 @@ EXTRA_DIST
> > +=					\
> > protocol/presentation_timing.xml	\
> > protocol/scaler.xml			\
> > protocol/ivi-application.xml		\
> > -	protocol/ivi-hmi-controller.xml
> > +	protocol/ivi-hmi-controller.xml		\
> > +	protocol/linux-dmabuf.xml
> >  
> >  man_MANS = weston.1 weston.ini.5
> >  
> > diff --git a/protocol/linux-dmabuf.xml b/protocol/linux-dmabuf.xml
> > new file mode 100644
> > index 0000000..c48121c
> > --- /dev/null
> > +++ b/protocol/linux-dmabuf.xml
> > @@ -0,0 +1,224 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="linux_dmabuf">
> > +

> > +  <interface name="zlinux_dmabuf" version="1">
> > +    <description summary="factory for creating dmabuf-based
> > wl_buffers">
> > +      Following the interfaces from:
> > +
> > https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_import.txt
> > +
> > +      This interface offers a way to create generic dmabuf-based
> > +      wl_buffers. Immediately after a client binds to this
> > interface,
> > +      the set of supported formats is sent with 'format' events.
> > +    </description>
> > +
> > +    <enum name="error">
> > +      <entry name="invalid_format" value="1"/>
> > +      <entry name="invalid_dmabuf" value="2"/>
> > +      <entry name="format_dmabufs_mismatch" value="3"/>
> > +      <entry name="invalid_dmabuf_params" value="4"/>
> > +    </enum>
> > +
> > +    <enum name="format">
> > +      <!-- The drm format codes match the #defines in drm_fourcc.h.
> > +           The formats actually supported by the compositor will be
> > +           reported by the format event. -->
> 
> Is there some way we could autogenerate this from drm_fourcc.h? We
> need to do changes to the kernel header to facilitate that we should
> do so imo.
> 
> Or should we just mandate that this must be a fourcc code from
> drm_fourcc.h and not list them all as enums?

Since this by definition must be the same as drm_fourcc.h values, and
drm_fourcc.h is always present if there is dmabuf support, I think
you're right. We could simply specify, that the values are defined by
drm_fourcc.h, and drop this whole list.


> > +    </enum>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="unbind the factory">
> > +      </description>
> > +    </request>
> > +
> > +    <request name="create_batch">
> > +      <description summary="create a collection of dmabufs">
> > +        This temporary object is used to collect multiple dmabuf
> > handles
> > +        into a single batch.
> > +      </description>
> > +      <arg name="batch_id" type="new_id" interface="dmabuf_batch"/>
> > +    </request>
> > +
> > +    <request name="create_buffer">
> > +      <description summary="create a dmabuf-based wl_buffer">
> > +        The result of the operation will be returned via the
> > provided
> > +        zlinux_dmabuf_create_feedback object.
> > +
> > +        The dmabuf_batch object must be destroyed immediately after
> > +        after this request.
> > +
> > +        Any errors in importing the set of dmabufs can be
> > delivered as
> > +        protocol errors immediately or later.
> > +      </description>
> > +      <arg name="batch" type="object" interface="dmabuf_batch"/>
> > +      <arg name="width" type="int"/>
> > +      <arg name="height" type="int"/>
> > +      <arg name="format" type="uint" summary="from format enum"/>
> > +      <arg name="feedback" type="new_id"
> > interface="zlinux_dmabuf_create_feedback"/>
> > +    </request>
> > +
> > +    <event name="format">
> > +      <description summary="supported buffer format">
> > +        This event advertises one buffer format that the server
> > support.
> > +        All the supported formats advertised once when the client
> > +        binds to this interface. A roundtrip after binding
> > guarantees,
> > +        that the client has received all supported formats.
> > +      </description>
> > +      <arg name="format" type="uint" summary="from format enum"/>
> > +    </event>
> > +
> > +  </interface>

> > +  <interface name="dmabuf_batch" version="1">
> > +    <description summary="a collection of dmabufs">
> > +      This is a collection of dmabufs, forming a single logical
> > buffer.
> > +      Usually only one dmabuf is needed, but some multi-planar
> > formats
> > +      may require more.
> > +
> > +      The order of dmabufs added for this object is significant,
> > and must
> > +      match the expectations of the format argument to
> > +      linux_dmabuf.create_buffer.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="delete this object, used or not">
> > +      </description>
> > +    </request>
> > +
> > +    <request name="add">
> > +      <description summary="add a dmabuf to the wl_buffer">
> > +        Multi-planar formats may require using more than one
> > +        dmabuf for passing all the data for one logical buffer.
> > +        This request adds one dmabuf to the set in this
> > dmabuf_batch. +
> > +        When one dmabuf has several planar channels, offset1 &
> > stride1 and
> > +        offset2 & stride2 must be used to denote them without
> > sending a
> > +        new add_dmabuf request which would create a new fd in the
> > server
> > +        while still pointing at the same dmabuf.
> > +
> > +        offset0 and stride0 must always be set. Other unused
> > offsets and
> > +        strides must be zero.
> > +      </description>
> > +
> > +      <arg name="name" type="fd"/>
> > +      <arg name="offset0" type="int"/>
> > +      <arg name="stride0" type="int"/>
> > +      <arg name="offset1" type="int"/>
> > +      <arg name="stride1" type="int"/>
> > +      <arg name="offset2" type="int"/>
> > +      <arg name="stride2" type="int"/>
> > +    </request>
> 
> Hm, with addfb we just ask userspace to pass the same fb 3/2 times for
> planar data that's all in the same buffer object.

Yeah, I chose to handle the fds separately, if that is what you mean.
The file descriptors are passed through unix socket SCM_RIGHTS
messages, and libwayland deliberately also dup's them. If a client sends
the same fd several times, the compositor will get different fds
all pointing to the same "file".

Can that be a problem for GBM, EGL, or addfb2 et al. dma_buf import?
Or rather, an implementation that makes it a problem, it legal or buggy?

I assumed it could be a problem, so I designed a way that maintains the
numerical identities between the fds.

Also, we don't have a mechanism to send "not a valid fd" for an fd
field, in case there are less than 3 (4?) planes, so we need a separate
request for each valid fd.


Thanks,
pq