Hi Marius, Thanks a lot for taking this on! It would be great to get this merged.
On 24 January 2018 at 19:09, Marius Vlad <marius-cristian.v...@nxp.com> wrote: > + <interface name="zwp_drm_lease_v1" version="1"> > + <description summary="drm lease"> > + This interface makes use of DRM lease written by Keith Packard. > + It requires libdrm at least 2.4.89 and a recent (4.15) kernel. A serious nitpick, but we can leave the versions out of this, especially as people will backport support to older versions. > + Events: > + > + - created -- event sent when the lease has been created succesfully > + - revoked -- event sent when the lease has been revoked succesfully > + - failed -- event sent when create/revoke requests failed. > + > + Requests: > + > + - create -- asks the server to create a lease. The server decides which > + ouput to lease to the client. In case of success, the client receives > + an event with a DRM capable-fd. The client can then issue libdrm > + ioctls (i.e., modesetting). The client also receives a lessee_id which > + has be used in revoke request. In case of failure, a failed event will > + be sent. > + - revoke -- asks the server to revoke a previously leased fd, using the > + lessee_id. > + A revoke event will be sent in case of success or failed event > otherwise. Also, the events/requests are already described in the actual protocol. A couple of paragraphs about what DRM leases actually are, and why you would/wouldn't want to use them, would be more useful I think. > + <request name="create"> > + <description summary="create a lease"> > + This asks for the creation of a lease. > + </description> > + </request> > + > + <event name="created"> > + <description summary="drm lease created successfully"> > + This event indicates that the lease has been created. It provides the > + leased fd which the client can use to perform modesetting and a lessee > + id to revoke the lease when it has finished with it. > + </description> > + <arg name="fd" type="fd" summary="leased fd"/> > + <arg name="id" type="uint" summary="lessee id"/> > + </event> > + > + <event name="failed"> > + <description summary="drm lease could not be created"> > + This event indicates that the lease could not be created/revoked. > + </description> > + </event> > + > + <request name="revoke"> > + <description summary="revoke a lease"> > + This asks to revoke a lease using the lessee id previously given in > event > + created. > + </description> > + <arg name="id" type="uint" summary="lessee id"/> > + </request> > + > + <event name="revoked"> > + <description summary="lease revoked"> > + This event indicates that the leased has been revoked. > + </description> > + </event> This interface seems a little idiosyncratic. Essentially, the client asks for creation of one lease (any lease), and the server returns it a lease with an ID. After that, the client destroys all the leases through the same interface. There are a couple of things I would suggest to improve this protocol, and make it more like other Wayland protocols: Most Wayland protocols carry lots of small objects, since creating them is lightweight and straightforward. I think this protocol could be improved by doing something similar to the dmabuf protocol, which carries three objects: one global which is pretty much just for extension advertisement, one temporary object used in the creation of a buffer, and then the buffer object itself. Applied to leases, this would be a zwp_kms_lease_manager_v1 global which only had two requests: one destroy, and the other to create a wp_kms_lease_request object. zwp_kms_lease_request_v1 would be analagous to zwp_linux_dmabuf_params_v1: it would have requests to lease particular parts of the device (e.g. HDMI-2 output as well as two planes), and successful/failed events. The successful event would carry a zwp_kms_lease_v1 object, which would represent the actual lease itself, only having a 'destroy' method. Essentially, this object would do nothing but represent the lifetime of a lease. This is more idiomatic: as a rule of thumb, any time a request/event takes an 'id' parameter which you have to look up in a list, this means you should probably be using an object instead. Other than that, I think it looks really good, and I'd be really happy to merge it in. Cheers, Daniel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel