[PATCH wayland-protocols] RFC: unstable: DRM lease support

[Daniel Stone]

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.vlad at 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