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

Fri Feb 3 15:09:45 UTC 2017

On Tue, 31 Jan 2017 18:41:52 +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 use the newly created wl_buffers without waiting on
> an event. 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)
> v3: preserve the optional protocol error added in v2 and explicitly
> state the outcome of import success or failure (pq)
> 
> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
> ---
>  unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml | 58 +++++++++++++++++++---
>  1 file changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> index ed2c4bb..ddb49cc 100644
> --- a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> +++ b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> @@ -24,13 +24,13 @@
>      DEALINGS IN THE 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.
>  
> @@ -56,10 +56,23 @@
>        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
> -      wl_buffer or a 'failed' event saying that it cannot use the dmabufs
> -      provided.
> +      the 'add' request. Finally, a 'create' or 'create_immed' request is
> +      issued, which has the following outcome depending on the import success.
> +
> +      The 'create' request,
> +      - on success, triggers a 'created' event which provides the final
> +        wl_buffer to the client.
> +      - on failure, triggers a 'failed' event to convey that the server
> +        cannot use the dmabufs received from the client.
> +
> +      For the 'create_immed' request,
> +      - on success, the server immediately imports the added dmabufs to
> +        create a wl_buffer. No event is sent from the server in this case.
> +      - on failure, the server can choose to either:
> +        - terminate the client by raising a fatal error.
> +        - create an invalid wl_buffer handle and send a 'failed' event to
> +          the client. The result of using this invalid wl_buffer in the
> +          client is left implementation-defined by the protocol.

Hi,

I would phrase the latter as:

	- Mark the wl_buffer as failed and send a 'failed' event to the
	  client. If the client uses a failed wl_buffer as an argument
	  to any request, the behaviour is compositor
	  implementation-defined.

>  
>        Warning! The protocol described in this file is experimental and
>        backward incompatible changes may be made. Backward compatible changes
> @@ -105,7 +118,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
> @@ -138,6 +151,9 @@
>               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 dmabufs from
> +               the given buffer_params"/>

I might have called it "import_failure". Not a big deal.

>      </enum>
>  
>      <request name="destroy" type="destructor">
> @@ -269,6 +285,34 @@
>          zlinux_buffer_params object.
>        </description>
>      </event>
> +
> +    <request name="create_immed" since="2">
> +      <description summary="immediately create a wl_buffer from the given
> +                     dmabufs">
> +        This asks for immediate creation of a wl_buffer by importing the
> +        added dmabufs.
> +
> +        In case of import success, no event is sent from the server, and the
> +        wl_buffer is ready to be used by the client.
> +
> +        Upon import failure, either of the following may happen, as seen fit
> +        by the implementation:
> +        - the client is terminated with a fatal protocol error.

Could mention the error code here, since there is one specifically for
this case.

> +        - the server creates an invalid wl_buffer and sends a 'failed' event
> +          to the client. The result of using this invalid wl_buffer created
> +          by the server is implementation defined.

The same rephrasing as above.

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

Looking good, just a couple cosmetics I'd wish to see adjusted, but
regardless:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Thanks,
pq