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.frant...@collabora.com> wrote: > Signed-off-by: Alexandros Frantzis <alexandros.frant...@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 <reve...@chromium.org> > +Daniel Stone <dani...@collabora.com> > +Alexandros Frantzis <alexandros.frant...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
