[1/7] protocol: add linux_dmabuf extension RFCv1

Submitted by Louis-Francis Ratté-Boulianne on Dec. 12, 2014, 9:51 p.m.

Details

Message ID 1418421068-3854-2-git-send-email-lfrb@collabora.com
State RFC
Headers show

Not browsing as part of any series.

Commit Message

Louis-Francis Ratté-Boulianne Dec. 12, 2014, 9:51 p.m.
From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@collabora.com>
---
 Makefile.am               |   7 +-
 protocol/linux-dmabuf.xml | 224 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 229 insertions(+), 2 deletions(-)
 create mode 100644 protocol/linux-dmabuf.xml

Patch hide | download patch | download mbox

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">
+
+  <copyright>
+    Copyright © 2014 Collabora, Ltd.
+
+    Permission to use, copy, modify, distribute, and sell this
+    software and its documentation for any purpose is hereby granted
+    without fee, provided that the above copyright notice appear in
+    all copies and that both that copyright notice and this permission
+    notice appear in supporting documentation, and that the name of
+    the copyright holders not be used in advertising or publicity
+    pertaining to distribution of the software without specific,
+    written prior permission.  The copyright holders make no
+    representations about the suitability of this software for any
+    purpose.  It is provided "as is" without express or implied
+    warranty.
+
+    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
+    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
+    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
+    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
+    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
+    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
+    THIS SOFTWARE.
+  </copyright>
+
+  <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. -->
+      <entry name="c8" value="0x20203843"/>
+      <entry name="rgb332" value="0x38424752"/>
+      <entry name="bgr233" value="0x38524742"/>
+      <entry name="xrgb4444" value="0x32315258"/>
+      <entry name="xbgr4444" value="0x32314258"/>
+      <entry name="rgbx4444" value="0x32315852"/>
+      <entry name="bgrx4444" value="0x32315842"/>
+      <entry name="argb4444" value="0x32315241"/>
+      <entry name="abgr4444" value="0x32314241"/>
+      <entry name="rgba4444" value="0x32314152"/>
+      <entry name="bgra4444" value="0x32314142"/>
+      <entry name="xrgb1555" value="0x35315258"/>
+      <entry name="xbgr1555" value="0x35314258"/>
+      <entry name="rgbx5551" value="0x35315852"/>
+      <entry name="bgrx5551" value="0x35315842"/>
+      <entry name="argb1555" value="0x35315241"/>
+      <entry name="abgr1555" value="0x35314241"/>
+      <entry name="rgba5551" value="0x35314152"/>
+      <entry name="bgra5551" value="0x35314142"/>
+      <entry name="rgb565" value="0x36314752"/>
+      <entry name="bgr565" value="0x36314742"/>
+      <entry name="rgb888" value="0x34324752"/>
+      <entry name="bgr888" value="0x34324742"/>
+      <entry name="xrgb8888" value="0x34325258"/>
+      <entry name="xbgr8888" value="0x34324258"/>
+      <entry name="rgbx8888" value="0x34325852"/>
+      <entry name="bgrx8888" value="0x34325842"/>
+      <entry name="argb8888" value="0x34325241"/>
+      <entry name="abgr8888" value="0x34324241"/>
+      <entry name="rgba8888" value="0x34324152"/>
+      <entry name="bgra8888" value="0x34324142"/>
+      <entry name="xrgb2101010" value="0x30335258"/>
+      <entry name="xbgr2101010" value="0x30334258"/>
+      <entry name="rgbx1010102" value="0x30335852"/>
+      <entry name="bgrx1010102" value="0x30335842"/>
+      <entry name="argb2101010" value="0x30335241"/>
+      <entry name="abgr2101010" value="0x30334241"/>
+      <entry name="rgba1010102" value="0x30334152"/>
+      <entry name="bgra1010102" value="0x30334142"/>
+      <entry name="yuyv" value="0x56595559"/>
+      <entry name="yvyu" value="0x55595659"/>
+      <entry name="uyvy" value="0x59565955"/>
+      <entry name="vyuy" value="0x59555956"/>
+      <entry name="ayuv" value="0x56555941"/>
+      <entry name="nv12" value="0x3231564e"/>
+      <entry name="nv21" value="0x3132564e"/>
+      <entry name="nv16" value="0x3631564e"/>
+      <entry name="nv61" value="0x3136564e"/>
+      <entry name="yuv410" value="0x39565559"/>
+      <entry name="yvu410" value="0x39555659"/>
+      <entry name="yuv411" value="0x31315559"/>
+      <entry name="yvu411" value="0x31315659"/>
+      <entry name="yuv420" value="0x32315559"/>
+      <entry name="yvu420" value="0x32315659"/>
+      <entry name="yuv422" value="0x36315559"/>
+      <entry name="yvu422" value="0x36315659"/>
+      <entry name="yuv444" value="0x34325559"/>
+      <entry name="yvu444" value="0x34325659"/>
+    </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="zlinux_dmabuf_create_feedback" version="1">
+    <description summary="feedback for buffer creation">
+      This interface is used to keep track of a buffer creation
+      operation.
+    </description>
+
+    <event name="create_successful">
+      <description summary="buffer creation succeeded">
+        This event indicates that the attempted buffer creation was
+        successful. It contains the id of the wl_buffer wrapping
+        the dmabuf.
+
+        Upon receiving this event, the client should destroy the
+        zlinux_dmabuf_create_feedback object.
+      </description>
+      <arg name="buffer_id" type="new_id" interface="wl_buffer"/>
+    </event>
+    <event name="create_failed">
+      <description summary="buffer creation failed">
+        This event indicates that the attempted buffer creation was
+        has failed. It usually means that one of the dmabuf constraints
+        has not been fulfilled.
+
+        Upon receiving this event, the client should destroy the
+        zlinux_dmabuf_create_feedback object.
+      </description>
+    </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 &amp; stride1 and
+        offset2 &amp; 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>
+
+  </interface>
+
+</protocol>

Comments

It looks like the purpose of "dmabuf_batch" is to send a more complex 
set of arguments to the dmabuf::create_buffer request. This is a 
variable-sized list of fd's, each containing a variable-sized list of 
planes. The Wayland protocol does not have a method of passing this as a 
request argument, so this object is used to build it with multiple 
requests. Is this correct? And is this considered the correct way to do 
this sort of thing in Wayland?

It is not clear but I assume you must add at least one fd to the 
dmabuf_batch for this to work, right?

Assuming this is correct, I think some consolidation of the objects 
would help. A few ideas:

- Make create_buffer a method on dmabuf_batch, not on dmabuf.

- Get rid of dmabuf_create_feedback object, and just reuse the 
already-existing dmabuf_batch object to deliver the success/failure event.

- I am a bit suspicious of the exactly 3 planes, there are 4:2:2 formats 
with alpha channels. I think it would be better if there was a request 
per-plane.

- Isn't there a problem with having an event deliver a new object (ie 
the wl_buffer in the create_successful event)? Can the dmabuf_batch 
object just "be" a wl_buffer, but you cannot use it as a wl_buffer until 
you get the success event?

So in the most-cleaned-up version I can come up with the api is more 
like this:

dmabuf - singleton factory used to create dmabuf_buffer.

dmabuf::create_buffer - create a new dmabuf_buffer. You must then do 
some requests on it before it can be used as a wl_buffer.

dmabuf::format event - same as you describe it

dmabuf_buffer - A subclass of wl_buffer representing a (potential) dma 
buffer.

dmabuf_buffer::add_fd - Add a new fd describing a dmabuf

dmabuf_buffer::add_plane - Add a plane (offset + stride) to map from the 
most recent fd.

dmabuf_buffer::create - try to create the dma buffer. After this you 
cannot do the add_fd/plane requests.

dmabuf_buffer::create_successful event - you can now use the 
dmabuf_buffer as a wl_buffer (for instance to attach it to a wl_surface).

dmabuf_buffer::create_failed event - It did not work. The only thing you 
are allowed to do is destroy the dmabuf_buffer.
On Tue, 16 Dec 2014 13:19:53 -0800
Bill Spitzak <spitzak@gmail.com> wrote:

> It looks like the purpose of "dmabuf_batch" is to send a more complex 
> set of arguments to the dmabuf::create_buffer request. This is a 
> variable-sized list of fd's, each containing a variable-sized list of 
> planes. The Wayland protocol does not have a method of passing this as a 
> request argument, so this object is used to build it with multiple 
> requests. Is this correct? And is this considered the correct way to do 
> this sort of thing in Wayland?

We could use wl_array, except it does not work with fd's. So yes, this
is the only way to get a variable number of fd's into a single
final request.

> It is not clear but I assume you must add at least one fd to the 
> dmabuf_batch for this to work, right?

Yes.

> Assuming this is correct, I think some consolidation of the objects 
> would help. A few ideas:
> 
> - Make create_buffer a method on dmabuf_batch, not on dmabuf.

I'm not sure what would be the difference. You can't do it without a
dmabuf_batch anyway (the spec does not have allow-null="true").

> - Get rid of dmabuf_create_feedback object, and just reuse the 
> already-existing dmabuf_batch object to deliver the success/failure event.

Then it's not a dmabuf_batch anymore, you need to think of a new name
that describes it. Using a one-shot feedback object is a standard
Wayland design pattern.

> - I am a bit suspicious of the exactly 3 planes, there are 4:2:2 formats 
> with alpha channels. I think it would be better if there was a request 
> per-plane.

This is modelled after
https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_import.txt

The EGL extension could be extended to fourth plane though, so maybe we
could use wl_array here to achieve easier extendability.

The DRM user ABI seems to use 4 indeed.

> - Isn't there a problem with having an event deliver a new object (ie 
> the wl_buffer in the create_successful event)? Can the dmabuf_batch 
> object just "be" a wl_buffer, but you cannot use it as a wl_buffer until 
> you get the success event?

There is no problem in creating a new object by an event. It is rarely
used, but it must work.

> So in the most-cleaned-up version I can come up with the api is more 
> like this:
> 
> dmabuf - singleton factory used to create dmabuf_buffer.
> 
> dmabuf::create_buffer - create a new dmabuf_buffer. You must then do 
> some requests on it before it can be used as a wl_buffer.
> 
> dmabuf::format event - same as you describe it
> 
> dmabuf_buffer - A subclass of wl_buffer representing a (potential) dma 
> buffer.

There is no such thing as sub-classing, all object types are strict and
must match exactly in the protocol (no inheritance or is-a). The
wl_buffer object must be created at some point.

> dmabuf_buffer::add_fd - Add a new fd describing a dmabuf
> 
> dmabuf_buffer::add_plane - Add a plane (offset + stride) to map from the 
> most recent fd.
> 
> dmabuf_buffer::create - try to create the dma buffer. After this you 
> cannot do the add_fd/plane requests.
> 
> dmabuf_buffer::create_successful event - you can now use the 
> dmabuf_buffer as a wl_buffer (for instance to attach it to a wl_surface).
> 
> dmabuf_buffer::create_failed event - It did not work. The only thing you 
> are allowed to do is destroy the dmabuf_buffer.

I wonder if that is simpler of just different, assuming you fix the
sub-classing.

I'd rather not do "cosmetic" changes yet, because we are likely going
to need more parameters as the dma_buf kernel interfaces are
being developed.


Thanks,
pq
On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis Ratté-Boulianne wrote:
> From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
> Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@collabora.com>

So I have no idea about how wayland protos work, so please take that into
account ;-)

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.

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.

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.

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">
> +
> +  <copyright>
> +    Copyright © 2014 Collabora, Ltd.
> +
> +    Permission to use, copy, modify, distribute, and sell this
> +    software and its documentation for any purpose is hereby granted
> +    without fee, provided that the above copyright notice appear in
> +    all copies and that both that copyright notice and this permission
> +    notice appear in supporting documentation, and that the name of
> +    the copyright holders not be used in advertising or publicity
> +    pertaining to distribution of the software without specific,
> +    written prior permission.  The copyright holders make no
> +    representations about the suitability of this software for any
> +    purpose.  It is provided "as is" without express or implied
> +    warranty.
> +
> +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> +    THIS SOFTWARE.
> +  </copyright>
> +
> +  <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?

> +      <entry name="c8" value="0x20203843"/>
> +      <entry name="rgb332" value="0x38424752"/>
> +      <entry name="bgr233" value="0x38524742"/>
> +      <entry name="xrgb4444" value="0x32315258"/>
> +      <entry name="xbgr4444" value="0x32314258"/>
> +      <entry name="rgbx4444" value="0x32315852"/>
> +      <entry name="bgrx4444" value="0x32315842"/>
> +      <entry name="argb4444" value="0x32315241"/>
> +      <entry name="abgr4444" value="0x32314241"/>
> +      <entry name="rgba4444" value="0x32314152"/>
> +      <entry name="bgra4444" value="0x32314142"/>
> +      <entry name="xrgb1555" value="0x35315258"/>
> +      <entry name="xbgr1555" value="0x35314258"/>
> +      <entry name="rgbx5551" value="0x35315852"/>
> +      <entry name="bgrx5551" value="0x35315842"/>
> +      <entry name="argb1555" value="0x35315241"/>
> +      <entry name="abgr1555" value="0x35314241"/>
> +      <entry name="rgba5551" value="0x35314152"/>
> +      <entry name="bgra5551" value="0x35314142"/>
> +      <entry name="rgb565" value="0x36314752"/>
> +      <entry name="bgr565" value="0x36314742"/>
> +      <entry name="rgb888" value="0x34324752"/>
> +      <entry name="bgr888" value="0x34324742"/>
> +      <entry name="xrgb8888" value="0x34325258"/>
> +      <entry name="xbgr8888" value="0x34324258"/>
> +      <entry name="rgbx8888" value="0x34325852"/>
> +      <entry name="bgrx8888" value="0x34325842"/>
> +      <entry name="argb8888" value="0x34325241"/>
> +      <entry name="abgr8888" value="0x34324241"/>
> +      <entry name="rgba8888" value="0x34324152"/>
> +      <entry name="bgra8888" value="0x34324142"/>
> +      <entry name="xrgb2101010" value="0x30335258"/>
> +      <entry name="xbgr2101010" value="0x30334258"/>
> +      <entry name="rgbx1010102" value="0x30335852"/>
> +      <entry name="bgrx1010102" value="0x30335842"/>
> +      <entry name="argb2101010" value="0x30335241"/>
> +      <entry name="abgr2101010" value="0x30334241"/>
> +      <entry name="rgba1010102" value="0x30334152"/>
> +      <entry name="bgra1010102" value="0x30334142"/>
> +      <entry name="yuyv" value="0x56595559"/>
> +      <entry name="yvyu" value="0x55595659"/>
> +      <entry name="uyvy" value="0x59565955"/>
> +      <entry name="vyuy" value="0x59555956"/>
> +      <entry name="ayuv" value="0x56555941"/>
> +      <entry name="nv12" value="0x3231564e"/>
> +      <entry name="nv21" value="0x3132564e"/>
> +      <entry name="nv16" value="0x3631564e"/>
> +      <entry name="nv61" value="0x3136564e"/>
> +      <entry name="yuv410" value="0x39565559"/>
> +      <entry name="yvu410" value="0x39555659"/>
> +      <entry name="yuv411" value="0x31315559"/>
> +      <entry name="yvu411" value="0x31315659"/>
> +      <entry name="yuv420" value="0x32315559"/>
> +      <entry name="yvu420" value="0x32315659"/>
> +      <entry name="yuv422" value="0x36315559"/>
> +      <entry name="yvu422" value="0x36315659"/>
> +      <entry name="yuv444" value="0x34325559"/>
> +      <entry name="yvu444" value="0x34325659"/>
> +    </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="zlinux_dmabuf_create_feedback" version="1">
> +    <description summary="feedback for buffer creation">
> +      This interface is used to keep track of a buffer creation
> +      operation.
> +    </description>
> +
> +    <event name="create_successful">
> +      <description summary="buffer creation succeeded">
> +        This event indicates that the attempted buffer creation was
> +        successful. It contains the id of the wl_buffer wrapping
> +        the dmabuf.
> +
> +        Upon receiving this event, the client should destroy the
> +        zlinux_dmabuf_create_feedback object.
> +      </description>
> +      <arg name="buffer_id" type="new_id" interface="wl_buffer"/>
> +    </event>
> +    <event name="create_failed">
> +      <description summary="buffer creation failed">
> +        This event indicates that the attempted buffer creation was
> +        has failed. It usually means that one of the dmabuf constraints
> +        has not been fulfilled.
> +
> +        Upon receiving this event, the client should destroy the
> +        zlinux_dmabuf_create_feedback object.
> +      </description>
> +    </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 &amp; stride1 and
> +        offset2 &amp; 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.

> +
> +  </interface>
> +
> +</protocol>
> -- 
> 2.1.0
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Thu, 18 Dec 2014 10:25:09 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis
> Ratté-Boulianne wrote:
> > From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
> > Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@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 &amp;
> > stride1 and
> > +        offset2 &amp; 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
On Thu, Dec 18, 2014 at 01:45:22PM +0200, Pekka Paalanen wrote:
> On Thu, 18 Dec 2014 10:25:09 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis
> > Ratté-Boulianne wrote:
> > > From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
> > > Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@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?

Yeah gl should always work. The complications really are just about trying
to get the optimum out of the plane hardware and the limited resources it
has (fifo space, limits for rotation, compressed formats and whatever
else). I expect that long-term wayland compositors need some form of hw
abstraction (like hwc from android) to really get the most out of the hw -
occasionally the limits are really crazy. That component would need to be
able to pass hints to clients a la "please use this hw specific layout for
the next frame". But that's mostly about picking the right tiling layout
or compressed format. So maybe something for a further revision (once we
have the addfb2 extension merged into the kernel).

> > 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 &amp;
> > > stride1 and
> > > +        offset2 &amp; 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?

buggy. But right now the worst-case scenario is that you wast some gpu
gart by mapping the same underlying memory multiple times on buggy
drivers. So I don't think you should care (and given that there's still
corner-cases in upstream drm drivers in this area I guess no one else does
care really either).

But as long as you don't do crazy things the drm dma-buf import will
notice that it's the same dma-buf and dtrt. Crazy = actively trying to
reimport buffers while some other thread is dropping the last drm
use-count for the same.

> 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.

Maybe we could share the drm helpers to compute the number of planes
somehow and make it part of the proto? tbh no idea whether that would help
you (I have no clue about wl protos after all ...).

Cheers, Daniel
On Mon, 5 Jan 2015 11:44:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Dec 18, 2014 at 01:45:22PM +0200, Pekka Paalanen wrote:
> > On Thu, 18 Dec 2014 10:25:09 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis
> > > Ratté-Boulianne wrote:
> > > > From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
> > > > Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@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?
> 
> Yeah gl should always work. The complications really are just about trying
> to get the optimum out of the plane hardware and the limited resources it
> has (fifo space, limits for rotation, compressed formats and whatever
> else). I expect that long-term wayland compositors need some form of hw
> abstraction (like hwc from android) to really get the most out of the hw -
> occasionally the limits are really crazy. That component would need to be
> able to pass hints to clients a la "please use this hw specific layout for
> the next frame". But that's mostly about picking the right tiling layout
> or compressed format. So maybe something for a further revision (once we
> have the addfb2 extension merged into the kernel).

Yes, a further revision sounds good for such hints. I don't think we
even know what the protocol additions should look like yet, or how a
client would use the suggestions.

...

> > > > +  <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 &amp;
> > > > stride1 and
> > > > +        offset2 &amp; 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?
> 
> buggy. But right now the worst-case scenario is that you wast some gpu
> gart by mapping the same underlying memory multiple times on buggy
> drivers. So I don't think you should care (and given that there's still
> corner-cases in upstream drm drivers in this area I guess no one else does
> care really either).

Ok. This should let us make the protocol simpler as we don't need
to maintain the exact numerical fd identities.

> But as long as you don't do crazy things the drm dma-buf import will
> notice that it's the same dma-buf and dtrt. Crazy = actively trying to
> reimport buffers while some other thread is dropping the last drm
> use-count for the same.

I'm not sure how one could even attempt such things.

> > 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.
> 
> Maybe we could share the drm helpers to compute the number of planes
> somehow and make it part of the proto? tbh no idea whether that would help
> you (I have no clue about wl protos after all ...).

Do you mean compute the number of planes from a DRM fourcc code? I
can't see how that would help protocol-wise. We have no such thing
as variable arguments. Every argument listed in the XML
specification of a message must be there exactly. If an argument is
specified to be an fd, it must also be a valid fd at runtime AFAIK.

To send 1-4 fds, one needs to either pick a message type (request)
that specifies exactly the right number of fds, or use a single
message type to send each fd one by one. The difference in IPC
efficiency is negligible, I believe.

I suppose one could use a single message type with 4 fds always,
just repeating the unwanted arguments as one of the wanted fds, but
it feels... ugly and inefficient (extra system calls to transmit
extra fds), especially if we usually need just one fd.

At least we could simplify the dmabuf_batch interface by leaving
just one offset/stride argument pair in the "add" request. That
would make it a lot more clear.

And we should bump the limit to 4 dmabufs per wl_buffer, like you
pointed out from AddFB2. The above simplification makes this very
easy. In fact, we wouldn't even need to specify any max number in
the protocol at all which is nice.


Thanks,
pq
On Thu, Jan 08, 2015 at 02:26:00PM +0200, Pekka Paalanen wrote:
> On Mon, 5 Jan 2015 11:44:49 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Dec 18, 2014 at 01:45:22PM +0200, Pekka Paalanen wrote:
> > > On Thu, 18 Dec 2014 10:25:09 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > > On Fri, Dec 12, 2014 at 04:51:02PM -0500, Louis-Francis
> > > > Ratté-Boulianne wrote:
> > > > > From: Pekka Paalanen <pekka.paalanen@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@collabora.co.uk>
> > > > > Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb@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?
> > 
> > Yeah gl should always work. The complications really are just about trying
> > to get the optimum out of the plane hardware and the limited resources it
> > has (fifo space, limits for rotation, compressed formats and whatever
> > else). I expect that long-term wayland compositors need some form of hw
> > abstraction (like hwc from android) to really get the most out of the hw -
> > occasionally the limits are really crazy. That component would need to be
> > able to pass hints to clients a la "please use this hw specific layout for
> > the next frame". But that's mostly about picking the right tiling layout
> > or compressed format. So maybe something for a further revision (once we
> > have the addfb2 extension merged into the kernel).
> 
> Yes, a further revision sounds good for such hints. I don't think we
> even know what the protocol additions should look like yet, or how a
> client would use the suggestions.
> 
> ...
> 
> > > > > +  <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 &amp;
> > > > > stride1 and
> > > > > +        offset2 &amp; 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?
> > 
> > buggy. But right now the worst-case scenario is that you wast some gpu
> > gart by mapping the same underlying memory multiple times on buggy
> > drivers. So I don't think you should care (and given that there's still
> > corner-cases in upstream drm drivers in this area I guess no one else does
> > care really either).
> 
> Ok. This should let us make the protocol simpler as we don't need
> to maintain the exact numerical fd identities.
> 
> > But as long as you don't do crazy things the drm dma-buf import will
> > notice that it's the same dma-buf and dtrt. Crazy = actively trying to
> > reimport buffers while some other thread is dropping the last drm
> > use-count for the same.
> 
> I'm not sure how one could even attempt such things.

You need your dma-buf to come from some other device (e.g. v2l pipeline),
otherwise it's impossible. And if it ever becomes an issue userspace
shouldn't ever care since the problem is in the kernel really.

> > > 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.
> > 
> > Maybe we could share the drm helpers to compute the number of planes
> > somehow and make it part of the proto? tbh no idea whether that would help
> > you (I have no clue about wl protos after all ...).
> 
> Do you mean compute the number of planes from a DRM fourcc code? I
> can't see how that would help protocol-wise. We have no such thing
> as variable arguments. Every argument listed in the XML
> specification of a message must be there exactly. If an argument is
> specified to be an fd, it must also be a valid fd at runtime AFAIK.
> 
> To send 1-4 fds, one needs to either pick a message type (request)
> that specifies exactly the right number of fds, or use a single
> message type to send each fd one by one. The difference in IPC
> efficiency is negligible, I believe.
> 
> I suppose one could use a single message type with 4 fds always,
> just repeating the unwanted arguments as one of the wanted fds, but
> it feels... ugly and inefficient (extra system calls to transmit
> extra fds), especially if we usually need just one fd.
> 
> At least we could simplify the dmabuf_batch interface by leaving
> just one offset/stride argument pair in the "add" request. That
> would make it a lot more clear.
> 
> And we should bump the limit to 4 dmabufs per wl_buffer, like you
> pointed out from AddFB2. The above simplification makes this very
> easy. In fact, we wouldn't even need to specify any max number in
> the protocol at all which is nice.

Well I was more thinking about the validation the wayland server is
supposed to do, not necessarily whether the protocol allows it. I.e. match
the input validation to what the drm core does (checks for number of
planes, that kind of stuff). Otoh I guess you could just do the addfb2
synchronously and reuse the checking the kernel does by forwarding the
-EINVAL.

I just thinking that some spec language along "the kernel's drm subsystem
is the authoritative sources for how these fourcc codes work" would be
good.
-Daniel
On Tue, 20 Jan 2015 07:41:52 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jan 08, 2015 at 02:26:00PM +0200, Pekka Paalanen wrote:
> > On Mon, 5 Jan 2015 11:44:49 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 

> > > But as long as you don't do crazy things the drm dma-buf import will
> > > notice that it's the same dma-buf and dtrt. Crazy = actively trying to
> > > reimport buffers while some other thread is dropping the last drm
> > > use-count for the same.
> > 
> > I'm not sure how one could even attempt such things.
> 
> You need your dma-buf to come from some other device (e.g. v2l pipeline),
> otherwise it's impossible. And if it ever becomes an issue userspace
> shouldn't ever care since the problem is in the kernel really.
> 
> > > > 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.
> > > 
> > > Maybe we could share the drm helpers to compute the number of planes
> > > somehow and make it part of the proto? tbh no idea whether that would help
> > > you (I have no clue about wl protos after all ...).
> > 
> > Do you mean compute the number of planes from a DRM fourcc code? I
> > can't see how that would help protocol-wise. We have no such thing
> > as variable arguments. Every argument listed in the XML
> > specification of a message must be there exactly. If an argument is
> > specified to be an fd, it must also be a valid fd at runtime AFAIK.
> > 
> > To send 1-4 fds, one needs to either pick a message type (request)
> > that specifies exactly the right number of fds, or use a single
> > message type to send each fd one by one. The difference in IPC
> > efficiency is negligible, I believe.
> > 
> > I suppose one could use a single message type with 4 fds always,
> > just repeating the unwanted arguments as one of the wanted fds, but
> > it feels... ugly and inefficient (extra system calls to transmit
> > extra fds), especially if we usually need just one fd.
> > 
> > At least we could simplify the dmabuf_batch interface by leaving
> > just one offset/stride argument pair in the "add" request. That
> > would make it a lot more clear.
> > 
> > And we should bump the limit to 4 dmabufs per wl_buffer, like you
> > pointed out from AddFB2. The above simplification makes this very
> > easy. In fact, we wouldn't even need to specify any max number in
> > the protocol at all which is nice.
> 
> Well I was more thinking about the validation the wayland server is
> supposed to do, not necessarily whether the protocol allows it. I.e. match
> the input validation to what the drm core does (checks for number of
> planes, that kind of stuff). Otoh I guess you could just do the addfb2
> synchronously and reuse the checking the kernel does by forwarding the
> -EINVAL.
> 
> I just thinking that some spec language along "the kernel's drm subsystem
> is the authoritative sources for how these fourcc codes work" would be
> good.

Ah. Foremost the compositor needs to guarantee that the wl_buffer
(a set of dmabufs and metadata) is usable in GL, so I would expect
the import checks done in EGL should be enough. If AddFB2 has
additional restrictions, that's fine: the compositor just can't
scan out the wl_buffer, but it can still composite it.

In the Wayland protocol extension, we would probably just say
something like "if the compositor cannot use this buffer, you get a
failure back instead of creating a wl_buffer object" or such.
Practically the checking would be done by the EGL implementation.

Of course, using a DRM fourcc code with a wrong number of planes
could be a fatal protocol error, just like using invalid offset or
stride is an indication of a bug rather than just an unsupported
format. That's a worthy idea to think about indeed.

How would we get this kind of centralized collection of DRM fourcc
format descriptions to run checks with?


Thanks,
pq
On Tue, Jan 20, 2015 at 10:31:52AM +0200, Pekka Paalanen wrote:
> On Tue, 20 Jan 2015 07:41:52 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Jan 08, 2015 at 02:26:00PM +0200, Pekka Paalanen wrote:
> > > On Mon, 5 Jan 2015 11:44:49 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> 
> > > > But as long as you don't do crazy things the drm dma-buf import will
> > > > notice that it's the same dma-buf and dtrt. Crazy = actively trying to
> > > > reimport buffers while some other thread is dropping the last drm
> > > > use-count for the same.
> > > 
> > > I'm not sure how one could even attempt such things.
> > 
> > You need your dma-buf to come from some other device (e.g. v2l pipeline),
> > otherwise it's impossible. And if it ever becomes an issue userspace
> > shouldn't ever care since the problem is in the kernel really.
> > 
> > > > > 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.
> > > > 
> > > > Maybe we could share the drm helpers to compute the number of planes
> > > > somehow and make it part of the proto? tbh no idea whether that would help
> > > > you (I have no clue about wl protos after all ...).
> > > 
> > > Do you mean compute the number of planes from a DRM fourcc code? I
> > > can't see how that would help protocol-wise. We have no such thing
> > > as variable arguments. Every argument listed in the XML
> > > specification of a message must be there exactly. If an argument is
> > > specified to be an fd, it must also be a valid fd at runtime AFAIK.
> > > 
> > > To send 1-4 fds, one needs to either pick a message type (request)
> > > that specifies exactly the right number of fds, or use a single
> > > message type to send each fd one by one. The difference in IPC
> > > efficiency is negligible, I believe.
> > > 
> > > I suppose one could use a single message type with 4 fds always,
> > > just repeating the unwanted arguments as one of the wanted fds, but
> > > it feels... ugly and inefficient (extra system calls to transmit
> > > extra fds), especially if we usually need just one fd.
> > > 
> > > At least we could simplify the dmabuf_batch interface by leaving
> > > just one offset/stride argument pair in the "add" request. That
> > > would make it a lot more clear.
> > > 
> > > And we should bump the limit to 4 dmabufs per wl_buffer, like you
> > > pointed out from AddFB2. The above simplification makes this very
> > > easy. In fact, we wouldn't even need to specify any max number in
> > > the protocol at all which is nice.
> > 
> > Well I was more thinking about the validation the wayland server is
> > supposed to do, not necessarily whether the protocol allows it. I.e. match
> > the input validation to what the drm core does (checks for number of
> > planes, that kind of stuff). Otoh I guess you could just do the addfb2
> > synchronously and reuse the checking the kernel does by forwarding the
> > -EINVAL.
> > 
> > I just thinking that some spec language along "the kernel's drm subsystem
> > is the authoritative sources for how these fourcc codes work" would be
> > good.
> 
> Ah. Foremost the compositor needs to guarantee that the wl_buffer
> (a set of dmabufs and metadata) is usable in GL, so I would expect
> the import checks done in EGL should be enough. If AddFB2 has
> additional restrictions, that's fine: the compositor just can't
> scan out the wl_buffer, but it can still composite it.
> 
> In the Wayland protocol extension, we would probably just say
> something like "if the compositor cannot use this buffer, you get a
> failure back instead of creating a wl_buffer object" or such.
> Practically the checking would be done by the EGL implementation.
> 
> Of course, using a DRM fourcc code with a wrong number of planes
> could be a fatal protocol error, just like using invalid offset or
> stride is an indication of a bug rather than just an unsupported
> format. That's a worthy idea to think about indeed.
> 
> How would we get this kind of centralized collection of DRM fourcc
> format descriptions to run checks with?

Copypaste from kernel maybe, perhaps after some refactoring to extract the
fourcc code checking into a mostly standalone drm_fourcc.c (functions like
drm_format_num_planes or anything else used by the input check code for
the addfb2 ioctl). Not a pretty approach, but I don't have a better idea
really.
-Daniel