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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
