[PATCH wayland-protocols v4] unstable/drm-lease: DRM lease protocol support
Marius-cristian Vlad
marius-cristian.vlad at nxp.com
Thu Aug 30 11:01:29 UTC 2018
Excuse my MUA, but I have no other means to reply atm.
-----Original Message-----
From: Philipp Zabel <philipp.zabel at gmail.com>
Sent: Thursday, August 30, 2018 10:03 AM
To: Marius-cristian Vlad <marius-cristian.vlad at nxp.com>; wayland-devel at lists.freedesktop.org
Cc: keithp at keithp.com; eucan at de.adit-jv.com; daniel at fooishbar.org; ppaalanen at gmail.com; sardemff7+wayland at sardemff7.net; p.zabel at pengutronix.de
Subject: Re: [PATCH wayland-protocols v4] unstable/drm-lease: DRM lease protocol support
Hi Marius,
Disclaimer: I don't feel very qualified to comment on the protocol design, take this with a grain of salt.
mvlad: The more comments the better I say 😊.
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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgi
> t.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc4171535389d72e9135c
> 9615cecd07b346fd6d7e&data=02%7C01%7Cmarius-cristian.vlad%40nxp.com
> %7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636712093917097791&sdata=5xcj%2BmYAkgmahSPWIQOWgtjLpaOy
> jEzqAVEINvanfrc%3D&reserved=0 [2]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%
> 2Fcommit%2F%3Fh%3Dv4.15-rc9%26id%3D62884cd386b876638720ef88374b31a84ca
> 7ee5f&data=02%7C01%7Cmarius-cristian.vlad%40nxp.com%7Ceba3e65e2d19
> 47d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6367
> 12093917097791&sdata=s%2FUhj6o7JUe7Is1GO3NZUphEwXg1BJeGAvVhVbr9gaU
> %3D&reserved=0
>
> 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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> lab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Ftree%2Fdrm-lease-adverti
> se-each-connnector-object&data=02%7C01%7Cmarius-cristian.vlad%40nx
> p.com%7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C636712093917097791&sdata=G4kPsx742yO4%2FY5swFgVLSk
> AMk5%2Fa9gnd7Z7%2BLSSgKU%3D&reserved=0
>
> 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.
Mvlad: That's what I expected to see, but I did some brief tests here, and it could the our lease kernel version could a little bit older, but when SIGKILL'ing the client, the DRM subsystem doesn't revoke the lease, or calls drm_lease_destroy. I suppose the following happens: when creating the lease the compositor will hold a reference of the leased fd -- well technically the DRM leases part does -- (and ofc I can't close it then because the client won't be able to use it), we pass that thru weston (over the unix socket), the client will also hold a reference for that leased fd, so basically the lease will only be destroyed when that ref counter hits 0. In the implementation I close the leased fd deliberately on the compositor side, when I get the revoke request from the client. There's no issue when the client behaves, but only when the client dies abruptly, and it won't notify the compositor that it is no longer using the lease. I've only seen this once when you brought it up, so I need to test it further to be sure about this.
> 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.
Mvlad: I'm not really sure on this, see my above comment.
> 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.
mvlad: Which means that the client would have to be notified when that happens, so it can shut down cleanly.
Somehow the client (in its rendering part perhaps) has to check if the lease is still valid. Guess we can also
verify errno, so it should not complicate the client that much.
> 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.
Mvlad: Yep.
> 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.
Mvlad: Indeed. I believe we have some methods exposed to verify what current leases are being in use, question
Is when we do this. There are some options, but will the need to see which are the best.
> 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'?
Mvlad: This was taken ad litteram from Keiths' description, but I guess he meant revoke.
> + 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.
Mvlad: Right, have to check that.
> + 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'?
Mvlad: Sure do.
> 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?
Mvlad: This might even simplify client code even further. I'll modify client code to see, but I don't know at the moment.
> +
> + 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.
Mvlad: I believe the compositor will still (hold) a ref on the leased fd. See my initial comment.
> +
> + 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'?
Mvlad: typo 😊
> + 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.
Mvlad: I'll give it a try and see how this works.
> + <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.
Mvlad: This relates also the above comments as well. Need to verify if indeed it works like this. But a follow up question here regarding planes:
We've discussed this quite extensively, and I've concluded that it is much easier to gather the objects ids required to create the lease, directly
In the compositor as it already has that information. I kind of assume that the connector object will also provide the CRTC and implicitly the planes it can drive. Given that, when extending the protocol the connector object will need to inform the client which planes can be leased. Is that correct?
> + </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.
Mvlad: Need to verify that.
> + <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?
Mvlad: Kept it here because of the ::revoke request. If there's no need for revoke request, yes, it would make more
sense to have it zwp_kms_lease_v1 interface.
> + <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