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

[Marius-cristian Vlad]

Apologies for inline answers. But there's no other way :(. I'll add my replies prefixed with my name.

-----Original Message-----
From: Daniel Stone [mailto:daniel at fooishbar.org] 
Sent: Wednesday, January 24, 2018 11:42 PM
To: Marius-cristian Vlad <marius-cristian.vlad at nxp.com>
Cc: wayland-devel at lists.freedesktop.org; Pekka Paalanen <pekka.paalanen at collabora.co.uk>; Keith Packard <keithp at keithp.com>
Subject: Re: [PATCH wayland-protocols] RFC: unstable: DRM lease support

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.

[mvlad] will remove it. 

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

[mvlad] Sure, I'll add more detailed info. 

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

[mvlad] I have some doubts that we can lease just parts of the DRM chain. drmModeCreateLease wants all the objects (planes,crtc,connector) to be passed in one go. Per my understanding we can lease the connector and the CRTC. The encoder for instance can't be leased. It used to, but after some re-designed of the lease it seemed no longer necessasry. One more thing: I haven't tried leasing an overlay plane, only universal planes, so I'm not sure the kernel interface allows to do that, need to check that.

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.

[mvlad] Thanks for the taking the time to explain this in more detail. I'll give it another spin soon.

Other than that, I think it looks really good, and I'd be really happy to merge it in.

Cheers,
Daniel