[PATCH wayland-protocols v4] unstable/drm-lease: DRM lease protocol support
Philipp Zabel
philipp.zabel at gmail.com
Thu Aug 30 07:03:06 UTC 2018
Hi Marius,
Disclaimer: I don't feel very qualified to comment on the protocol
design, take this with a grain of salt.
Am Mittwoch, den 29.08.2018, 14:00 +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 v3:
> - instead of advertising all leasable connectors at once, advertise each
> connector for each leasable output. Use an object to represent a
> leasable connector and provide a request to add that leasable connector
> when creating the lease (Philipp Zabel)
> - add two events to handle add_connector request
> - add some comments in the manager interface to explain how the protocol
> can/should be used
>
> Changes since v2:
> - advertise connectors when creating a lease request object (Pekka Paalanen)
> - when revoking the lease use the lease object instead of lessee id (Pekka Paalanen)
> - various grammar/spelling fixes (Pekka Paalanen)
>
> Changes since v1:
> - added manager: advertise lease capability and manage the lease (Daniel Stone)
> - add request(s) for adding connector/crtc/plane to behave more like dmabuf (Daniel Stone)
>
> Signed-off-by: Marius Vlad <marius-cristian.vlad at nxp.com>
> ---
> A (rather incomplete) implementation for this version can be found at
> https://gitlab.freedesktop.org/marius.vlad0/weston/tree/drm-lease-advertise-each-connnector-object
>
> One interesting question is how to handle the situation when the client
> deliberately, or not, holds the lease indefinitely.
The client implicitly cancels the lease by closing the lease fd (or by
having it closed automatically when it is terminated). The compositor
will get a hotplug event then, and it has to check the lessee list
happens and revoke leases to lost lessees.
> This has multiple
> ramification like what happens when the client un-expectingly dies and doesn't
> revoking the lease,
See above, a lease terminates when its last fd is closed.
> or when hot-plugging the connector and weston tries to get
> a hold of a connector previously leased?
I'd say hot-unplugging a connector that is part of a lease should cause
this lease to be canceled.
> It could be there might be other
> situations where the compositor will need to revoke the lease.
The VT switch case, for example. If the compositor has to give up
drm_master temporarily, it will have to revoke all leases first.
> In the implementation we could deny the compositor to get a hold of the
> connector when hot-plugging that output, if that's desirable, and presumably
> establish a way to detect periodically if the lease is still in use.
>
> Alternatively, shouldn't we use something like a ping/pong approach in which
> the client (in rendering part), sends periodically an alive signal?
I don't think the compositor should care what the lessee is doing with
its lease, only whether it is still holding the fd.
> Makefile.am | 1 +
> unstable/drm-lease/README | 4 +
> unstable/drm-lease/drm-lease-unstable-v1.xml | 244 +++++++++++++++++++++++++++
> 3 files changed, 249 insertions(+)
> create mode 100644 unstable/drm-lease/README
> create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml
>
> diff --git a/Makefile.am b/Makefile.am
> index 6394e26..5d13dca 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -4,6 +4,7 @@ unstable_protocols = \
> unstable/pointer-gestures/pointer-gestures-unstable-v1.xml \
> unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml \
> unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml \
> + unstable/drm-lease/drm-lease-unstable-v1.xml \
> unstable/text-input/text-input-unstable-v1.xml \
> unstable/text-input/text-input-unstable-v3.xml \
> unstable/input-method/input-method-unstable-v1.xml \
> diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
> new file mode 100644
> index 0000000..a25600c
> --- /dev/null
> +++ b/unstable/drm-lease/README
> @@ -0,0 +1,4 @@
> +Linux DRM lease
> +
> +Maintainers:
> +Marius Vlad <marius-cristian.vlad at nxp.com>
> diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml b/unstable/drm-lease/drm-lease-unstable-v1.xml
> new file mode 100644
> index 0000000..40eedb4
> --- /dev/null
> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> @@ -0,0 +1,244 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="drm_lease_unstable_v1">
> +
> +<copyright>
> + Copyright 2018 NXP
> +
> + Permission is hereby granted, free of charge, to any person obtaining a
> + copy of this software and associated documentation files (the "Software"),
> + to deal in the Software without restriction, including without limitation
> + the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + and/or sell copies of the Software, and to permit persons to whom the
> + Software is furnished to do so, subject to the following conditions:
> +
> + The above copyright notice and this permission notice (including the next
> + paragraph) shall be included in all copies or substantial portions of the
> + Software.
> +
> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + DEALINGS IN THE SOFTWARE.
> +</copyright>
> +
> + <interface name="zwp_kms_lease_manager_v1" version="1">
> + <description summary="lease manager">
> + This interface makes use of DRM lease written by Keith Packard.
> +
> + A DRM master can create another DRM master and ``lease'' resources it has
> + control over to the new DRM master. Once leased, resources can not be
> + controlled by the owner unless the owner cancels the lease, or the new
Should this say 'unless the owner revokes the lease'?
> + DRM master is closed.
> +
> + A lease is a contract between the Lessor (DRM master which has leased out
> + resources to one or more other DRM masters) and a Lessee
> + (DRM master which controls resources leased from another DRM master). This
> + contract specifies which resources may be controlled by the Lessee.
> +
> + The Lessee can issue modesetting/page-flipping atomic operations etc.,
> + just like a Lessor using the leased file-descriptor passed by the Lessor.
> +
> + Besides the leased file-descriptor, an integer is used to uniquely
> + identify a Lessee within the tree of DRM masters descending from a single
> + Owner. Once the Lessee has finished with the resources it had used, the
> + Lessee ID can be used to revoke that lease.
'Tree of DRM masters' sounds like subleasing is still supported.
I think that got removed.
> + Upon connecting to the compositor all leasable connectors will be
> + advertised to the client. These connectors are represented by
> + zwp_kms_lease_connector_v1 interface, and have to be "added" before
> + creating a lease object.
'and have to be "added" to a lease request'?
> 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. zwp_kms_lease_connector_v1 provides requests
> + and events to retrieve additional information specific to that connector
> + object.
Is it required/preferred to hide the common connector properties behind
requests, or should the connector object send these events
unsolicitedly?
> +
> + 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,
> + the ::revoke request will require the previous obtained lease object to be
> + used when revoking the lease.
Closing the fd should terminate the lease as well, causing the
compositor to revoke it.
> +
> + 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">
> + This has no effect on any remaining objects created through this
> + interface.
> + </description>
> + </request>
> +
> + <request name="create_lease_req">
> + <description summary="create a temporary object for managing the lease">
> + 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">
> + 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 leasable (based on compositor policy), the
'can be leasable'?
> + client can request additional information using
> + zwp_kms_lease_connector_v1 interface 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 has no effect on any remaining objects created through this
> + interface.
> + </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>
> +
> + <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.
> + </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. The client can distinguish between
> + different leasable connectors by requesting additional information for
> + that connector.
> + </description>
> +
> + <request name="retrieve_name">
> + <description summary="name">
> + Request to retrieve connector output name on which the leasable
> + connector object is based on.
> + </description>
> + </request>
Is this necessary? It seems to me that the name event could just be
sent without request to new listeners.
> + <event name="name">
> + <description summary="name">
> + Event sent when requesting connector output name.
> + </description>
> + <arg name="conn_name" type="string" summary="connector name" />
> + </event>
> + </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.
I'm not sure about the revoking part, actually.
I expected this to handle more like zwp_linux_buffer_params_v1, as a
single-use object to create a zwp_kms_lease_v1 once (after adding
connectors and, possibly later, planes). After successful lease
creation (or failure) this could be destroyed.
Since the zwp_kms_lease_v1 itself has a destructor, and the actual
lease can be terminated by closing the fd anyway, I see no reason to
keep the original zwp_kms_lease_request_v1 around.
> + </description>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroys the lease request object">
> + This has no effect on any remaining objects created through this
> + interface.
> + </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>
> +
> + <request name="revoke">
> + <description summary="revoke">
> + Revoke the lease, using the zwp_kms_lease_v1 objected received in
> + zwp_kms_lease_request_v1::created event.
> + </description>
> + <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1"
> + summary="lease object to remove" />
> + </request>
See above, I think this is something the client shouldn't have to do.
> + <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/revoked.
> + </description>
> + </event>
> +
> + <event name="revoked">
> + <description summary="lease revoked successfully">
> + This event indicates that the lease has been revoked.
> + </description>
> + </event>
Should this be moved to the zwp_kms_lease_v1?
> + <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>
> +
> + </interface>
> +</protocol>
regards
Philipp
More information about the wayland-devel
mailing list