[wayland-protocols] linux-dmabuf: add immediate dmabuf import path

[Pekka Paalanen]

On Fri, 11 Nov 2016 17:10:44 +0530
Varad Gautam <varadgautam at gmail.com> wrote:

> From: Varad Gautam <varad.gautam at collabora.com>
> 
> provide a mechanism that allows clients to import the added dmabufs
> and immediately receive the newly created wl_buffer handle. this is
> useful to clients that are sure of their import request succeeding,
> and wish to avoid the wl_buffer communication roundtrip.
> 
> bump zwp_linux_dmabuf_v1, zwp_linux_buffer_params_v1 interface
> versions.
> 
> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
> ---
>  unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml | 58 +++++++++++++++++++---
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> index 3b4861f..a0aa42e 100644
> --- a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> +++ b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> @@ -26,13 +26,13 @@
>      THIS SOFTWARE.
>    </copyright>
>  
> -  <interface name="zwp_linux_dmabuf_v1" version="1">
> +  <interface name="zwp_linux_dmabuf_v1" version="2">
>      <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
>        and the Linux DRM sub-system's AddFb2 ioctl.
>  
> -      This interface offers a way to create generic dmabuf-based
> +      This interface offers ways 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.
>  
> @@ -58,10 +58,16 @@
>        To create a wl_buffer from one or more dmabufs, a client creates a
>        zwp_linux_dmabuf_params_v1 object with a zwp_linux_dmabuf_v1.create_params
>        request. All planes required by the intended format are added with
> -      the 'add' request. Finally, a 'create' request is issued. The server
> -      will reply with either a 'created' event which provides the final
> +      the 'add' request. Finally, a 'create' or 'create_immed' request is
> +      issued. Depending on the request, the server will:
> +
> +      - reply with either a 'created' event which provides the final
>        wl_buffer or a 'failed' event saying that it cannot use the dmabufs
> -      provided.
> +      provided, in case of a 'create' request.
> +
> +      - reply with either a 'created_immed' event or a 'failed_immed' event to
> +      notify successful wl_buffer creation, in case of a 'create_immed'
> +      request. The created wl_buffer is returned to the client by the request.
>  
>        Warning! The protocol described in this file is experimental and
>        backward incompatible changes may be made. Backward compatible changes
> @@ -106,7 +112,7 @@
>      </event>
>    </interface>
>  
> -  <interface name="zwp_linux_buffer_params_v1" version="1">
> +  <interface name="zwp_linux_buffer_params_v1" version="2">
>      <description summary="parameters for creating a dmabuf-based wl_buffer">
>        This temporary object is a collection of dmabufs and other
>        parameters that together form a single logical buffer. The temporary
> @@ -255,6 +261,25 @@
>        <arg name="flags" type="uint" summary="see enum flags"/>
>      </request>
>  
> +    <request name="create_immed" since="2">
> +      <description summary="create and return a wl_buffer from the given dmabufs">
> +        This asks for immediate creation of a wl_buffer from added dmabufs and
> +        returns the newly created wl_buffer. On successful creation, a
> +        'created_immed' event is triggered that can be used to verify that the
> +        wl_buffer received from this request is a valid handle. A 'failed_immed'
> +        event notifies unsuccessful dmabuf import.
> +
> +        This takes the same arguments as a 'create' request, and obeys the
> +        same restrictions.
> +      </description>
> +      <arg name="buffer_id" type="new_id" interface="wl_buffer"
> +           summary="id for the newly created wl_buffer"/>
> +      <arg name="width" type="int" summary="base plane width in pixels"/>
> +      <arg name="height" type="int" summary="base plane height in pixels"/>
> +      <arg name="format" type="uint" summary="DRM_FORMAT code"/>
> +      <arg name="flags" type="uint" summary="see enum flags"/>
> +    </request>
> +
>      <event name="created">
>        <description summary="buffer creation succeeded">
>          This event indicates that the attempted buffer creation was
> @@ -277,6 +302,27 @@
>          zlinux_buffer_params object.
>        </description>
>      </event>
> +
> +    <event name="created_immed" since="2">
> +      <description summary="immediate buffer creation succeeded">
> +        This event notifies the success of 'create_immed' request.
> +
> +        Upon receiving this event, the client should destroy the
> +        zlinux_dmabuf_params object.
> +      </description>
> +    </event>
> +
> +    <event name="failed_immed" since="2">
> +      <description summary="immediate buffer creation failed">
> +        This event indicates that the attempted immediate buffer creation 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_buffer_params object.
> +      </description>
> +    </event>
> +
>    </interface>
>  
>  </protocol>

Hi,

Daniel asked me to take quick look at this, and I have a dilemma.

The original intention is that one can create a wl_buffer without the
roundtrip and use it. In that respect, there is no need for the
"created_immed" and "failed_immed" events. No-one would ever inspect
them anyway, they just assume things work. If things then don't work,
the compositor should just fire a fatal protocol error. That will kill
the client rather than letting it run oblivious of not actually showing
anything.

However, would there ever be need to have a non-fatal failure mode?
Maybe if some vendor EGL used this internally and needs to expose
errors via its API, rather than just cause a Wayland disconnection?

If yes, then the events are both necessary. But it also raises the
question of what will happen when a client commits a wl_buffer the
compositor has failed? That case needs to be specified.

Quite likely a client that cannot wait for a roundtrip will just render
into the buffer and commit it ASAP, so the wl_buffer will be used
before the client could receive any feedback. Feedback comes only after
the fact, which is contrary to the fundamental design guidelines of
Wayland, but so is the whole core idea of "create_immed" anyway.

So there is justification for both ways: 1) things cannot fail, but if
they do you die; 2) things should not fail, but if they do you get told
only after the fact.

I suspect we might want to go for the latter, but it really must be
specified what it means when a client uses a failed wl_buffer, in this
spec.

Daniel suggested undefined behaviour, and I can agree. Weston can
implement it by sending a fatal error, while someone who actually needs
graceful failures can patch their compositor to, well, do whatever is
best for them. Presumably the use cases for the graceful failure, like
for "create_immed" to begin with, are embedded use cases where the
vendor controls all the relevant software.

Undefined behaviour should also discourage desktop software stacks from
using "create_immed" without a roundtrip. By definition, using
"create_immed" without a roundtrip risks glitching, and we don't want
such software outside strictly embedded use cases.


Thanks,
pq