[wayland-protocols,v2] Add zwp_linux_explicit_synchronization_v1

Submitted by Alexandros Frantzis on Oct. 9, 2018, 3:11 p.m.

Details

Message ID 20181009151122.12690-1-alexandros.frantzis@collabora.com
State Superseded
Headers show
Series "Add zwp_linux_explicit_synchronization_v1" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Alexandros Frantzis Oct. 9, 2018, 3:11 p.m.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
---

patch v1 here: https://patchwork.freedesktop.org/patch/177866/
Changes since patch v1:
  - Add NO_SURFACE error for zwp_surface_synchronization_v1 requests.
  - Remove restriction to destroy a zwp_surface_synchronization_v1 object
    after the associated wl_surface is destroyed.
  - Clarify which buffer the acquire fence is associated with.
  - Clarify that exactly one event, either a fenced_release or a
    immediate_release, will be emitted for each commit.

 Makefile.am                                   |   1 +
 .../linux-explicit-synchronization/README     |   6 +
 ...x-explicit-synchronization-unstable-v1.xml | 222 ++++++++++++++++++
 3 files changed, 229 insertions(+)
 create mode 100644 unstable/linux-explicit-synchronization/README
 create mode 100644 unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml

Patch hide | download patch | download mbox

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@chromium.org>
+Daniel Stone <daniels@collabora.com>
+Alexandros Frantzis <alexandros.frantzis@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..11ef3f0
--- /dev/null
+++ b/unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml
@@ -0,0 +1,222 @@ 
+<?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.
+
+      Explicit 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.
+      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. This is referred
+      to as 'implicit synchronization'.
+
+      By contrast, explicit synchronization takes dma_fence objects as a
+      marker of when the asynchronous operations are complete. The fence
+      provided by the client will be waited on before the compositor
+      accesses the buffer. Conversely, in place of wl_buffer.release
+      events, the Wayland server will inform the client when the buffer
+      can be destructively accessed through a zwp_buffer_release_v1
+      object.
+
+      This interface cannot be instantiated multiple times for a single
+      surface, and as such should only be used by the component which
+      performs the wl_surface.attach request. Whenever control of
+      buffer attachments is released, the releasing component should
+      ensure it destroys the zwp_surface_synchronization_v1 object.
+    </description>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy synchronization object">
+        Destroy this explicit synchronization object.
+
+        Any fence set with set_acquire_fence in this commit cycle will
+        be invalidated in the server.
+
+        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"/>
+    </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
+        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 signaled
+        to the client.
+
+        If a fence has already been attached during the same commit cycle,
+        a DUPLICATE_FENCE error is signaled to the client.
+
+        If the associated wl_surface was destroyed, a NO_SURFACE error
+        is signaled to the client.
+      </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.
+
+        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.
+      </description>
+
+      <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 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 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, providing a dma_fence which will be signaled when all
+        operations by the compositor on that buffer have finished.
+
+        Clients must not perform any destructive operations (e.g. clearing or
+        rendering) on the buffer until that fence has passed. They may,
+        however, destroy the wl_buffer 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, and either performed no operations using it, or has a
+        guarantee that all its operations have finished and no more external
+        effects will be observed from these operations.
+      </description>
+    </event>
+  </interface>
+
+</protocol>

Comments

Hi,

this is a good specification, all my comments are clarifications or
minor adjustments. The fundamental design looks fine to me.


On Tue,  9 Oct 2018 18:11:22 +0300
Alexandros Frantzis <alexandros.frantzis@collabora.com> wrote:

> Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
> ---
> 
> patch v1 here: https://patchwork.freedesktop.org/patch/177866/
> Changes since patch v1:
>   - Add NO_SURFACE error for zwp_surface_synchronization_v1 requests.
>   - Remove restriction to destroy a zwp_surface_synchronization_v1 object
>     after the associated wl_surface is destroyed.
>   - Clarify which buffer the acquire fence is associated with.
>   - Clarify that exactly one event, either a fenced_release or a
>     immediate_release, will be emitted for each commit.
> 
>  Makefile.am                                   |   1 +
>  .../linux-explicit-synchronization/README     |   6 +
>  ...x-explicit-synchronization-unstable-v1.xml | 222 ++++++++++++++++++
>  3 files changed, 229 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@chromium.org>
> +Daniel Stone <daniels@collabora.com>
> +Alexandros Frantzis <alexandros.frantzis@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..11ef3f0
> --- /dev/null
> +++ b/unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml
> @@ -0,0 +1,222 @@
> +<?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.
> +
> +      Explicit synchronization refers to co-ordination of pipelined

Strike "Explicit"? It's odd because this starts with "explicit
synchronization refers to..." and then ends saying this is "implicit
synchronization.

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

Should there be a paragraph break here?

> +      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. This is referred
> +      to as 'implicit synchronization'.
> +
> +      By contrast, explicit synchronization takes dma_fence objects as a
> +      marker of when the asynchronous operations are complete. The fence
> +      provided by the client will be waited on before the compositor
> +      accesses the buffer. Conversely, in place of wl_buffer.release
> +      events, the Wayland server will inform the client when the buffer
> +      can be destructively accessed through a zwp_buffer_release_v1
> +      object.

So this guarantees that no more wl_buffer.release events can be triggered
by commits that used explicit sync?

> +
> +      This interface cannot be instantiated multiple times for a single
> +      surface, and as such should only be used by the component which
> +      performs the wl_surface.attach request. Whenever control of

> +      buffer attachments is released, the releasing component should
> +      ensure it destroys the zwp_surface_synchronization_v1 object.

Is the last sentence necessary? It might confuse things as it can
easily be read as if zwp_surface_synchronization_v1 was a one-shot
thing.

> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy synchronization object">
> +        Destroy this explicit synchronization object.
> +
> +        Any fence set with set_acquire_fence in this commit cycle will
> +        be invalidated in the server.

This means that if zwp_surface_synchronization_v1 object is
destroyed before issuing wl_surface.commit, then the pending acquire
fence is discarded by the server, right?

> +
> +        zwp_buffer_release_v1 objects created by this object are not affected
> +        by this request.

And after wl_surface.commit, destruction has no effect on the commit.

> +      </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"/>
> +    </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
> +        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 signaled
> +        to the client.

"... error is raised." There is no need to be able to recover from a
failed fence import, disconnection is fine, right?

> +
> +        If a fence has already been attached during the same commit cycle,
> +        a DUPLICATE_FENCE error is signaled to the client.
> +
> +        If the associated wl_surface was destroyed, a NO_SURFACE error
> +        is signaled to the client.
> +      </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.
> +
> +        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.
> +      </description>
> +
> +      <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 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 event does not replace wl_buffer.release events; servers are still
> +      required to send those events.

Ok. The introduction lead me to believe otherwise, that should probably
be worded differently.

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

Is it inconceivable to ever add other requests in this interface?

Could there ever be a benefit from destroying this object before it has
sent an event?

Could this object ever be used as an argument to a request?

If not, this seems like the perfect candidate for destroy-on-event
behaviour, similar to wp_presentation_feedback. But if there is any
suspicion about extending this interface, then it's better to keep the
destroy request.

> +    </request>
> +
> +    <event name="fenced_release">
> +      <description summary="release buffer with fence">
> +        Sent when the compositor has finalised its usage of the associated
> +        buffer, providing a dma_fence which will be signaled when all
> +        operations by the compositor on that buffer have finished.
> +
> +        Clients must not perform any destructive operations (e.g. clearing or
> +        rendering) on the buffer until that fence has passed. They may,
> +        however, destroy the wl_buffer object.

"...without any visible effects" I would add. Clients may destroy a
wl_buffer at any time, but at a "wrong" time it may lead to visual
glitches. This even here says that no glitches can happen. Unless, of
course, the wl_buffer has been submitted again or elsewhere already.

Or maybe just remove the note about destroying the wl_buffer? Strictly
speaking this event alone is not sufficient, there must not be other
unreleased uses of the wl_buffer.

> +      </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, and either performed no operations using it, or has a
> +        guarantee that all its operations have finished and no more external
> +        effects will be observed from these operations.

Right. Should there be explicit wording that this applies to the
specific commit, and not the buffer's usage in general?

After all, this is the extension that makes it well-defined to use a
wl_buffer again (with its contents intact) before it has been released,
providing a solution to
https://gitlab.freedesktop.org/wayland/wayland/issues/46 .

> +      </description>
> +    </event>
> +  </interface>
> +
> +</protocol>


Thanks,
pq
On Fri, Oct 12, 2018 at 03:24:30PM +0300, Pekka Paalanen wrote:
> Hi,
> 
> this is a good specification, all my comments are clarifications or
> minor adjustments. The fundamental design looks fine to me.

Hi Pekka,

thanks for the review! My answers are inline. I have sent a updated
version (v3) of this proposal based on this discussion.

> On Tue,  9 Oct 2018 18:11:22 +0300
> Alexandros Frantzis <alexandros.frantzis@collabora.com> wrote:
> 
> > Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
> > ---
> > 
> > patch v1 here: https://patchwork.freedesktop.org/patch/177866/
> > Changes since patch v1:
> >   - Add NO_SURFACE error for zwp_surface_synchronization_v1 requests.
> >   - Remove restriction to destroy a zwp_surface_synchronization_v1 object
> >     after the associated wl_surface is destroyed.
> >   - Clarify which buffer the acquire fence is associated with.
> >   - Clarify that exactly one event, either a fenced_release or a
> >     immediate_release, will be emitted for each commit.
> > 
> >  Makefile.am                                   |   1 +
> >  .../linux-explicit-synchronization/README     |   6 +
> >  ...x-explicit-synchronization-unstable-v1.xml | 222 ++++++++++++++++++
> >  3 files changed, 229 insertions(+)
> >  create mode 100644 unstable/linux-explicit-synchronization/README
> >  create mode 100644 unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml

<snip>

> > +  <interface name="zwp_surface_synchronization_v1" version="1">
> > +    <description summary="per-surface explicit synchronization support">
> > +      This object implements per-surface explicit synchronization.
> > +
> > +      Explicit synchronization refers to co-ordination of pipelined
> 
> Strike "Explicit"? It's odd because this starts with "explicit
> synchronization refers to..." and then ends saying this is "implicit
> synchronization.

See below.

> > +      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.
> 
> Should there be a paragraph break here?

See below.

> > +      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. This is referred
> > +      to as 'implicit synchronization'.
> > +
> > +      By contrast, explicit synchronization takes dma_fence objects as a
> > +      marker of when the asynchronous operations are complete. The fence
> > +      provided by the client will be waited on before the compositor
> > +      accesses the buffer. Conversely, in place of wl_buffer.release
> > +      events, the Wayland server will inform the client when the buffer
> > +      can be destructively accessed through a zwp_buffer_release_v1
> > +      object.
> 
> So this guarantees that no more wl_buffer.release events can be triggered
> by commits that used explicit sync?

See below.

> > +
> > +      This interface cannot be instantiated multiple times for a single
> > +      surface, and as such should only be used by the component which
> > +      performs the wl_surface.attach request. Whenever control of
> 
> > +      buffer attachments is released, the releasing component should
> > +      ensure it destroys the zwp_surface_synchronization_v1 object.
> 
> Is the last sentence necessary? It might confuse things as it can
> easily be read as if zwp_surface_synchronization_v1 was a one-shot
> thing.

Ack on all of the above. In v3 I have reworded a big part of this
section, taking into account the points you brought up.

> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy synchronization object">
> > +        Destroy this explicit synchronization object.
> > +
> > +        Any fence set with set_acquire_fence in this commit cycle will
> > +        be invalidated in the server.
> 
> This means that if zwp_surface_synchronization_v1 object is
> destroyed before issuing wl_surface.commit, then the pending acquire
> fence is discarded by the server, right?

Yes, reworded to make this more clear.

> > +
> > +        zwp_buffer_release_v1 objects created by this object are not affected
> > +        by this request.
> 
> And after wl_surface.commit, destruction has no effect on the commit.
> 

I have clarified this in the previous paragraph about set_acquire_fence. 

> > +      </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"/>
> > +    </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
> > +        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 signaled
> > +        to the client.
> 
> "... error is raised." There is no need to be able to recover from a
> failed fence import, disconnection is fine, right?

I think disconnection is fine.

This is a client-side error, and the server has no reasonable way to
continue with an invalid fence, since it doesn't know if it safe to use
the committed buffer. One option would be for the server to ignore the
fence, but since the client explicitly requested set_acquire_fence, it's
a good sign that the buffer may not be ready at commit time.

<snip>

> > +  <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 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 event does not replace wl_buffer.release events; servers are still
> > +      required to send those events.
> 
> Ok. The introduction lead me to believe otherwise, that should probably
> be worded differently.

I have remove all mention of wl_buffer.release from the intro.

> > +    </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>
> 
> Is it inconceivable to ever add other requests in this interface?

I don't expect other requests to be added, this is meant as
an event-only interface.

> Could there ever be a benefit from destroying this object before it has
> sent an event?

The main scenario is an early exit from some code using this
object, in which case it will be easier to correctly synchronize proper
destruction of any user data used by the zwp_buffer_release_v1 listener,
when having an explicit destroy request.

This isn't particular to this protocol though, it's a general
(theoretical) concern of mine with the destroy-on-event pattern.  But if
this has worked well for wp_presentation_feedback, perhaps it's not a big
deal.

> Could this object ever be used as an argument to a request?

I can't think of any cases where this would be particularly useful.  In
a wild scenario we could pass this as a meta-fence object, but that
would probably require a better wayland abstraction anyway.

> If not, this seems like the perfect candidate for destroy-on-event
> behaviour, similar to wp_presentation_feedback. But if there is any
> suspicion about extending this interface, then it's better to keep the
> destroy request.

I agree that it's a good match, with only the 2nd point concerning me a
bit. 

Anyway, this change is not in v3, but if previous experience has shown
that my concern is not issue, I am happy to update this in v4.

> > +    </request>
> > +
> > +    <event name="fenced_release">
> > +      <description summary="release buffer with fence">
> > +        Sent when the compositor has finalised its usage of the associated
> > +        buffer, providing a dma_fence which will be signaled when all
> > +        operations by the compositor on that buffer have finished.
> > +
> > +        Clients must not perform any destructive operations (e.g. clearing or
> > +        rendering) on the buffer until that fence has passed. They may,
> > +        however, destroy the wl_buffer object.
> 
> "...without any visible effects" I would add. Clients may destroy a
> wl_buffer at any time, but at a "wrong" time it may lead to visual
> glitches. This even here says that no glitches can happen. Unless, of
> course, the wl_buffer has been submitted again or elsewhere already.
> 
> Or maybe just remove the note about destroying the wl_buffer? Strictly
> speaking this event alone is not sufficient, there must not be other
> unreleased uses of the wl_buffer.

Removed, and updated this section a bit.

> > +      </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, and either performed no operations using it, or has a
> > +        guarantee that all its operations have finished and no more external
> > +        effects will be observed from these operations.
> 
> Right. Should there be explicit wording that this applies to the
> specific commit, and not the buffer's usage in general?
>
> After all, this is the extension that makes it well-defined to use a
> wl_buffer again (with its contents intact) before it has been released,
> providing a solution to
> https://gitlab.freedesktop.org/wayland/wayland/issues/46 .

I have tried to make this more explicit, both in this and the previous
section.

Thanks,
Alexandros
On Mon, 15 Oct 2018 17:16:59 +0300
Alexandros Frantzis <alexandros.frantzis@collabora.com> wrote:

> On Fri, Oct 12, 2018 at 03:24:30PM +0300, Pekka Paalanen wrote:
> > Hi,
> > 
> > this is a good specification, all my comments are clarifications or
> > minor adjustments. The fundamental design looks fine to me.  
> 
> Hi Pekka,
> 
> thanks for the review! My answers are inline. I have sent a updated
> version (v3) of this proposal based on this discussion.
> 
> > On Tue,  9 Oct 2018 18:11:22 +0300
> > Alexandros Frantzis <alexandros.frantzis@collabora.com> wrote:
> >   
> > > Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>

...

> > > +  <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 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 event does not replace wl_buffer.release events; servers are still
> > > +      required to send those events.  
> > 
> > Ok. The introduction lead me to believe otherwise, that should probably
> > be worded differently.  
> 
> I have remove all mention of wl_buffer.release from the intro.
> 
> > > +    </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>  
> > 
> > Is it inconceivable to ever add other requests in this interface?  
> 
> I don't expect other requests to be added, this is meant as
> an event-only interface.
> 
> > Could there ever be a benefit from destroying this object before it has
> > sent an event?  
> 
> The main scenario is an early exit from some code using this
> object, in which case it will be easier to correctly synchronize proper
> destruction of any user data used by the zwp_buffer_release_v1 listener,
> when having an explicit destroy request.
> 
> This isn't particular to this protocol though, it's a general
> (theoretical) concern of mine with the destroy-on-event pattern.  But if
> this has worked well for wp_presentation_feedback, perhaps it's not a big
> deal.

Hi Alf,

I'm not sure what you mean with that concern.

When an event destroys a protocol object, the compositor will
unconditionally destroy the wl_resource right after sending the event.
That means the server-side request listener cannot receive any messages
anymore, so any user data would be destroyed at the same time anyway.

On client side, regardless of whether there is destroy request or not,
the client will destroy the wl_proxy. The request would only let the
compositor know that the protocol object is no more. Regardless of a
destroy request, the client side will automatically ignore any events
to the destroyed wl_proxy. That ignoring is what makes client initiated
object destruction safe in the first place.

When the client destroys the wl_proxy, it can also free any user data
associated with it, because that will guarantee that the listeners
cannot be called anymore.

If we have a destructor event, and the client destroys the wl_proxy
before that event is sent, then the event will simply be ignored. Once
the compositor sends the event, then both client and compositor again
agree that the protocol object no longer exists.

wl_display has a "secret" event that tells libwayland-client when the
server has destroyed the protocol object, which makes all the above
work.


Thanks,
pq
On Wed, Oct 31, 2018 at 11:59:34AM +0200, Pekka Paalanen wrote:
> On Mon, 15 Oct 2018 17:16:59 +0300
> Alexandros Frantzis <alexandros.frantzis@collabora.com> wrote:

...

> > 
> > The main scenario is an early exit from some code using this
> > object, in which case it will be easier to correctly synchronize proper
> > destruction of any user data used by the zwp_buffer_release_v1 listener,
> > when having an explicit destroy request.
> > 
> > This isn't particular to this protocol though, it's a general
> > (theoretical) concern of mine with the destroy-on-event pattern.  But if
> > this has worked well for wp_presentation_feedback, perhaps it's not a big
> > deal.
> 
> Hi Alf,
> 
> I'm not sure what you mean with that concern.
> 
> When an event destroys a protocol object, the compositor will
> unconditionally destroy the wl_resource right after sending the event.
> That means the server-side request listener cannot receive any messages
> anymore, so any user data would be destroyed at the same time anyway.
> 
> On client side, regardless of whether there is destroy request or not,
> the client will destroy the wl_proxy. The request would only let the
> compositor know that the protocol object is no more. Regardless of a
> destroy request, the client side will automatically ignore any events
> to the destroyed wl_proxy. That ignoring is what makes client initiated
> object destruction safe in the first place.
> 
> When the client destroys the wl_proxy, it can also free any user data
> associated with it, because that will guarantee that the listeners
> cannot be called anymore.
> 
> If we have a destructor event, and the client destroys the wl_proxy
> before that event is sent, then the event will simply be ignored. Once
> the compositor sends the event, then both client and compositor again
> agree that the protocol object no longer exists.
> 
> wl_display has a "secret" event that tells libwayland-client when the
> server has destroyed the protocol object, which makes all the above
> work.
> 
> 
> Thanks,
> pq

Hi Pekka,

thanks for the detailed explanation. I was misunderstanding how
destroy-on-event is expected to work.

I'll update the protocol to use destroy-on-event in v5.

Thanks,
Alexandros