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

[Pekka Paalanen]

On Thu, 19 Jan 2017 12:50:07 +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.
> 
> v2: specify using incorrectly imported dmabufs as undefined behavior
> instead of sending success/failure events. (pq, daniels)
> 
> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
> ---
>  unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml | 39 ++++++++++++++++++----
>  1 file changed, 33 insertions(+), 6 deletions(-)

Hi Varad,

I think there is still some to tune.

> diff --git a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> index 60240f9..22e7af9 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,15 @@
>        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.
> +
> +      - return a newly created wl_buffer from the imported dmabufs, in case
> +      of a 'create_immed' request.

In this case the server does not return anything, the wl_buffer exists
when 'create_immed' has been sent to the server.

>  
>        Warning! The protocol described in this file is experimental and
>        backward incompatible changes may be made. Backward compatible changes
> @@ -107,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
> @@ -140,6 +145,8 @@
>               summary="invalid width or height"/>
>        <entry name="out_of_bounds" value="6"
>               summary="offset + stride * height goes out of dmabuf bounds"/>
> +      <entry name="invalid_wl_buffer" value="7"
> +             summary="invalid wl_buffer resulted from importing given buffer_params"/>
>      </enum>
>  
>      <request name="destroy" type="destructor">
> @@ -271,6 +278,26 @@
>          zlinux_buffer_params object.
>        </description>
>      </event>
> +
> +    <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 the added dmabufs
> +        and returns the newly created wl_buffer. The result of using a
> +        wl_buffer handle returned by this request is undefined in the case
> +        when the server fails to import the supplied dmabufs, and the server
> +        may raise an optional fatal invalid_wl_buffer error.
> +
> +        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>
> +
>    </interface>
>  
>  </protocol>

I see you have dropped the 'created_immed' and 'failed_immed' events,
but still went for the "undefined behaviour if used". This has the
problem that now there is no way to have a graceful failure with an
eventual corrective action in the client. If it fails, then either the
compositor is not showing it the and client does not know, or the
client gets disconnected.

The speculated EGL or some other library use case would be to use
'create_immed' and if it turns out to fail, return failure from some
API call after the fact. This is bad design, yes, because the user will
likely see a glitch on the screen, but implementing a bad design is the
whole point of this patch. However, I think being able to recover later
is better than not recover at all.

I think we should keep:

- 'created_immed', because otherwise the client is left with
  uncertainty on what happened when things do work. The client needs to
  be able to stop waiting, if it chooses to wait.

- 'failed_immed', so that the client can know if the import failed and
  it needs to do something else.

It is up to the client to wait or not wait for the above feedback
before using the wl_buffer.

If the client decides to not wait, it is possible that it uses a
failed wl_buffer. What happens in that case is orthogonal to having the
events, and needs to be explained in the spec to be undefined: the
compositor may choose to show nothing, show garbage, unmap the surface,
send a fatal protocol error, or do something else.

Sorry for not being too clear earlier.


Thanks,
pq