[PATCH wayland-protocols v4] Add zwp_linux_explicit_synchronization_v1
Simon Ser
contact at emersion.fr
Thu Nov 1 22:10:40 UTC 2018
Hi Alexandros,
Here are a few comments about someone who doesn't know a lot about
explicit synchronization. Let me know if I got something wrong. :)
Overall this looks pretty good.
On Thursday, October 18, 2018 3:48 PM, Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
>
> Changes in patch v4:
> - Guarantee protocol compatibility only with zwp_linux_dmabuf buffers.
> - Add the UNSUPPORTED_BUFFER error.
>
> Changes in patch v3:
> - Reworded implicit/explicit synchronization intro in
> zwp_surface_synchronization_v1 description.
> - Removed confusing mention of wl_buffer.release in
> zwp_surface_synchronization_v1 description.
> - Clarified which fences are affected on sync object destruction.
> - Removed unclear mention about wl_buffer destruction
> in fenced_release description.
> - Clarified that the release events and their guarantees apply to
> the relevant commit only.
> - Reformatted text.
>
> Changes in patch v2:
> - Added NO_SURFACE error for zwp_surface_synchronization_v1 requests.
> - Removed restriction to destroy a zwp_surface_synchronization_v1 object
> after the associated wl_surface is destroyed.
> - Clarified which buffer the acquire fence is associated with.
> - Clarified that exactly one event, either a fenced_release or a
> immediate_release, will be emitted for each commit.
>
> patch v1 here: https://patchwork.freedesktop.org/patch/177866/
>
> Makefile.am | 1 +
> .../linux-explicit-synchronization/README | 6 +
> ...x-explicit-synchronization-unstable-v1.xml | 232 ++++++++++++++++++
> 3 files changed, 239 insertions(+)
> create mode 100644 unstable/linux-explicit-synchronization/README
> create mode 100644 unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml
>
> diff --git a/Makefile.am b/Makefile.am
> index 6394e26..7dfbb9e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -21,6 +21,7 @@ unstable_protocols = \
> unstable/xdg-output/xdg-output-unstable-v1.xml \
> unstable/input-timestamps/input-timestamps-unstable-v1.xml \
> unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \
> + unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \
> $(NULL)
>
> stable_protocols = \
> diff --git a/unstable/linux-explicit-synchronization/README b/unstable/linux-explicit-synchronization/README
> new file mode 100644
> index 0000000..f13b404
> --- /dev/null
> +++ b/unstable/linux-explicit-synchronization/README
> @@ -0,0 +1,6 @@
> +Linux explicit synchronization (dma-fence) protocol
> +
> +Maintainers:
> +David Reveman <reveman at chromium.org>
> +Daniel Stone <daniels at collabora.com>
> +Alexandros Frantzis <alexandros.frantzis at collabora.com>
diff --git a/unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml > b/unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml
> new file mode 100644
> index 0000000..4800756
> --- /dev/null
> +++ b/unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml
> @@ -0,0 +1,232 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="zwp_linux_explicit_synchronization_unstable_v1">
> +
> + <copyright>
> + Copyright 2016 The Chromium Authors.
> + Copyright 2017 Intel Corporation
> + Copyright 2018 Collabora, Ltd
> +
> + Permission is hereby granted, free of charge, to any person obtaining a
> + copy of this software and associated documentation files (the "Software"),
> + to deal in the Software without restriction, including without limitation
> + the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + and/or sell copies of the Software, and to permit persons to whom the
> + Software is furnished to do so, subject to the following conditions:
> +
> + The above copyright notice and this permission notice (including the next
> + paragraph) shall be included in all copies or substantial portions of the
> + Software.
> +
> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + DEALINGS IN THE SOFTWARE.
> + </copyright>
> +
> + <interface name="zwp_linux_explicit_synchronization_v1" version="1">
> + <description summary="protocol for providing explicit synchronization">
> + This global is a factory interface, allowing clients to request
> + explicit synchronization for buffers on a per-surface basis.
> +
> + See zwp_surface_synchronization_v1 for more information.
> +
> + This interface is derived from Chromium's
> + zcr_linux_explicit_synchronization_v1.
> +
> + Warning! The protocol described in this file is experimental and
> + backward incompatible changes may be made. Backward compatible changes
> + may be added together with the corresponding interface version bump.
> + Backward incompatible changes are done by bumping the version number in
> + the protocol and interface names and resetting the interface version.
> + Once the protocol is to be declared stable, the 'z' prefix and the
> + version number in the protocol and interface names are removed and the
> + interface version number is reset.
> + </description>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy explicit synchronization factory object">
> + Destroy this explicit synchronization factory object. Other objects,
> + including zwp_surface_synchronization_v1 objects created by this
> + factory, shall not be affected by this request.
> + </description>
> + </request>
> +
> + <enum name="error">
> + <entry name="synchronization_exists" value="0"
> + summary="the surface already has a synchronization object associated"/>
> + </enum>
> +
> + <request name="get_synchronization">
> + <description summary="extend surface interface for explicit synchronization">
> + Instantiate an interface extension for the given wl_surface to provide
> + explicit synchronization.
> +
> + If the given wl_surface already has an explicit synchronization object
> + associated, the synchronization_exists protocol error is raised.
> + </description>
> +
> + <arg name="id" type="new_id" interface="zwp_surface_synchronization_v1"
> + summary="the new synchronization interface id"/>
> + <arg name="surface" type="object" interface="wl_surface"
> + summary="the surface"/>
> + </request>
> + </interface>
> +
> + <interface name="zwp_surface_synchronization_v1" version="1">
> + <description summary="per-surface explicit synchronization support">
> + This object implements per-surface explicit synchronization.
> +
> + Synchronization refers to co-ordination of pipelined operations performed
> + on buffers. Most GPU clients will schedule an asynchronous operation to
> + render to the buffer, then immediately send the buffer to the compositor
> + to be attached to a surface.
> +
> + In implicit synchronization, ensuring that the rendering operation is
> + complete before the compositor displays the buffer is an implementation
> + detail handled by either the kernel or userspace graphics driver.
> +
> + By contrast, in explicit synchronization, dma_fence objects mark when the
> + asynchronous operations are complete. When submitting a buffer, the
> + client provides an acquire fence which will be waited on before the
> + compositor accesses the buffer. The Wayland server, through a
> + zwp_buffer_release_v1 object, will inform the client with an event which
> + may be accompanied by a release fence, when the buffer can be
> + destructively accessed.
> +
> + Each surface can be associated with only one object of this interface at
> + any time.
> +
> + Explicit synchronization is guaranteed to be supported only for buffers
> + created with any version of the zwp_linux_dmabuf buffer factory.
I think we can drop the "z" prefix here.
> + </description>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy synchronization object">
> + Destroy this explicit synchronization object.
> +
> + Any fence set by this object with set_acquire_fence since the last
> + commit will be discarded by the server. Any fences set by this object
> + before the last commit are not affected.
> +
> + zwp_buffer_release_v1 objects created by this object are not affected
> + by this request.
> + </description>
> + </request>
> +
> + <enum name="error">
> + <entry name="invalid_fence" value="0"
> + summary="the fence specified by the client could not be imported"/>
> + <entry name="duplicate_fence" value="1"
> + summary="multiple fences added for a single surface commit"/>
> + <entry name="duplicate_release" value="2"
> + summary="multiple releases added for a single surface commit"/>
> + <entry name="no_surface" value="3"
> + summary="the associated wl_surface was destroyed"/>
> + <entry name="unsupported_buffer" value="4"
> + summary="the buffer does not support explicit synchronization"/>
> + </enum>
> +
> + <request name="set_acquire_fence">
> + <description summary="set the acquire fence">
> + Set the acquire fence that must be signaled before the compositor
> + may sample from the buffer attached with wl_buffer_attach. The fence
wl_buffer.attach
> + is a dma_fence kernel object.
> +
> + The acquire fence is double-buffered state, and will be applied on the
> + next wl_surface.commit request for the associated surface. Thus, it
> + applies only to the buffer that is attached to the surface at commit
> + time.
> +
> + If the fence could not be imported, an INVALID_FENCE error is raised.
I wonder if failures to import a fence should really be protocol errors.
Protocol errors are meant to be used for protocol violations. I understand that
a client can send an invalid fence, but are there other reasons why a fence
cannot be imported? Maybe we could change this to "if the file descriptor isn't
a dma_fence"?
> + If a fence has already been attached during the same commit cycle, a
> + DUPLICATE_FENCE error is raised.
> +
> + If the associated wl_surface was destroyed, a NO_SURFACE error is
> + raised.
> +
> + If at surface commit time the attached buffer does not support explicit
> + synchronization, or there is no buffer attached, an UNSUPPORTED_BUFFER
> + error is raised.
> + </description>
> + <arg name="fd" type="fd" summary="acquire fence fd"/>
> + </request>
> +
> + <request name="get_release">
> + <description summary="release fence for last-attached buffer">
> + Create a listener for the release of the buffer attached by the
> + client with wl_buffer.attach. See zwp_buffer_release_v1
> + documentation for more information.
> +
> + The release object is double-buffered state, and will be applied
> + on the next wl_surface.commit request for the associated surface.
"will be applied" is a little bit strange for this request. Maybe change to
"will provide release information about the next wl_surface.commit request"?
> + If a zwp_buffer_release_v1 object has already been requested for
> + the surface in the same commit cycle, a DUPLICATE_RELEASE error
> + is signaled to the client.
> +
> + If the associated wl_surface was destroyed, a NO_SURFACE error
> + is signaled to the client.
> +
> + If at surface commit time the attached buffer does not support
> + explicit synchronization, or there is no buffer attached, an
> + UNSUPPORTED_BUFFER error is raised.
I agree with pq here. It would be nice to be able to use this request on any
buffer type.
> + </description>
> +
Extra line here
> + <arg name="release" type="new_id" interface="zwp_buffer_release_v1"
> + summary="new zwp_buffer_release_v1 object"/>
> + </request>
> + </interface>
> +
> + <interface name="zwp_buffer_release_v1" version="1">
> + <description summary="buffer release explicit synchronization">
> + This object is instantiated in response to a
> + zwp_surface_synchronization_v1.get_release request.
> +
> + It provides an alternative to wl_buffer.release events, providing a unique
> + release from a single wl_surface.commit request. The release event also
> + supports explicit synchronization, providing a fence FD where relevant for
> + the client to synchronize against before reusing the buffer.
> +
> + Exactly one event, either a fenced_release or an immediate_release, will
> + be emitted for each wl_surface.commit request.
This makes it sound like this object can be used for multiple commits. Maybe we
can change the wording to "will be emitted after the wl_surface.commit request".
> + This event does not replace wl_buffer.release events; servers are still
> + required to send those events.
> + </description>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy buffer release synchronization object">
> + Destroy this buffer release explicit synchronization object. The object
> + may be destroyed at any time.
> + </description>
> + </request>
> +
> + <event name="fenced_release">
> + <description summary="release buffer with fence">
> + Sent when the compositor has finalised its usage of the associated
> + buffer for the relevant commit, providing a dma_fence which will be
> + signaled when all operations by the compositor on that buffer for that
> + commit have finished.
> +
> + Clients must not perform any destructive operations (e.g. clearing or
> + rendering) on the buffer until that fence has signaled.
We should probably add to this request and to immediate_release something among
the lines of:
> Upon receiving this event, the client should destroy this object.
> + </description>
> + <arg name="fence" type="fd" summary="fence for last operation on buffer"/>
> + </event>
> +
> + <event name="immediate_release">
> + <description summary="release buffer immediately">
> + Sent when the compositor has finalised its usage of the associated
> + buffer for the relevant commit, and either performed no operations
> + using it, or has a guarantee that all its operations on that buffer for
> + that commit have finished, and any destructive operations on the buffer
> + will have no external effects.
> + </description>
> + </event>
> + </interface>
> +
> +</protocol>
> --
> 2.19.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list