[PATCH wayland-protocols v4] Add zwp_linux_explicit_synchronization_v1

[Alexandros Frantzis]

On Thu, Nov 01, 2018 at 10:10:40PM +0000, Simon Ser wrote:
> 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.

Hi Simon,

thanks for the review. My comments are below. I agree with everything I
have left out.

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

Hmm, not sure about this. We would be using a protocol prefix/name
combination that doesn't exist (yet). Of course the intention is clear,
but I think it would be better to update this to, e.g.,
(z)wp_linux_dmabuf, when linux_dmabuf actually becomes stable.

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

Yes, the this is all about having a valid fence (and that's how it's
implemented in the WIP branch). Requiring something more from this
request would be asking too much, both from the server and the client. I
will rephrase.

It's unlikely that we won't be able to use valid fence at a later point.
If this happens it's most likely a driver issue (or we have messed up in
weston), in which case I think disconnecting the client is a valid
option (compared to allowing bad data on screen), but that's a different
case and error. 

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

"will be applied" tries to convey that the release will be associated
with the buffer that is attached at commit time (instead of any
intermediate attachments).

So, perhaps "will be associated with the buffer that is attached to the
surface at wl_surface.commit time"?

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

Perhaps "will be emitted for the wl_surface.commit request", instead? My
concern is that "after" may be misread as the commit immediately
triggering an event (which, to be fair, doesn't make sense in this
context).

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

In v5 I am changing zwp_buffer_release to use destroy-on-event, so this
doesn't apply any more. Of course, the client should still destroy the
proxy, but I don't think this is something we need to explicitly state.

Thanks,
Alexandros