[PATCH v8] unstable/drm-lease: DRM lease protocol support
Pekka Paalanen
ppaalanen at gmail.com
Fri Nov 8 13:19:24 UTC 2019
On Thu, 7 Nov 2019 09:59:01 -0500
Drew DeVault <sir at cmpwn.com> wrote:
> From: Marius Vlad <marius.vlad at collabora.com>
>
> DRM leasing is a feature which allows the DRM master to "lease" a subset
> of its DRM resources to another DRM master via drmModeCreateLease, which
> returns a file descriptor for the new DRM master. We use this protocol
> to negotiate the terms of the lease and transfer this file descriptor to
> clients.
>
> In less DRM-specific terms: this protocol allows Wayland compositors to
> give over their GPU resources (like displays) to a Wayland client to
> exclusively control.
>
> The primary use-case for this is Virtual Reality headsets, which via the
> non-desktop DRM property are generally not used as desktop displays by
> Wayland compositors, and for latency reasons (among others) are most
> useful to games et al if they have direct control over the DRM resources
> associated with it. Basically, these are peripherals which are of no use
> to the compositor and may be of use to a client, but since they are tied
> up in DRM we need to use DRM leasing to get them into client's hands.
>
> Signed-off-by: Marius Vlad <marius.vlad at collabora.com>
> Signed-off-by: Drew DeVault <sir at cmpwn.com>
> ---
Hi Drew,
I re-read all the v7 discussions and collected the thoughts from there.
As we have the resource discovery issue sorted now, we can drill more
to the details.
> v8 changes lease_manager -> lease_device on the grounds that managers
> are singletons and this interface has one global per DRM device.
This is a good idea.
> The logind concern was found to be valid - logind will not let you open
> the DRM node twice. However, for a non-privileged node like the one
> called for here, most compositors in use today will be able to use
> drmGetDeviceNameFromFd2 and open the file themselves. Regardless, the
> need for improvement in logind does not affect the viability of this
> protocol, and I think that v8 is ready to be applied.
Agreed about logind.
> Quick reminder that, for the sake of the corresponding Vulkan extension,
> we'd like to fast track this protocol to stability once we've agreed on
> the unstable version.
>
What does fast-tracking mean? To me it sounds like we're going to bake
in mistakes we just cannot see quite yet. I don't think this can be
declared stable until we have had real-world use in a multi-GPU system,
for instance. The v7 discussion had open questions as well, like must
disconnecting a connector cause the associated lease to be revoked.
Are we not going to use the new wayland-protocols governance processes?
These issues were raised on v7 and have not been addressed yet:
- hotplug handling
- lease from a wrong device
- drmModeGetLease
- deferred sending of fd
- revoke all leases when losing DRM-master
I note all of them below again in more detail.
> Makefile.am | 1 +
> unstable/drm-lease/README | 5 +
> unstable/drm-lease/drm-lease-unstable-v1.xml | 246 +++++++++++++++++++
> 3 files changed, 252 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 345ae6a..d9fff89 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..16f8551
> --- /dev/null
> +++ b/unstable/drm-lease/README
> @@ -0,0 +1,5 @@
> +Linux DRM lease
> +
> +Maintainers:
> +Drew DeVault <sir at cmpwn.com>
> +Marius Vlad <marius-cristian.vlad at nxp.com>
I believe Marius' email address still needs refreshing.
> 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..50a310b
> --- /dev/null
> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> @@ -0,0 +1,246 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="drm_lease_unstable_v1">
> + <copyright>
> + Copyright © 2018 NXP
> + Copyright © 2019 Status Research & Development GmbH.
> +
> + 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_drm_lease_device_v1" version="1">
> + <description summary="lease device">
> + This protocol is used by Wayland compositors which act as Direct
> + Renderering Manager (DRM) masters to lease DRM resources to Wayland
> + clients. Once leased, the compositor will not use the leased resources
> + until the lease is revoked or the client closes the file descriptor. The
> + compositor will advertise one zwp_drm_lease_device_v1 for each DRM node
> + which has resources available for leasing.
> +
> + The lease device is used to advertise connectors which are available for
> + leasing, and by the client to negotiate a lease request.
Nothing in this spec says anything about hotplug handling, yet it has
been said in reviews that clients probably want to handle hotplug
somehow. Therefore something should be said, particularly because the
hidden premise in this specification seemed to be that:
- disconnected connectors are not leasable
- hotplugged connectors trigger an advertisement if otherwise leasable
- hot-unplugged connectors are revoked
If the compositor does anything else, hotplug handling in clients
becomes a lot harder, so this behaviour should be nailed down.
Likewise it should be noted that the compositor losing DRM-master
should revoke all leases and withdraw all connectors. The compositor
should re-advertise all leasable connectors when it regains DRM master.
> +
> + 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>
> +
> + <enum name="error">
> + <entry name="stopped_device" value="0"
> + summary="request sent to a device which has been stopped"/>
Do you actually ever use this error code anywhere in your
implementation? Is it not unnecessary?
> + </enum>
> +
> + <request name="create_lease_request">
> + <description summary="create a lease request object">
> + Creates a lease request object.
> +
> + See the documentation for zwp_drm_lease_request_v1 for details.
> + </description>
> + <arg name="id" type="new_id" interface="zwp_drm_lease_request_v1" />
> + </request>
> +
> + <request name="stop">
> + <description summary="stop sending events">
> + Indicates the client no longer wishes to receive connector events. The
> + compositor may still send connector events until it sends the finish
> + event, however.
> +
> + The client must not send any requests after this one.
"..Doing so will raise the protocol error stopped_device or a wl_display
error."
I say wl_display error, because that is what I think happens if the
compositor has already sent 'finished' and destroyed the object. In
fact, wl_display errors are the only kind of errors possible, if the
compositor handles 'stop' request by sending 'finished' and destroying
the object immediately.
The spec should say if this affects any existing
zwp_drm_lease_connector_v1, zwp_drm_lease_request_v1, or
zwp_drm_lease_v1 objects.
> + </description>
> + </request>
> +
> + <event name="drm_fd">
> + <description summary="open a non-master fd for this DRM node">
> + The compositor will send this event when the zwp_drm_lease_device_v1
> + global is bound. The included fd is a non-master DRM file descriptor
> + opened for this device. The purpose of this event is to give the client
> + the ability to query DRM and discover information which may help them
> + pick the appropriate DRM device or select the appropriate connectors
> + therein.
What if the compositor cannot get a harmless DRM fd when a client binds
to zwp_drm_lease_device_v1? Maybe the compositor can get one later?
Should clients expect to wait more than a simple roundtrip? Something
should be specified about this.
E.g. when the compositor is inactive (not DRM master itself, or
VT-switched away).
I think it would be worth to record some implementation instructions
here as well. Implementation details are DRM-specific, but so is this
whole extension.
The compositor must not simply dup() its own DRM fd, it must get a
different fd from an independent open() on the device node. The
compositor must check that the resulting fd is not master. The
compositor *can* share the resulting DRM fd with multiple clients. The
compositor must not authenticate the DRM fd.
Any failure to the above would be a potential security hole. Also
there is no DRM documentation we could refer to to point all this out,
and this stuff really needs to be written down somewhere.
> + </description>
> + <arg name="fd" type="fd" summary="DRM file descriptor" />
> + </event>
> +
> + <event name="connector">
> + <description summary="advertise connectors available for leases">
> + The compositor may choose to advertise 0 or more connectors which may be
> + leased to clients, and will use this event to do so. This object may be
> + passed into a lease request to lease that connector. See
> + zwp_drm_lease_request_v1.add_connector for details.
> +
> + When this global is bound, the compositor will send all connectors
> + available for lease, but may send additional connectors at any time.
> + </description>
> + <arg name="id" type="new_id" interface="" />
> + </event>
> +
> + <event name="finished">
> + <description summary="the compositor has finished using the device">
> + This event indicates that the compositor is done sending connector
> + events. The compositor will destroy this object immediately after
> + sending this event, and it will become invalid. The client should
> + release any resources associated with this device after receiving this
> + event.
> + </description>
> + </event>
> + </interface>
> +
> + <interface name="zwp_drm_lease_connector_v1" version="1">
> + <description summary="a leasable DRM connector">
> + Represents a DRM connector which is available for lease. These objects are
> + created via zwp_drm_lease_device_v1.connector, and should be passed into
> + lease requests via zwp_drm_lease_request_v1.add_connector.
> + </description>
> +
> + <event name="name">
> + <description summary="name">
> + The compositor sends this event once the connector is created to
> + indicate the name of this connector. This will not change for the
> + duration of the Wayland session, but is not guaranteed to be consistent
> + between sessions.
> +
> + If the compositor also supports zxdg_output_manager_v1 and this
> + connector corresponds to a zxdg_output_v1, this name will match the
> + name of this zxdg_output_v1 object.
> + </description>
> + <arg name="name" type="string" summary="connector name" />
> + </event>
> +
> + <event name="description">
> + <description summary="description">
> + The compositor sends this event once the connector is created to provide
> + a human-readable description for this connector, which may be presented
> + to the user.
> + </description>
> + <arg name="description" type="string" summary="connector description" />
> + </event>
> +
> + <event name="connector_id">
> + <description summary="connector_id">
> + The compositor will send this event to indicate the DRM ID which
> + represents the underlying connector which is being offered. Note that
> + the final lease may include additional object IDs, such as CRTCs and
> + planes.
> + </description>
> + <arg name="connector_id" type="int" summary="DRM Connector ID" />
Type should be uint instead.
> + </event>
> +
> + <event name="withdrawn">
> + <description summary="lease offer withdrawn">
> + Sent to indicate that the compositor will no longer honor requests for
> + DRM leases which include this connector. The client may still issue a
> + lease request including this connector, but the compositor will send
> + zwp_drm_lease_v1.finished without issuing a lease fd.
> + </description>
> + </event>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy connector">
> + The client may send this request to indicate that it will not issue a
> + lease request for this connector. Clients are encouraged to send this
> + after receiving the "withdrawn" request so that the server can release
> + the resources associated with this connector offer.
What if this zwp_drm_lease_connector_v1 has been used for a
zwp_drm_lease_request_v1 object which has not been destroyed or
submitted yet?
Does the connector remain in the lease object or is it removed from the
lease object?
Does this affect any leases that have been submitted already?
These questions should be answered by the spec.
> + </description>
> + </request>
> + </interface>
> +
> + <interface name="zwp_drm_lease_request_v1" version="1">
> + <description summary="DRM lease request">
> + A client that wishes to lease DRM resources will attach the list of
> + connectors advertised with zwp_drm_lease_device_v1.connector that they
> + wish to lease, then use zwp_drm_lease_request_v1.submit to submit the
> + request.
> + </description>
> +
> + <enum name="error">
> + <entry name="submitted_lease" value="0"
> + summary="attempted to reuse a submitted lease"/>
> + </enum>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroys the lease request object">
> + Indicates that the client will no longer use this lease request.
> + </description>
> + </request>
> +
> + <request name="request_connector">
> + <description summary="request a connector for this lease">
> + Indicates that the client would like to lease the given connector.
> + This is only used as a suggestion, the compositor may choose to
> + include any resources in the lease it issues, or change the set of
> + leased resources at any time.
Ok, but surely the compositor should at least keep the exact connector
that was asked for?
The "change" is a bit odd in other ways too. How about this instead:
Indicates that the client would like to lease the given connector.
The compositor will automatically add at least the minimal
other resources (CRTC, primary plane) needed to light up the
connector.
> + </description>
> + <arg name="connector" type="object"
> + interface="zwp_drm_lease_connector_v1" />
Should it not be an error to use a zwp_drm_lease_connector_v1 from a
different zwp_drm_lease_device_v1 than where the
zwp_drm_lease_request_v1 was created?
What if a client adds the same connector twice?
Either the spec needs to define error codes for these cases, or say
what happens then.
> + </request>
> +
> + <request name="submit">
> + <description summary="submit the lease request">
> + Submits the lease request and creates a new zwp_drm_lease_v1 object.
> + After calling submit, issuing any other request than destroy is a
> + protocol error.
Please name the protocol error.
> + </description>
> + <arg name="id" type="new_id" interface="zwp_drm_lease_v1" />
> + </request>
> + </interface>
> +
> + <interface name="zwp_drm_lease_v1" version="1">
> + <description summary="a DRM lease">
> + A DRM lease object is used to transfer the DRM file descriptor to the
> + client and manage the lifetime of the lease.
> + </description>
> +
> + <event name="lease_fd">
> + <description summary="shares the DRM file descriptor">
> + This event returns a file descriptor suitable for use with DRM-related
> + ioctls. The client should use drmModeGetLease to enumerate the DRM
> + objects which have been leased to them. If the compositor cannot or
> + will not grant a lease for the requested connectors, it will not send
> + this event, instead sending the finished event immediately.
Isn't this about drmModeGetLease still wrong?
Is it enough to do a roundtrip after submit to ensure this event is
received, or should clients be prepared to wait for longer? Maybe the
compositor is not a DRM master at this time, what should it do then?
> +
> + It is a protocol error for the compositor to send this event more than
> + once for a given lease.
Would be sufficient to just say "the compositor sends this event at
most once during the object lifetime". No use making it a protocol
error, the client cannot send an error to the compositor.
> + </description>
> + <arg name="leased_fd" type="fd" summary="leased DRM file descriptor" />
> + </event>
> +
> + <event name="finished">
> + <description summary="sent when the lease has been revoked">
> + When the compositor revokes the lease, it will issue this event to
> + notify clients of the change. If the client requires a new lease, they
> + should destroy this object and submit a new lease request. The
> + compositor will send no further events for this object after sending
> + the finish event.
> + </description>
> + </event>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroys the lease object">
> + The client should send this to indicate that it no longer wishes to use
> + this lease. The compositor should use drmModeRevokeLease on the
> + appropriate file descriptor, if necessary, then release this object.
Strike "then release this object", it does not add value as the request
is already defined as a destructor.
> + </description>
> + </request>
> + </interface>
> +</protocol>
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20191108/1da1f071/attachment-0001.sig>
More information about the wayland-devel
mailing list