[PATCH wayland-protocols v5] unstable/drm-lease: DRM lease protocol support

Wed Sep 5 06:55:46 UTC 2018

Hi Marius,

thank you for the update!

Am Dienstag, den 04.09.2018, 17:39 +0300 schrieb Marius Vlad:
> Simple protocol extension to manage DRM lease. Based on the work by
> Keith
> Packard in [1], respectively [2].
> 
> [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f
> 
> Changes since v4:
[...]
> - removed 'revoked' event entirely as it adds complexity without adding
> too much benefit.

The client will notice this via the leased drm fd sooner or later
anyway, so it seems that this event is not strictly necessary. I wonder
if there is any value in letting the client know immediately, though.
For example, if a client displays mostly static content (like a
presentation running on a non-desktop projector), it could be a long
while until the next page flip attempt.

> Signed-off-by: Marius Vlad <marius-cristian.vlad at nxp.com>
> ---
> 
> An implementation for this version can be found at
> https://gitlab.freedesktop.org/marius.vlad0/weston/commits/drm-lease-advertise-each-connnector-object-fixes
> 

I tried the current weston implementation with this protocol version,
and I think the leased property and lease object have to be moved from
output to head.
Currently, all heads without enabled output are skipped, and trying to
lease a disabled monitor with:

[output]
name=HDMI-A-1
leasable=on
mode=off

crashes the compositor.

[...]
> +      Upon connecting to the compositor all leasable connectors will be
> +      advertised to the client.

What happens if a client has received the full list of leasable
connectors and then one of those becomes unavailable, either because it
is physically unplugged, or because it is leased to another client?
Is the client notified about this?

What happens if a new connector becomes available, either because it is
physically plugged in, or because another client cancels its lease?
This could be handled by sending the connector advertisement (again),
in which case leasable connectors could appear any time, not only upon
connecting.

>  These connectors are represented by
> +      zwp_kms_lease_connector_v1 interface, and have to be "added" to a lease
> +      request object before creating a lease object. This instructs the
> +      compositor to use that connector when creating a lease. The client can
> +      receive multiple events for multiple leasable connectors and needs a way
> +      to discern between various leasable connectors. When receiving this
> +      connector object, events will be sent gratuitously so that the
> +      client can properly choose which connector wants to use.
> +
> +      A lease request object is represented by zwp_kms_lease_request_v1
> +      interface and is used temporarily as a storage place for objects

s/objects/object/

> +      IDs. Once the client has a lease object it can freely call its
> +      destructor, zwp_kms_lease_request_v1::destroy.
> +
> +      zwp_kms_lease_request_v1::created event will provide a lease object
> +      represented by zwp_kms_lease_v1 interface. The client will then use
> +      this lease object to retrieve the file-descriptor (leased fd) and
> +      use it to perform mode-setting/atomic operations.
> +
> +      Whilst the operation to revoke a lease requires a lesse id, in our case,
> +      calling the destructor for the lease object will cause the lease to be
> +      revoked.

The lessee ID detail is not necessary in the wayland protocol
description. The above paragraph could be replaced with a list of ways
the lease can be terminated:

- The client closes the leased drm fd.
- The compositor revokes the lease because objects in the lease become
  unavailable for leasing.

> +
> +      Warning! The protocol described in this file is experimental and
> +      backward incompatible changes may be made. Backward compatible changes
> +      may be added together with the corresponding interface version bump.
> +      Backward incompatible changes are done by bumping the version number in
> +      the protocol and interface names and resetting the interface version.
> +      Once the protocol is to be declared stable, the 'z' prefix and the
> +      version number in the protocol and interface names are removed and the
> +      interface version number is reset.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the manager">
> +        Destroys the manager object and should be called last.
> +      </description>
> +    </request>
> +
> +    <request name="create_lease_req">
> +      <description summary="create a temporary object for managing the lease">

Maybe "create a temporary object for managing a lease"?
There could be multiple leases.

> +        Create an object for temporarily storing all the KMS resources to be leased.
> +      </description>
> +      <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"
> +           summary="the new temporary"/>
> +    </request>
> +
> +    <event name="connector">
> +      <description summary="advertise which connector can be used to request a lease">

Maybe "advertise a connector that can be used to request a lease"?

> +        This event advertises a leasable connector object. When creating a
> +        lease pass this object to zwp_kms_lease_request_v1::add_connector. As
> +        multiple connectors can be leasable (based on compositor policy),
> +        zwp_kms_lease_connector_v1 will send gratuitously events in order
> +        to distinguish between different leasable connectors.
> +        After the client has added (using
> +        zwp_kms_lease_request_v1::add_connector) a leasable
> +        connector object, zwp_kms_lease_request_v1::create request should be
> +        called for creating a zwp_kms_lease_v1 lease object.
> +      </description>
> +      <arg name="conn_obj" type="new_id" interface="zwp_kms_lease_connector_v1"
> +      summary="the new temporary" />
> +    </event>
> +
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_v1" version="1">
> +    <description summary="the lease object itself">
> +      This interface represents the lease object and encapsulates the lease
> +      properties. This objected is sent by zwp_kms_lease_request_v1::created
> +      event. Use this object to retrieve lease properties.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the temporary lease request object">
> +        This request ask the compositor to revoke the lease, destroy the lease
> +        object and free temporary storage.
> +      </description>
> +    </request>
> +
> +    <request name="retrieve_leased_fd">
> +      <description summary="request to retrieve info about the lease">
> +        Request to retrieve the leased file-descriptor.
> +      </description>
> +    </request>

Same issue as for the connector "name" event, this request could be
removed if the "leased_fd" event is just sent automatically when the
listener is bound.

> +
> +    <event name="leased_fd">
> +      <description summary="returns information about the lease">
> +        This event returns the leased fd which is required for modesetting
> +        or querying page flips/atomic modesetting.
> +        The client can use the leased fd to find out DRM-related information
> +        like connector ID, CRTC ID, and plane ID using drmModeGetLease().
> +        Using that information it can derive a suitable mode useful
> +        when performing a modeset.

It would be useful to mention that closing the leased fd can be used to
terminate the lease.

> +      </description>
> +      <arg name="leased_fd" type="fd" summary="leased fd" />
> +    </event>
> +
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_connector_v1" version="1">
> +    <description summary="zwp_kms_lease_connector_v1">
> +      This interface describes a connector object advertised by
> +      zwp_kms_lease_manager_v1::connector.
> +    </description>
> +
> +    <event name="name">
> +      <description summary="name">
> +         Event sent gratuitously to the client representing the connector name.
> +      </description>
> +      <arg name="conn_name" type="string" summary="connector name" />
> +    </event>

It would be useful to have an optional event that contains
vendor/product id, and serial from EDID, if available.

> +  </interface>
> +
> +  <interface name="zwp_kms_lease_request_v1" version="1">
> +    <description summary="lease request object">
> +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> +      to create a zwp_kms_lease_v1 object (the actual lease object), to revoke
> +      it, and to specify from which connector the lease should be created.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the lease request object">
> +        This destroys the lease request interface, and can be called once
> +        the client obtained a zwp_kms_lease_v1 object.
> +      </description>
> +    </request>
> +
> +  <request name="add_connector">
> +     <description summary="add a leasable connector object">
> +       This request is used to create the lease object using the leasable
> +       connector object, and must be called before zwp_kms_lease_request_v1::create.
> +     </description>
> +     <arg name="conn_obj" type="object" interface="zwp_kms_lease_connector_v1"
> +     summary="a leasable connector added to the lease"/>
> +  </request>
> +
> +  <request name="create">
> +     <description summary="create the lease">
> +       This request will create a zwp_kms_lease_v1 object based on
> +       zwp_kms_lease_connector_v1 that was added using
> +       zwp_kms_lease_request_v1::add_connector.
> +     </description>
> +  </request>
> +
> +  <event name="created">
> +    <description summary="lease created successfully">
> +      This event indicates that the lease has been created. It provides a
> +      zwp_kms_lease_v1 object used for retrieving the file-descriptor
> +      representing the lease.
> +    </description>
> +    <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1"
> +    summary="the lease obj"/>
> +  </event>
> +
> +  <event name="failed">
> +    <description summary="lease could not be created">
> +      This event indicates that the lease could not be created.
> +    </description>
> +  </event>
> +
> +  <event name="connector_add_failed">
> +    <description summary="failed to add connector">
> +      This event indicates that the leasable connector object specified in
> +      zwp_kms_lease_request_v1::add_connector couldn't be added.
> +    </description>
> +  </event>
> +
> +  <event name="connector_added">
> +    <description summary="connector was added successfully">
> +      This event indicates that the leasable connector object specified in
> +      zwp_kms_lease_request_v1::add_connector has been added successfully.
> +    </description>
> +  </event>

Not sure if the "connector_add_failed" and "connector_added" events are
necessary. If something is wrong, the following "create" request can
just return the "failed" event.

regards
Philipp