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

Post by Marius Vlad
+ <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.

Post by Marius Vlad
+
+ - 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.
+
+
+ - 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.

Post by Marius Vlad
+ <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.

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.

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

Cheers,
Daniel

Marius-cristian Vlad

2018-01-25 10:17:44 UTC

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:***@fooishbar.org]
Sent: Wednesday, January 24, 2018 11:42 PM
To: Marius-cristian Vlad <marius-***@nxp.com>
Cc: wayland-***@lists.freedesktop.org; Pekka Paalanen <***@collabora.co.uk>; Keith Packard <***@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.

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.

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.

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

Daniel Stone

2018-01-25 10:41:40 UTC

Hi Marius,

On 25 January 2018 at 10:17, Marius-cristian Vlad

Post by Marius-cristian Vlad
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
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.

Right. What I meant is that we would build the list of objects
incrementally, rather than requiring them to be passed all at once,
e.g.:
req = zwp _kms_lease_manager_v1_get_request(mgr);
zwp_kms_lease_request_v1_add_output(req, output_hdmi1); /* adds CRTC +
connector + primary plane */
zwp_kms_lease_request_v1_add_plane(req); /* adds overlay plane */
lease = zwp_kms_lease_request_v1_create(req); /* only here is
drmModeCreateLease called */
zwp_kms_lease_request_v1_destroy(lease);

This would give a little bit more flexibility and avoids hardcoding
specific setups. It also makes it very clear what the life cycle is
for each stage.

Post by Marius-cristian Vlad
[mvlad] 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.

I think you mean 'scanout plane' here: 'universal planes' refers to
being able to treat scanout/cursor planes like overlay planes (i.e.
enumerate them through GetResources, and configure them outside of
SetCrtc/SetCursor/MoveCursor). I just checked now, and any kind of
plane can be leased.

There is one issue I spotted though: it's theoretically possible for
Weston to not enable universal planes but enable leasing; this will
result in us passing a plane ID of 0 (from our fake scanout plane) in
the lease request. Probably best to just not enable leasing if
universal planes isn't enabled; you'd have to have a very weird
contorted partial backport of libdrm/kernel to do that, but people
will absolutely try to do that, I'm sure. :)

Cheers,
Daniel

Marius Vlad

2018-02-12 14:51:51 UTC

--
2.9.3

Marius-cristian Vlad

2018-03-28 11:21:58 UTC

Ping? Daniel?

Have submitted a v4 for the server/client example. Would be nice to
know if the protocol is at least OK or needs more work.

Post by 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=c4171535389d72e9
135c9615cecd07b346fd6d7e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.gi
t/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f
- 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)
---
Makefile.am                                  |   1 +
unstable/drm-lease/README                    |   4 +
unstable/drm-lease/drm-lease-unstable-v1.xml | 150
+++++++++++++++++++++++++++
3 files changed, 155 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 4b9a901..4f6a874 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,6 +2,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/input-method/input-method-unstable-v1.xml
\
unstable/xdg-shell/xdg-shell-unstable-v5.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
+
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..907efb0
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,150 @@
+<?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
+
+    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
+      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 Lesor using the leased file-descriptor passed by
the Lesor.
+
+      Besides the leased file-description, 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.
+
+      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 lease manager object.
+      </description>
+    </request>
+
+    <request name="create_lease_req">
+      <description summary="create a temporary object for managing
the lease">
+      Create a lease request object to manage the lease. All further
interaction
+      is achived using this object. Returns a
zwp_kms_lease_request_v1.
+      </description>
+      <arg name="params_id" type="new_id"
interface="zwp_kms_lease_request_v1"
+    summary="the new temporary"/>
+    </request>
+
+  </interface>
+
+  <interface name="zwp_kms_lease_request_v1" version="1">
+  <description summary="lease request object">
+      Typical usage is to create this request lease object using the
+      'zwp_kms_lease_manager_v1', then call 'add_*' requests to add
a connector,
+      crtc (and plane) id.
+
+      At the end, use 'create' request to actually create the lease.
The client
+      is responsible for finding a suitable combination of
connector/crtc/plane
+      to pass. This can be achived by going over all connected
connectors and
+      and determine if a lease can be created.
+  </description>
+
+    <request name="add_connector">
+      <description summary="connector id">
+      Request to add a connector id to current lease request object.
+      </description>
+      <arg name="id" type="uint" summary="connector id"/>
+    </request>
+
+    <request name="add_crtc">
+      <description summary="crtc id">
+      Request to add a CRTC id to current lease request object.
+      </description>
+      <arg name="id" type="uint" summary="crtc id"/>
+    </request>
+
+    <request name="add_plane">
+      <description summary="plane id">
+      Request to add a plane id to current lease request object.
+      </description>
+      <arg name="id" type="uint" summary="plane id"/>
+    </request>
+
+    <request name="create">
+      <description summary="create the lease">
+      This request is to be called last to get the lease. Either a
'created'
+      event in case of success, or 'failed' event in case of failure
is
+      generated.
+      </description>
+    </request>
+
+    <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="created">
+    <description summary="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>
+
+    <event name="revoked">
+    <description summary="lease revoked">
+ This event indicates that the lease has been revoked.
+    </description>
+    </event>
+
+  </interface>
+
+</protocol>

Pekka Paalanen

2018-05-29 14:10:02 UTC

On Mon, 12 Feb 2018 16:51:51 +0200

Hi Marius,

it's great to have someone working on this!

I finally got a chance to look at it. Comments are inline as usual. Most
of my questions call for an answer in the spec text.

Post by Marius Vlad
diff --git a/Makefile.am b/Makefile.am
index 4b9a901..4f6a874 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,6 +2,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/input-method/input-method-unstable-v1.xml \
unstable/xdg-shell/xdg-shell-unstable-v5.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
+
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..907efb0
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,150 @@
+<?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
+
+ 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
+ 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 Lesor using the leased file-descriptor passed by the Lesor.

s/Lesor/Lessor/ twice.

Post by Marius Vlad
+
+ Besides the leased file-description, an integer is used to uniquely

file-descriptor

Post by Marius Vlad
+ 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.

I think we should use a Wayland protocol object to represent a tentative
Lessee ID so it will never need to be communicated. The only thing a
client does with it is pass it back to the compositor. What if a client
maliciously or by accident passes someone else's lessee ID? Using a
protocol object instead will make such mistake impossible.

+ This has no effect on any remaining objects created through this
interface.

Post by Marius Vlad
+ </description>
+ </request>
+
+ <request name="create_lease_req">
+ <description summary="create a temporary object for managing the lease">
+ Create a lease request object to manage the lease. All further interaction
+ is achived using this object. Returns a zwp_kms_lease_request_v1.

Create an object for temporarily storing all the KMS resources to be leased.

Post by Marius Vlad
+ </description>
+ <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"
+ summary="the new temporary"/>
+ </request>
+
+ </interface>
+
+ <interface name="zwp_kms_lease_request_v1" version="1">
+ <description summary="lease request object">
+ Typical usage is to create this request lease object using the

a lease request object

Post by Marius Vlad
+ 'zwp_kms_lease_manager_v1', then call 'add_*' requests to add a connector,
+ crtc (and plane) id.
+
+ At the end, use 'create' request to actually create the lease. The client
+ is responsible for finding a suitable combination of connector/crtc/plane
+ to pass. This can be achived by going over all connected connectors and
+ and determine if a lease can be created.

To me this sounds like a client needs to first open a DRM card node,
iterate through all the resources (is that even possible if it is not a
DRM master?), look at the information and guess which ones the
compositor might be willing to lease out, and then through trial and
error maybe find something that will actually be accepted.

I do not like this plan.

It assumes that the client can open a DRM card node itself - this is
often not the case. Even display servers do not open the DRM card node
themselves, they ask logind, so it is actually likely that the client
does not have the file system access to the card node.

Another problem is that the client has no idea which resources the
compositor is willing to lease out. That set of resources could even
change at runtime when the compositor output configuration changes, if
the compositor has a policy that everything it is not using itself can
be leased (I would propose Weston to have this policy).

A compositor could even be willing to lease out resources it is using,
e.g. plane resources it can fall back from, or a CRTC for an output
that will get temporarily disabled if a Lessee wants it. Especially in
the latter case, the compositor might ask the user if he is willing to
give the output to an app and explaining how he can get his output back
in case the app malfunctions (e.g. compositor hotkey to revoke all
leases).

Both problems could be solved by extending the kms_lease_manager
interface to advertise the leasable KMS resources, including enough
information to allow the client to make a good guess on e.g. which
connectors it might be interested in.

Another fun problem is how to pick a CRTC, since CRTCs cannot be
assumed to be equal. It's quite possible that the client needs to lease
every available CRTC one at a time, attempt modeset, and see if it can
drive the connector with the mode it wants. If it doesn't work, try the
next available CRTC. Since we probably cannot avoid trial and error
completely, it is important to reduce the number of possible
combinations as much as possible.

Another approach would be to not let the client pick a CRTC on its own.
Let the client pick a connector from a list, and have the compositor
find a suitable CRTC to go with it.

Post by Marius Vlad
+ </description>
+
+ <request name="add_connector">
+ <description summary="connector id">
+ Request to add a connector id to current lease request object.
+ </description>
+ <arg name="id" type="uint" summary="connector id"/>
+ </request>
+
+ <request name="add_crtc">
+ <description summary="crtc id">
+ Request to add a CRTC id to current lease request object.
+ </description>
+ <arg name="id" type="uint" summary="crtc id"/>
+ </request>
+
+ <request name="add_plane">
+ <description summary="plane id">
+ Request to add a plane id to current lease request object.
+ </description>
+ <arg name="id" type="uint" summary="plane id"/>
+ </request>

What happens if any of the ids is invalid?
I suppose the compositor just fails the lease.

Should we not forbid using zero though, because that will always be
invalid? In which case we need an error code for an invalid resource id
protocol error (enum name="error" in the interface).

Adding the same resource twice should be caught too.

Post by Marius Vlad
+
+ <request name="create">
+ <description summary="create the lease">
+ This request is to be called last to get the lease. Either a 'created'
+ event in case of success, or 'failed' event in case of failure is
+ generated.
+ </description>

As Daniel suggested before, this request should create a new protocol
object that represents the actual lease. All requests and events below
would be part of that object's interface instead of kms_lease_request's.

Let me call the new protocol object interface kms_lease, in lack of a
better name. The lessee ID will not be needed in the protocol at all,
it will be represented by the kms_lease protocol object. (Wayland
objects are essentially typesafe per-client IDs.)

Post by Marius Vlad
+ </request>
+
+ <request name="revoke">
+ <description summary="revoke a lease">
+ This asks to revoke a lease using the lessee id previously given in event
+ created.

What happens when a client asks for a revoke?

Will it result in a revoked event?

What are the requirements a client must have done with the leased DRM
resources before it sends this request?

What if a client simply closes the DRM fd it got with the lease and
destroys this protocol object? Or does it in the opposite order,
destroys first and closes then?

Post by Marius Vlad
+ </description>
+ <arg name="id" type="uint" summary="lessee id"/>
+ </request>
+
+
+ <event name="created">
+ <description summary="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.

How should the compositor have programmed the leased DRM resources
before sending this event?

My question is related to flicker avoidance (avoid having the CRTC go
through a temporary off-state if it was on already), and information
leaks (the FB left on the CRTC could be queried and read back by the
Lessee.)

Should we instruct the compositor to program any CRTCs with FBs that do
not contain any sensitive information? How can the compositor ensure
those FBs get destroyed after the client has programmed its own FBs?

Post by Marius Vlad
+ </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.

+ The client should destroy this object, and possibly try again with a
different set of DRM resources.

Post by Marius Vlad
+ </description>
+ </event>
+
+ <event name="revoked">
+ <description summary="lease revoked">
+ This event indicates that the lease has been revoked.

Does the compositor send this event before or after it has called the
revoke ioctl?

I would assume after, which means that the client possibly starts
hitting KMS errors before it gets this event. That could be worth to
note here.

On revoke, what happens to the FB on a leased CRTC? Can a compositor
query and read it back? In this direction the information leak is
probably ok.

Post by Marius Vlad
+ </description>
+ </event>

This interface is missing a destroy request. Interfaces must always
have a destroy request unless there is a very good reason to not have
one. In any case, every object must be destroyable somehow.

Post by Marius Vlad
+
+ </interface>
+
+</protocol>

Thanks,
pq

Marius-cristian Vlad

2018-05-30 12:38:22 UTC

Post by Pekka Paalanen
On Mon, 12 Feb 2018 16:51:51 +0200

Hi Marius,
it's great to have someone working on this!
I finally got a chance to look at it. Comments are inline as usual. Most
of my questions call for an answer in the spec text.

Thanks for taking the time to go over this. I have some minor follow-up
questions bellow.

Post by Marius Vlad
diff --git a/Makefile.am b/Makefile.am
index 4b9a901..4f6a874 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,6 +2,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/input-method/input-method-unstable-v1.xml
\
unstable/xdg-shell/xdg-shell-unstable-v5.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
+
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..907efb0
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,150 @@
+<?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
+
+    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
+      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 Lesor using the leased file-descriptor passed by
the Lesor.

s/Lesor/Lessor/ twice.

Post by Marius Vlad
+
+ Besides the leased file-description, an integer is used to
uniquely

file-descriptor

Post by Marius Vlad
+      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.

I think we should use a Wayland protocol object to represent a
tentative
Lessee ID so it will never need to be communicated. The only thing a
client does with it is pass it back to the compositor. What if a client
maliciously or by accident passes someone else's lessee ID? Using a
protocol object instead will make such mistake impossible.

Alright, this sounds quite fine.

+ This has no effect on any remaining objects created through this
interface.

Post by Marius Vlad
+      </description>
+    </request>
+
+    <request name="create_lease_req">
+      <description summary="create a temporary object for managing
the lease">
+      Create a lease request object to manage the lease. All
further interaction
+      is achived using this object. Returns a
zwp_kms_lease_request_v1.

Create an object for temporarily storing all the KMS resources to be leased.

Post by Marius Vlad
+      </description>
+      <arg name="params_id" type="new_id"
interface="zwp_kms_lease_request_v1"
+    summary="the new temporary"/>
+    </request>
+
+  </interface>
+
+  <interface name="zwp_kms_lease_request_v1" version="1">
+  <description summary="lease request object">
+      Typical usage is to create this request lease object using
the

a lease request object

Post by Marius Vlad
+      'zwp_kms_lease_manager_v1', then call 'add_*' requests to
add a connector,
+      crtc (and plane) id.
+
+      At the end, use 'create' request to actually create the
lease. The client
+      is responsible for finding a suitable combination of
connector/crtc/plane
+      to pass. This can be achived by going over all connected
connectors and
+      and determine if a lease can be created.

Indeed, that's how the client demo/example does it now.

Post by Pekka Paalanen
I do not like this plan.
It assumes that the client can open a DRM card node itself - this is
often not the case. Even display servers do not open the DRM card node
themselves, they ask logind, so it is actually likely that the client
does not have the file system access to the card node.

Well... I assumed that the client can do this on its own.

Post by Pekka Paalanen
Another problem is that the client has no idea which resources the
compositor is willing to lease out. That set of resources could even
change at runtime when the compositor output configuration changes, if
the compositor has a policy that everything it is not using itself can
be leased (I would propose Weston to have this policy).

Right.

Post by Pekka Paalanen
A compositor could even be willing to lease out resources it is using,
e.g. plane resources it can fall back from, or a CRTC for an output
that will get temporarily disabled if a Lessee wants it. Especially in
the latter case, the compositor might ask the user if he is willing to
give the output to an app and explaining how he can get his output back
in case the app malfunctions (e.g. compositor hotkey to revoke all
leases).

Yes.

Post by Pekka Paalanen
Both problems could be solved by extending the kms_lease_manager
interface to advertise the leasable KMS resources, including enough
information to allow the client to make a good guess on e.g. which
connectors it might be interested in.
Another fun problem is how to pick a CRTC, since CRTCs cannot be
assumed to be equal. It's quite possible that the client needs to lease
every available CRTC one at a time, attempt modeset, and see if it can
drive the connector with the mode it wants. If it doesn't work, try the
next available CRTC. Since we probably cannot avoid trial and error
completely, it is important to reduce the number of possible
combinations as much as possible.
Another approach would be to not let the client pick a CRTC on its own.
Let the client pick a connector from a list, and have the compositor
find a suitable CRTC to go with it.

Alright, if I got this right: the compositor will advertise which
connectors can be leased.
Will present that to the client, client will pick one of those. On the
reply the compositor will determine a valid CRTC to drive that
connector then will create the lease and send back to the client the
lease fd.

How will the compositor choose which connectors can be leased? We
assume that all connectors can be leased?

For a client a unsigned int doesn't really tell anything .... is this
the panel on my right, the display on my left or the upper panel -- or
the VR is just hooked up? Do we advertise this to the client as well in
the form of output names or something equivalent? I guess more general
question show we present the potential leasable resource(s) in a human
form? Would that make sense?

The approach with the client choosing which crtc/connector/plane
somehow assumes that client has a-priory knowledge of which resource
wants... albeit will all the drawbacks you enumerated before.

Post by Marius Vlad
+  </description>
+
+    <request name="add_connector">
+      <description summary="connector id">
+      Request to add a connector id to current lease request
object.
+      </description>
+      <arg name="id" type="uint" summary="connector id"/>
+    </request>
+
+    <request name="add_crtc">
+      <description summary="crtc id">
+      Request to add a CRTC id to current lease request object.
+      </description>
+      <arg name="id" type="uint" summary="crtc id"/>
+    </request>
+
+    <request name="add_plane">
+      <description summary="plane id">
+      Request to add a plane id to current lease request object.
+      </description>
+      <arg name="id" type="uint" summary="plane id"/>
+    </request>

What happens if any of the ids is invalid?
I suppose the compositor just fails the lease.
Should we not forbid using zero though, because that will always be
invalid? In which case we need an error code for an invalid resource id
protocol error (enum name="error" in the interface).
Adding the same resource twice should be caught too.

Post by Marius Vlad
+
+    <request name="create">
+      <description summary="create the lease">
+      This request is to be called last to get the lease. Either a
'created'
+      event in case of success, or 'failed' event in case of
failure is
+      generated.
+      </description>

As Daniel suggested before, this request should create a new protocol
object that represents the actual lease. All requests and events below
would be part of that object's interface instead of
kms_lease_request's.
Let me call the new protocol object interface kms_lease, in lack of a
better name. The lessee ID will not be needed in the protocol at all,
it will be represented by the kms_lease protocol object. (Wayland
objects are essentially typesafe per-client IDs.)

Post by Marius Vlad
+    </request>
+
+    <request name="revoke">
+      <description summary="revoke a lease">
+       This asks to revoke a lease using the lessee id previously
given in event
+       created.

What happens when a client asks for a revoke?
Will it result in a revoked event?
What are the requirements a client must have done with the leased DRM
resources before it sends this request?
What if a client simply closes the DRM fd it got with the lease and
destroys this protocol object? Or does it in the opposite order,
destroys first and closes then?

Post by Marius Vlad
+      </description>
+      <arg name="id" type="uint" summary="lessee id"/>
+    </request>
+
+
+    <event name="created">
+    <description summary="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.

How should the compositor have programmed the leased DRM resources
before sending this event?
My question is related to flicker avoidance (avoid having the CRTC go
through a temporary off-state if it was on already), and information
leaks (the FB left on the CRTC could be queried and read back by the
Lessee.)
Should we instruct the compositor to program any CRTCs with FBs that do
not contain any sensitive information? How can the compositor ensure
those FBs get destroyed after the client has programmed its own FBs?

Post by Marius Vlad
+    </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.

+ The client should destroy this object, and possibly try again with a
different set of DRM resources.

Post by Marius Vlad
+    </description>
+    </event>
+
+    <event name="revoked">
+    <description summary="lease revoked">
+ This event indicates that the lease has been revoked.

Does the compositor send this event before or after it has called the
revoke ioctl?
I would assume after, which means that the client possibly starts
hitting KMS errors before it gets this event. That could be worth to
note here.
On revoke, what happens to the FB on a leased CRTC? Can a compositor
query and read it back? In this direction the information leak is
probably ok.

Post by Marius Vlad
+ </description>
+ </event>

This interface is missing a destroy request. Interfaces must always
have a destroy request unless there is a very good reason to not have
one. In any case, every object must be destroyable somehow.

Post by Marius Vlad
+
+ </interface>
+
+</protocol>

Thanks,
pq

Pekka Paalanen

2018-05-31 12:38:35 UTC

On Wed, 30 May 2018 12:38:22 +0000

Post by Pekka Paalanen
On Mon, 12 Feb 2018 16:51:51 +0200

Hi Marius,
it's great to have someone working on this!
I finally got a chance to look at it. Comments are inline as usual. Most
of my questions call for an answer in the spec text.

Thanks for taking the time to go over this. I have some minor follow-up
questions bellow.

Hi Marius,

I have written some pondering as replies to your questions below. I hope
they make sense.

I had a chat with Keith Packard in IRC about some details of how DRM
leasing actually works. These details affect the implementations more,
but there might be something we need to take into account with the
protocol as well.

These are my notes from the chat:

- When there is a change with DRM leases, a normal DRM hotplug event
will be emitted.

- Particularly Lessor needs to listen for hotplug events and re-query
the state of all DRM leases. This is how Lessor finds out a lease has
been terminated by a Lessee. This needs to happen also on gaining DRM
master.

- VT-switching: When the original DRM master (Lessor) drops the master
status, all DRM leases it has given out will be temporarily disabled.
When Lessor regains DRM master, the disabled leases are automatically
reinstated.

- Lessee can tell the difference between a permanent revoke and a
temporary disable of a lease by the error it gets from the KMS API
(drmModeSetCrtc, drmModePageFlip, drmModeAtomicCommit): EACCESS for
temporary disable, and ENOENT for a revoked lease that is not coming
back.

- If Lessee gets EACCESS, it should stand by and wait for hotplug
events to try again with the hope that the lease comes back.

Some random thoughts from me:

- Need to make sure the client has enough information to listen for
hotplug events in a multi-card system.

- If the Wayland client disconnects, revoke all its leases.
Implementation-wise this is easy as the wl_resource gets
destroyed.

- We probably do not want a Wayland event for compositor dropping DRM
master, because it would be racy in the client against KMS calls and
the client will notice on its own anyway.

- There could be a Wayland event for compositor gaining DRM master, to
tell the client to try KMS again now, but it seems redundant: the DRM
hotplug event provides the same, and needs to be listened to anyway
if the client wants to react to hotplug on its leased connector.

Post by Marius Vlad
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..907efb0
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,150 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<protocol name="drm_lease_unstable_v1">

...

Post by Pekka Paalanen
To me this sounds like a client needs to first open a DRM card node,
iterate through all the resources (is that even possible if it is not a
DRM master?), look at the information and guess which ones the
compositor might be willing to lease out, and then through trial and
error maybe find something that will actually be accepted.

Indeed, that's how the client demo/example does it now.

Well... I assumed that the client can do this on its own.

DRM leasing protocol could be used to allow direct display access from
application sandboxes.

There is one more problem I forgot about: multiple card nodes.
How would a client know which card nodes the compositor is using and
might be able to give out leases for?

It could probably try and probe to see what it can get from DRM card
nodes directly, but that seems awkward.

A compositor could also be using multiple card nodes. If we have DRM
resource advertisements as part of this extension, it needs to take
into account that some resources could be from a different card and
therefore cannot be paired with some other resources.

Right.

Yes.

Yes. If a client wants to lease multiple connectors, I assume it can
just ask each separately. This does assume that the client is not
aiming for shared-CRTC clone mode as it would get more CRTCs than it
needs in that case.

I'm not sure it would be worth the complexity to be able to lease out
some connectors without CRTCs, the chances of being able to use
shared-CRTC clone mode are slim and use cases probably hard to find. If
a use case pops up, we can look at it then.

As for a compositor determining a valid CRTC, it could go as far as
doing a TEST_ONLY or even a real atomic commit of the CRTC/connector
with the preferred video mode to ensure it's supposed to work.

Post by Marius-cristian Vlad
How will the compositor choose which connectors can be leased? We
assume that all connectors can be leased?Â

This would be left as compositor policy.

I would guess that most desktop compositors would lease out only
connectors that are marked as "non-desktop" or explicitly configured to
be leasable and not used by the compositor.

For Weston it would be nice to take things further than that, maybe
even allow leasing connectors that are in use, so that we can
experiment with CRTC/connector hand-over with leases. If a non-VR
application, e.g. a 3D game, would want to drive a display directly,
this is how it would happen. Another problem is how that game would get
input, but that's irrelevant here.

I'm totally ok with the initial weston implementation only leasing out
unused resources.

Post by Marius-cristian Vlad
For a client a unsigned int doesn't really tell anything .... is this
the panel on my right, the display on my left or the upper panel -- or
the VR is just hooked up? Do we advertise this to the client as well in
the form of output names or something equivalent? I guess more general
question show we present the potential leasable resource(s) in a human
form? Would that make sense?
The approach with the client choosing which crtc/connector/plane
somehow assumes that client has a-priory knowledge of which resource
wants... albeit will all the drawbacks you enumerated before.

Right, the advertisement needs to provide enough information.

How do applications that want to drive a display directly actually pick
the displays?

I assume the primary use case here are VR compositors, who are only
looking for HMDs or any "non-desktop" outputs. We need to provide
enough information to let those be identified. I suppose it would be at
least:
- EDID: make
- EDID: model
- EDID: serial number
- physical connector type
- connection status

The protocol needs to be designed to allow new fields to be added, IOW
to be able to add new events carrying bits of information not sent
before, in a backward-compatible manner. The bits of information that
are not available due to the connector being disconnected etc. could be
simply left unsent.

Almost all of the above depend on the currently connected monitor,
which means that they can change at runtime. Therefore hotplug and
unplug needs to be accounted for, in case an application could take a
long while (e.g. interactive UI) to choose the connectors it wants.

The secondary use case are all other programs. They could be interested
in any kind of monitors. They would probably be interested in name and
description from the xdg_output interface. See:
https://cgit.freedesktop.org/wayland/wayland-protocols/commit/?id=d296d0760c186e540438174843f3e93849cc4d70

But because the compositor would be primarily leasing out connectors it
is not using, there is no respective xdg_output, so we should probably
duplicate the name and description.

Saying anything explicitly about monitor layout is likely useless: if
the compositor is not using the monitor, it's not part of its layout.
If a compositor is using a monitor, it could add a word or two about
the layout in the description.

Btw. a compositor should probably send out connector protocol objects,
not events with connector IDs. That would fit the Wayland object model
better. There might not be a need to send the actual connector ID at
all.

If we go with leasing one connector at a time and the compositor
automatically picking the CRTC for it, a connector protocol object
could have a request "lease", creating a lease object which then
delivers the failure or success events.

Thanks,
pq

Marius Vlad

2018-08-20 20:00:48 UTC

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

Signed-off-by: Marius Vlad <marius-***@nxp.com>

Changes since v2:
- advertise connector names 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)
---
Some caveats while at it. This is just in RFC form adapted from the comments
I've mostly got from Pekka Paalanen. It most certainly doesn't address all the issues
brought up: like multi-node card environments/system, doing a TEST_ONLY
commit before giving out a lease, or takes hot-plugging into consideration.
As I was side-tracked to other things and
being on hiatus for a while, I wanted first to get some impressions first if
indeed this is the right approach and do some incremental updates afterwards.

The are some issues which I'd like to point out from the beginning, which
require some further comments. In no particular order:

I've found that I can't pass a wl_array as a way to advertise various
information to the client. (wl_array works fine with integers not with
allocated data). It would probably be better to advertise also
monitor name, other EDID info or available modes, but at the moment I only
join-split a delimiter between connector names and send it out as a string.
While it is ugly I'm not aware of a way to send this information back to the
client in the form of a list.
The client is aware before hand of this delimiter and has the number of entries:
can easily choose or pick one of the connectors. It could be that there are
some other ways this can be achieved, I welcome any kind of feedback here.

Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to the client.
The obvious type="object" interface="drmModeModeInfo" fails to link and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note that this
can only be "used" after the lease has been created.

A server/client implementation to match this protocol
can be found at https://gitlab.freedesktop.org/marius.vlad0/weston/commits/new-drm-lease

Makefile.am | 1 +
unstable/drm-lease/README | 4 +
unstable/drm-lease/drm-lease-unstable-v1.xml | 173 +++++++++++++++++++++++++++
3 files changed, 178 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-***@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..6cb3c0a
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,173 @@
+<?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
+ 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.
+
+ 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="connectors">
+ <description summary="advertise which connectors can be used to request creation for a lease">
+ This event advertises leasable connectors. When multiple connectors are
+ advertised, the client should properly parse and choose one of connectors
+ A client trying to create a lease using zwp_kms_lease_request_v1::create
+ request would use this connector name.
+ </description>
+ <arg name="connectors" type="string" summary="list of connector entries, split by '|' char"/>
+ <arg name="connectors_entries" type="uint" summary="number of connectors found in connectors string"/>
+ </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="get_lease_info">
+ <description summary="request to retrieve info about the lease">
+ Request to retrieve lease information, like leased fd, monitor
+ and connector name.
+ </description>
+ </request>
+
+ <event name="lease_info">
+ <description summary="returns information about the lease">
+ This event returns (among other information about the connector leased)
+ the leased fd which is required for modesetting or queying page flips.
+ The client can use the leased fd to find out DRM-related information
+ like connector, CRTC, and plane ID using drmModeGetLease(). Using that
+ information it can derive a suitable mode useful when performing modeset.
+ </description>
+ <arg name="leased_fd" type="fd" summary="leased fd" />
+ <arg name="conn_name" type="string" summary="connector name of the lease" />
+ <arg name="monitor_name" type="string" summary="monitor 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) and also to
+ revoke the lease.
+ </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="create">
+ <description summary="create">
+ Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
+ Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
+ </description>
+ <arg name="connector" type="string" summary="connector output name"/>
+ </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>
+
+ <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 fd 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>
+
+ </interface>
+</protocol>

--
2.9.3

Philipp Zabel

2018-08-22 07:17:50 UTC

Hi Marius,

Why not just send the connectors one by one, a single event with all
relevant information for each?

Especially EDID info would be most useful for finding the right VR
headset.

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to the client.
The obvious type="object" interface="drmModeModeInfo" fails to link and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note that this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector via the
leased fd?

Post by Marius Vlad
A server/client implementation to match this protocol
can be found at https://gitlab.freedesktop.org/marius.vlad0/weston/commits/new-drm-lease

This crashed for me in drm_lease_manager_create_lease_req with a NULL
pointer dereference because head->output == NULL for an unconnected
head.
Also I noticed zwm_kms_lease_request_v1_implementation is missing the
.destroy request callback.

Post by Marius Vlad
Makefile.am | 1 +
unstable/drm-lease/README | 4 +
unstable/drm-lease/drm-lease-unstable-v1.xml | 173 +++++++++++++++++++++++++++
3 files changed, 178 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
+
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..6cb3c0a
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,173 @@
+<?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
+
+ 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
+ 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.
+
+ 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="connectors">
+ <description summary="advertise which connectors can be used to request creation for a lease">
+ This event advertises leasable connectors. When multiple connectors are
+ advertised, the client should properly parse and choose one of connectors
+ A client trying to create a lease using zwp_kms_lease_request_v1::create
+ request would use this connector name.
+ </description>
+ <arg name="connectors" type="string" summary="list of connector entries, split by '|' char"/>
+ <arg name="connectors_entries" type="uint" summary="number of connectors found in connectors string"/>
+ </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="get_lease_info">
+ <description summary="request to retrieve info about the lease">
+ Request to retrieve lease information, like leased fd, monitor
+ and connector name.
+ </description>
+ </request>
+
+ <event name="lease_info">
+ <description summary="returns information about the lease">
+ This event returns (among other information about the connector leased)
+ the leased fd which is required for modesetting or queying page flips.
+ The client can use the leased fd to find out DRM-related information
+ like connector, CRTC, and plane ID using drmModeGetLease(). Using that
+ information it can derive a suitable mode useful when performing modeset.
+ </description>
+ <arg name="leased_fd" type="fd" summary="leased fd" />
+ <arg name="conn_name" type="string" summary="connector name of the lease" />
+ <arg name="monitor_name" type="string" summary="monitor 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) and also to
+ revoke the lease.
+ </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="create">

There's a level of indentation is missing from here.

Post by Marius Vlad
+ <description summary="create">
+ Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
+ Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
+ </description>
+ <arg name="connector" type="string" summary="connector output name"/>
+ </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>
+
+ <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 fd 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>
+
+ </interface>
+</protocol>

regards
Philipp

Marius-cristian Vlad

2018-08-22 11:12:20 UTC

Post by Daniel Stone
Hi Marius,

Hi,

Post by 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%2
F%2Fcgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc417153538
9d72e9135c9615cecd07b346fd6d7e&data=02%7C01%7Cmarius-
cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&sdata=M
gPbYibU3CTjKcSMJaiqBSP7FfAJD2h%2BLPYiE%2FXJ8z0%3D&reserved=0
[2] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
linux.git%2Fcommit%2F%3Fh%3Dv4.15-
rc9%26id%3D62884cd386b876638720ef88374b31a84ca7ee5f&data=02%7C0
1%7Cmarius-
cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&sdata=S
h%2B1TbPeqbUnaYF%2FwenCZIKTcR9P7l585Ellujk0Bq8%3D&reserved=0
- advertise connector names 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)
- 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)
---
Some caveats while at it. This is just in RFC form adapted from the comments
I've mostly got from Pekka Paalanen. It most certainly doesn't
address all the issues
brought up: like multi-node card environments/system, doing a TEST_ONLY
commit before giving out a lease, or takes hot-plugging into
consideration.
As I was side-tracked to other things and
being on hiatus for a while, I wanted first to get some impressions first if
indeed this is the right approach and do some incremental updates afterwards.
The are some issues which I'd like to point out from the beginning, which
I've found that I can't pass a wl_array as a way to advertise various
information to the client. (wl_array works fine with integers not with
allocated data). It would probably be better to advertise also
monitor name, other EDID info or available modes, but at the moment
I only
join-split a delimiter between connector names and send it out as a
string.
While it is ugly I'm not aware of a way to send this information back to the
client in the form of a list.
can easily choose or pick one of the connectors. It could be that there are
some other ways this can be achieved, I welcome any kind of
feedback here.

Why not just send the connectors one by one, a single event with all
relevant information for each?

Hmm, okay, I'll try do that.

Post by Daniel Stone
Especially EDID info would be most useful for finding the right VR
headset.

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to
the client.
The obvious type="object" interface="drmModeModeInfo" fails to link and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note that this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector via the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

Post by Marius Vlad
A server/client implementation to match this protocol
can be found at https://emea01.safelinks.protection.outlook.com/?ur
l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fco
mmits%2Fnew-drm-lease&data=02%7C01%7Cmarius-
cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&sdata=G
4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3D&reserved=0

Good catch, fixed. You need to fetch it again.

Post by Marius Vlad
Makefile.am                                  |   1 +
unstable/drm-lease/README                    |   4 +
unstable/drm-lease/drm-lease-unstable-v1.xml | 173
+++++++++++++++++++++++++++
3 files changed, 178 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
+
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..6cb3c0a
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,173 @@
+<?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
+
+    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
+      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.
+
+      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="connectors">
+      <description summary="advertise which connectors can be used
to request creation for a lease">
+        This event advertises leasable connectors. When multiple
connectors are
+        advertised, the client should properly parse and choose
one of connectors
+        A client trying to create a lease using
zwp_kms_lease_request_v1::create
+        request would use this connector name.
+      </description>
+      <arg name="connectors" type="string" summary="list of
connector entries, split by '|' char"/>
+      <arg name="connectors_entries" type="uint" summary="number
of connectors found in connectors string"/>
+    </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="get_lease_info">
+      <description summary="request to retrieve info about the
lease">
+        Request to retrieve lease information, like leased fd,
monitor
+        and connector name.
+      </description>
+    </request>
+
+    <event name="lease_info">
+      <description summary="returns information about the lease">
+        This event returns (among other information about the
connector leased)
+        the leased fd which is required for modesetting or queying
page flips.
+        The client can use the leased fd to find out DRM-related
information
+        like connector, CRTC, and plane ID using
drmModeGetLease(). Using that
+        information it can derive a suitable mode useful when
performing modeset.
+      </description>
+      <arg name="leased_fd" type="fd" summary="leased fd" />
+      <arg name="conn_name" type="string" summary="connector name
of the lease" />
+      <arg name="monitor_name" type="string" summary="monitor
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) and also to
+      revoke the lease.
+    </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="create">

There's a level of indentation is missing from here.

Post by Marius Vlad
+    <description summary="create">
+      Create a lease using the connector output name received in
zwp_kms_lease_manager_v1::connectors.
+      Any attempt to use an incorrect connector name will reswult
in zwp_kms_lease_request_v1::failed.
+    </description>
+    <arg name="connector" type="string" summary="connector output
name"/>
+  </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>
+
+  <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 fd 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>
+
+  </interface>
+</protocol>

regards
Philipp

Keith Packard

2018-08-22 16:24:55 UTC

Post by Philipp Zabel
Can't the client query available modes on the passed connector via the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

Just so you know -- basic mode information isn't sufficient to identify
the target headset. Systems have been attempting to identify the headset
assuming the pixel size of the device was unique, but there are now
headsets reporting 'normal' desktop sizes and so something based on
EDID is likely to be necessary.

--
-keith

Philipp Zabel

2018-08-23 06:12:17 UTC

Post by Keith Packard

Post by Philipp Zabel
Can't the client query available modes on the passed connector via the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

I think the best way to identify the target headset would be serial
number information from EDID, which should match the serial number
information from USB for most of them.

I have noticed that my Vive doesn't contain serial number information in
its EDID block - it is impossible to differentiate between two of those
if they are connected at the same time. In that case matching against
manufacturer ID and product code from EDID should be good enough for
most cases.

regards
Philipp

Philipp Zabel

2018-08-23 06:41:30 UTC

Hi,

On Wed, 2018-08-22 at 11:12 +0000, Marius-cristian Vlad wrote:
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.

What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?

Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to
the client.
The obvious type="object" interface="drmModeModeInfo" fails to link and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note that this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector via the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

If there was a "leasable connector" object instead of just the connector
event, that could report modes and EDID data as events, like wl_output.
I'm not sure if that is a good idea or unnecessary complication.

Good catch, fixed. You need to fetch it again.

Thank you, it works now. I've poked at it a bit, and noticed that the
compositor crashes if the same lease is requested again before it was
improperly revoked. This happened in three cases:

- if a second weston-egl-simple-lease is started while the first one
is still running,
- if weston-egl-simple-lease is stopped by a VT switch (because the
next pageflip fails) and started again after switching back to weston,
- if weston-egl-simple-lease is killed with SIGTERM and started again.

In the last case the last frame the last frame of the already killed
application was still visible on the output.

I've also tried pulling and replugging the cable on the leased
connector, which ends with a compositor assertion after replugging:

weston: compositor/main.c:1726: drm_process_layoutput: Assertion `output->output->enabled' failed.

regards
Philipp

Pekka Paalanen

2018-08-23 11:39:05 UTC

Sorry for the potentially duplicate email, my MUA messed up the CC list
on the first go.

On Thu, 23 Aug 2018 08:41:30 +0200

Post by Philipp Zabel
Hi,
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hi,

yes, sending one event per connector is a good design, but see below if
we actually might want to extend that to creating an object per
connector with "per-attribute" events for pieces of information.

Post by Marius-cristian Vlad
Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.
What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?

One certainly cannot rely on wl_output, because that will not be
present for outputs that are not currently used as part of the desktop.
IOW, HMDs are likely never exposed as a wl_output.

Post by Philipp Zabel
Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

These are good questions, I hope answers will be found for what the
Vulkan and other APIs will actually need for advertising what they need
to advertise.

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

Sharing EDID could be tricky mechanically. If we assume that an average
EDID blob is 256 bytes, it would be small enough to pass "inline" in
Wayland, e.g. as a wl_array argument on an event. But since we want to
provide it for all leasable connectors, the burst of data could grow
bigger than the buffers in libwayland, and we start relying on the
kernel socket buffers which are much larger usually.

Maybe it's ok to start with a wl_array while we are still in unstable
protocol, but this question may need to be revisited in the future.

Is there a defined upper limit on EDID size?

Btw. since we may need to share EDID, and we may need applications to
parse EDID to dig out what they need, I believe there will be a demand
for a library to do that. Does any such exist already?

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo).Â Â I'm currently unsure how to pass this back to
the client.
The obvious type="object" interface="drmModeModeInfo" fails to link and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --Â
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note that this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector via the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

The client should not receive "all" the data from the compositor
via Wayland, but only the bits it needs to make the decision whether to
attempt leasing a connector or not.

Modes are part of EDID, so if we send EDID, I think that's good enough.
After all, we want to describe the monitor/HMD foremost, not what the
kernel or the compositor have decided it can do (e.g. add standard
modes the EDID might miss, or prune modes that cannot be used).

That data does not need to be complete either, I think. It should allow
coarse pruning, and yes, ideally it would allow choosing the right
connectors immediately, but clients still have the opportunity to lease
a connector and ask DRM for more details before making the decision.

In summary, I don't know if we need to send the whole EDID or just
selected standard fields from EDID (make, model, serial, ...), and
likewise I'm not sure there is need to send video mode info if that's
not useful for choosing a connector/monitor.

After the client has chosen a connector and leased it, then it will
receive everything it needs directly from the kernel, e.g. a video mode
list.

Post by Philipp Zabel
If there was a "leasable connector" object instead of just the connector
event, that could report modes and EDID data as events, like wl_output.
I'm not sure if that is a good idea or unnecessary complication.

That depends on how much data we want to send. A single message has a
quite limited size. libwayland buffering sets a hard limit at 4096
bytes, but in practise we should use, say, 1024 bytes max. If that
message includes EDID inline, that's already 128-512 bytes taken.

From protocol specification point of view, if the ad is just one event
that carries all per-connector information, it cannot be extended. If we
later want to include more information, we need to duplicate the event
to add new arguments to it, because existing events cannot be modified
without breaking all consumers. OTOH, if each piece of information is
sent as a separate event on a "leasable connector" object, adding new
pieces of information is much cleaner, no need to duplicate the
existing events.

On Thu, 23 Aug 2018 08:49:13 +0200

Post by Philipp Zabel
[...]

Post by Marius-cristian Vlad
+ <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) and also to
+ revoke the lease.
+ </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="create">
+ <description summary="create">
+ Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
+ Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
+ </description>
+ <arg name="connector" type="string" summary="connector output name"/>

Instead of submitting the connector identification as an argument to the
create request, would adding a separate add_connector request be more
extensible?
That would allow to request a single lease for multiple connectors,
or to add a mechanism to request adding overlay planes to the lease.

If there is any possibility that we might want to build a single lease
request from additional bits (e.g. planes) in the future, then I would
recommend the add_connector request on a tentative lease config object
approach. The linux_dmabuf extension already uses the same pattern to
have a variable number of things in the final request.

As said, one cannot add arguments to an existing request or event later
- you have to replace the whole message if you want something added.

Sorry, I'm a bit too busy to do a proper review of the protocol for
now, but I hope these comments push it in a good direction.

Thanks,
pq

Marius-cristian Vlad

2018-08-23 15:12:03 UTC

Post by Pekka Paalanen
Sorry for the potentially duplicate email, my MUA messed up the CC list
on the first go.
On Thu, 23 Aug 2018 08:41:30 +0200

Hi,
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hi,
yes, sending one event per connector is a good design, but see below if
we actually might want to extend that to creating an object per
connector with "per-attribute" events for pieces of information.

Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.
What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?

One certainly cannot rely on wl_output, because that will not be
present for outputs that are not currently used as part of the
desktop.
IOW, HMDs are likely never exposed as a wl_output.

Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

These are good questions, I hope answers will be found for what the
Vulkan and other APIs will actually need for advertising what they need
to advertise.

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

Sharing EDID could be tricky mechanically. If we assume that an average
EDID blob is 256 bytes, it would be small enough to pass "inline" in
Wayland, e.g. as a wl_array argument on an event. But since we want to
provide it for all leasable connectors, the burst of data could grow
bigger than the buffers in libwayland, and we start relying on the
kernel socket buffers which are much larger usually.
Maybe it's ok to start with a wl_array while we are still in unstable
protocol, but this question may need to be revisited in the future.
Is there a defined upper limit on EDID size?
Btw. since we may need to share EDID, and we may need applications to
parse EDID to dig out what they need, I believe there will be a demand
for a library to do that. Does any such exist already?

Isn't this overkill? Asking the client to parse that blob?

Wouldn't make more sense to add the fields Philipp mentions
(manufacturer ID/product code) fields in drm_edid? The compositor
already parses the EDID to provide/supply a serial number/monitor name.

If the client can ask the kernel (through the leased fd) and chooses
what mode it wants, then what would be the point to send the whole
EDID?

Regarding the size, I've modified the proto to include both drm_edid
fields we have currently and the EDID blob. The size depends on the
monitor/VR. My monitors have either 128 or 256-bytes EDIDs.

Post by Philipp Zabel

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to
the client.
The obvious type="object" interface="drmModeModeInfo" fails
to link
and instead
I rely on the fact that a) the client can retrieve IDs from
the
lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note
that
this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector
via
the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

The client should not receive "all" the data from the compositor
via Wayland, but only the bits it needs to make the decision whether to
attempt leasing a connector or not.
Modes are part of EDID, so if we send EDID, I think that's good enough.
After all, we want to describe the monitor/HMD foremost, not what the
kernel or the compositor have decided it can do (e.g. add standard
modes the EDID might miss, or prune modes that cannot be used).
That data does not need to be complete either, I think. It should allow
coarse pruning, and yes, ideally it would allow choosing the right
connectors immediately, but clients still have the opportunity to lease
a connector and ask DRM for more details before making the decision.
In summary, I don't know if we need to send the whole EDID or just
selected standard fields from EDID (make, model, serial, ...), and
likewise I'm not sure there is need to send video mode info if that's
not useful for choosing a connector/monitor.
After the client has chosen a connector and leased it, then it will
receive everything it needs directly from the kernel, e.g. a video mode
list.

If there was a "leasable connector" object instead of just the connector
event, that could report modes and EDID data as events, like
wl_output.
I'm not sure if that is a good idea or unnecessary complication.

That depends on how much data we want to send. A single message has a
quite limited size. libwayland buffering sets a hard limit at 4096
bytes, but in practise we should use, say, 1024 bytes max. If that
message includes EDID inline, that's already 128-512 bytes taken.
From protocol specification point of view, if the ad is just one event
that carries all per-connector information, it cannot be extended. If we
later want to include more information, we need to duplicate the event
to add new arguments to it, because existing events cannot be
modified
without breaking all consumers. OTOH, if each piece of information is
sent as a separate event on a "leasable connector" object, adding new
pieces of information is much cleaner, no need to duplicate the
existing events.
On Thu, 23 Aug 2018 08:49:13 +0200

[...]

+  <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) and also to
+      revoke the lease.
+    </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="create">
+    <description summary="create">
+      Create a lease using the connector output name received in
zwp_kms_lease_manager_v1::connectors.
+      Any attempt to use an incorrect connector name will
reswult in zwp_kms_lease_request_v1::failed.
+    </description>
+    <arg name="connector" type="string" summary="connector
output name"/>

If there is any possibility that we might want to build a single lease
request from additional bits (e.g. planes) in the future, then I would
recommend the add_connector request on a tentative lease config object
approach. The linux_dmabuf extension already uses the same pattern to
have a variable number of things in the final request.
As said, one cannot add arguments to an existing request or event later
- you have to replace the whole message if you want something added.
Sorry, I'm a bit too busy to do a proper review of the protocol for
now, but I hope these comments push it in a good direction.
Thanks,
pq

Pekka Paalanen

2018-08-24 08:26:12 UTC

On Thu, 23 Aug 2018 15:12:03 +0000

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.Â Â

Sharing EDID could be tricky mechanically. If we assume that an average
EDID blob is 256 bytes, it would be small enough to pass "inline" in
Wayland, e.g. as a wl_array argument on an event. But since we want to
provide it for all leasable connectors, the burst of data could grow
bigger than the buffers in libwayland, and we start relying on the
kernel socket buffers which are much larger usually.
Maybe it's ok to start with a wl_array while we are still in unstable
protocol, but this question may need to be revisited in the future.
Is there a defined upper limit on EDID size?
Btw. since we may need to share EDID, and we may need applications to
parse EDID to dig out what they need, I believe there will be a demand
for a library to do that. Does any such exist already?

As I mentioned below, I don't know if we *need* to send EDID. It all
depends on what the applications will need for making the decision to
attempt leasing.

A problem with sending just specific parsed fields of EDID instead of
the blob is that we need to update the protocol if new very useful
fields appear in EDID.

It's a trade-off, both ways can work.

For the record, I think on X11 EDID is already exposed to clients, so
VR runtimes might expect it to be available. Of course, one can get it
after creating the lease, too, so not including it in the leasing
protocol will not prohibit clients from getting it. It just takes more
effort on the client.

Thanks,
pq

Philipp Zabel

2018-08-24 08:57:43 UTC

Hi Pekka,

Post by Pekka Paalanen
Sorry for the potentially duplicate email, my MUA messed up the CC list
on the first go.
On Thu, 23 Aug 2018 08:41:30 +0200

Post by Philipp Zabel
Hi,
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hi,
yes, sending one event per connector is a good design, but see below if
we actually might want to extend that to creating an object per
connector with "per-attribute" events for pieces of information.

One reason for creating an object would be that in some cases (like VR)
the client will want to match on vendor/model/serial from EDID, but in
other scenarios EDID data might not even be available.
When leasing a DPI or LVDS panel without EDID information, the connector
would have to be identified via the connector name, for example.

And object would allow handling both cases the same way when building
the lease request.

Post by Marius-cristian Vlad
Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.
What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?

One certainly cannot rely on wl_output, because that will not be
present for outputs that are not currently used as part of the desktop.
IOW, HMDs are likely never exposed as a wl_output.

Understood, it would have to be a new object. There could be more of
them than wl_outputs, and they wouldn't carry the same information:
output geometry obviously has no place here. But there is some overlap.
The wl_output geometry event also contains "make" and "model" arguments
(but is missing product id or serial). And I notice that zxdg_output_v1
already has a "name" event that likely contains the connector name.
Maybe the leasable connector object should report all of these as
events.

Post by Philipp Zabel
Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

These are good questions, I hope answers will be found for what the
Vulkan and other APIs will actually need for advertising what they need
to advertise.

I suppose this is something that should be discussed on the Vulkan side
first? Given we'd need a new extension there anyway, maybe the solution
is not making this wayland specific at all and just adding a way to
import the leased DRM fd.

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

I'm not sure if pushing the complete EDID is necessary. Maybe there
should be an EDID request if preparsed vendor/product/serial or
connector name are good enough to identify the to-be-leased connector in
most cases.

Post by Pekka Paalanen
Maybe it's ok to start with a wl_array while we are still in unstable
protocol, but this question may need to be revisited in the future.
Is there a defined upper limit on EDID size?

EDID contains an 8-bit number of extension blocks, there should be an
upper limit of about 32 KiB.

Post by Pekka Paalanen
Btw. since we may need to share EDID, and we may need applications to
parse EDID to dig out what they need, I believe there will be a demand
for a library to do that. Does any such exist already?

I'm not sure if there is a commonly used one.

[...]

Post by Pekka Paalanen
The client should not receive "all" the data from the compositor
via Wayland, but only the bits it needs to make the decision whether to
attempt leasing a connector or not.

Seconded, the protocol should contain just enough to make a decision to
lease.

Post by Pekka Paalanen
Modes are part of EDID, so if we send EDID, I think that's good enough.
After all, we want to describe the monitor/HMD foremost, not what the
kernel or the compositor have decided it can do (e.g. add standard
modes the EDID might miss, or prune modes that cannot be used).

What about legacy LVDS and DPI displays that have no EDID information?
I expect those would be matched by connector name, but maybe there are
also use cases where somebody would select a connector to lease
depending on its native resolution or refresh rate.

Post by Pekka Paalanen
That data does not need to be complete either, I think. It should allow
coarse pruning, and yes, ideally it would allow choosing the right
connectors immediately, but clients still have the opportunity to lease
a connector and ask DRM for more details before making the decision.

Ok.

Post by Pekka Paalanen
In summary, I don't know if we need to send the whole EDID or just
selected standard fields from EDID (make, model, serial, ...), and
likewise I'm not sure there is need to send video mode info if that's
not useful for choosing a connector/monitor.

For the use cases that I can currently imagine (VR, special fixed
displays, projectors on a special connector) either

manufacturer id (make), product code (better than monitor name
string) and monitor serial number
or
connector name

should be good enough. But as said, I'm not sure if there could be
cases where just matching native resolution or refresh rate would
be required.

Post by Pekka Paalanen
After the client has chosen a connector and leased it, then it will
receive everything it needs directly from the kernel, e.g. a video mode
list.

Yes.

That depends on how much data we want to send. A single message has a
quite limited size. libwayland buffering sets a hard limit at 4096
bytes, but in practise we should use, say, 1024 bytes max. If that
message includes EDID inline, that's already 128-512 bytes taken.
From protocol specification point of view, if the ad is just one event
that carries all per-connector information, it cannot be extended. If we
later want to include more information, we need to duplicate the event
to add new arguments to it, because existing events cannot be modified
without breaking all consumers. OTOH, if each piece of information is
sent as a separate event on a "leasable connector" object, adding new
pieces of information is much cleaner, no need to duplicate the
existing events.

If we had an object, we could start with an event that just carries
model/vendor/serial and add an edid event (possibly even with an edid
request) later, if necessary.

[...]

Post by Philipp Zabel
Instead of submitting the connector identification as an argument to the
create request, would adding a separate add_connector request be more
extensible?
That would allow to request a single lease for multiple connectors,
or to add a mechanism to request adding overlay planes to the lease.

If there is any possibility that we might want to build a single lease
request from additional bits (e.g. planes) in the future, then I would
recommend the add_connector request on a tentative lease config object
approach. The linux_dmabuf extension already uses the same pattern to
have a variable number of things in the final request.
As said, one cannot add arguments to an existing request or event later
- you have to replace the whole message if you want something added.
Sorry, I'm a bit too busy to do a proper review of the protocol for
now, but I hope these comments push it in a good direction.

Thank you for the detailed initial feedback.

regards
Philipp

Marius-cristian Vlad

2018-08-24 09:21:59 UTC

Post by Philipp Zabel
Hi Pekka,

Post by Pekka Paalanen
Sorry for the potentially duplicate email, my MUA messed up the CC list
on the first go.
On Thu, 23 Aug 2018 08:41:30 +0200

Hi,
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hi,
yes, sending one event per connector is a good design, but see below if
we actually might want to extend that to creating an object per
connector with "per-attribute" events for pieces of information.

One reason for creating an object would be that in some cases (like VR)
the client will want to match on vendor/model/serial from EDID, but in
other scenarios EDID data might not even be available.
When leasing a DPI or LVDS panel without EDID information, the
connector
would have to be identified via the connector name, for example.
And object would allow handling both cases the same way when building
the lease request.

To make sure I understand this "object" leasing. Instead of advertising
connectors this way:

    <event name="connector">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector. This allows the
client to
        choose which connector the compositor should lease. It can
either
        use connector name, monitor name, or if that's not sufficient
to parse
        EDID blob.
      </description>
      <arg name="conn_name" type="string" summary="connector name" />
      <arg name="eisa_id" type="string" summary="eisa id"/>
      <arg name="monitor_name" type="string" summary="monitor name"/>
      <arg name="pnp_id" type="string" summary="pnp id"/>
      <arg name="serial_num" type="string" summary="serial number"/>
      <arg name="edid" type="array" summary="EDID blob"/>
    </event>

We do something like this:

    <event name="connector_object">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector object.
      </description>
      <arg name="conn_obj" type="new_id"
interface="zwp_kms_lease_connector_v1"
      summary="the new temporary" />
    </event>

Then that interface is used to query info (like EDID/conn_name/monitor
name) and using that information to ask/revoke the lease (based on
either connector name, or based on EDID)?

Is this the right assumption?

Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.
What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?

One certainly cannot rely on wl_output, because that will not be
present for outputs that are not currently used as part of the desktop.
IOW, HMDs are likely never exposed as a wl_output.

Understood, it would have to be a new object. There could be more of
output geometry obviously has no place here. But there is some
overlap.
The wl_output geometry event also contains "make" and "model"
arguments    <event
name="connector"><

      <description summary="advertise a leasable
connector"><
        This event advertises a leasable connector. This allows the
client to<
        choose which connector the compositor should lease. It can
either<
        use connector name, monitor name, or if that's not sufficient
to parse<
        EDID
blob.<

      </description><

      <arg name="conn_name" type="string" summary="connector name"
/><
      <arg name="eisa_id" type="string" summary="eisa
id"/><
      <arg name="monitor_name" type="string" summary="monitor
name"/><
      <arg name="pnp_id" type="string" summary="pnp
id"/><
      <arg name="serial_num" type="string" summary="serial
number"/><
      <arg name="edid" type="array" summary="EDID
blob"/><
    </event><

(but is missing product id or serial). And I notice that
zxdg_output_v1
already has a "name" event that likely contains the connector name.
Maybe the leasable connector object should report all of these as
events.

Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

These are good questions, I hope answers will be found for what the
Vulkan and other APIs will actually need for advertising what they need
to advertise.

I suppose this is something that should be discussed on the Vulkan side
first? Given we'd need a new extension there anyway, maybe the
solution
is not making this wayland specific at all and just adding a way to
import the leased DRM fd.

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

Post by Pekka Paalanen
Maybe it's ok to start with a wl_array while we are still in
unstable
protocol, but this question may need to be revisited in the future.
Is there a defined upper limit on EDID size?

EDID contains an 8-bit number of extension blocks, there should be an
upper limit of about 32 KiB.

I'm not sure if there is a commonly used one.
[...]

Post by Pekka Paalanen
The client should not receive "all" the data from the compositor
via Wayland, but only the bits it needs to make the decision
whether to
attempt leasing a connector or not.

Seconded, the protocol should contain just enough to make a decision to
lease.

What about legacy LVDS and DPI displays that have no EDID
information?
I expect those would be matched by connector name, but maybe there are
also use cases where somebody would select a connector to lease
depending on its native resolution or refresh rate.

Ok.

For the use cases that I can currently imagine (VR, special fixed
displays, projectors on a special connector) either
manufacturer id (make), product code (better than monitor name
string) and monitor serial number
or
connector name
should be good enough. But as said, I'm not sure if there could be
cases where just matching native resolution or refresh rate would
be required.

Post by Pekka Paalanen
After the client has chosen a connector and leased it, then it will
receive everything it needs directly from the kernel, e.g. a video mode
list.

Yes.

That depends on how much data we want to send. A single message has a
quite limited size. libwayland buffering sets a hard limit at 4096
bytes, but in practise we should use, say, 1024 bytes max. If that
message includes EDID inline, that's already 128-512 bytes taken.
From protocol specification point of view, if the ad is just one event
that carries all per-connector information, it cannot be extended. If we
later want to include more information, we need to duplicate the event
to add new arguments to it, because existing events cannot be modified
without breaking all consumers. OTOH, if each piece of information is
sent as a separate event on a "leasable connector" object, adding new
pieces of information is much cleaner, no need to duplicate the
existing events.

If we had an object, we could start with an event that just carries
model/vendor/serial and add an edid event (possibly even with an edid
request) later, if necessary.
[...]

Instead of submitting the connector identification as an argument to the
create request, would adding a separate add_connector request be more
extensible?
That would allow to request a single lease for multiple
connectors,
or to add a mechanism to request adding overlay planes to the
lease.

If there is any possibility that we might want to build a single lease
request from additional bits (e.g. planes) in the future, then I would
recommend the add_connector request on a tentative lease config object
approach. The linux_dmabuf extension already uses the same pattern to
have a variable number of things in the final request.
As said, one cannot add arguments to an existing request or event later
- you have to replace the whole message if you want something added.
Sorry, I'm a bit too busy to do a proper review of the protocol for
now, but I hope these comments push it in a good direction.

Thank you for the detailed initial feedback.
regards

.com/watch?v=QulEM-Azz

Philipp Zabel

2018-08-24 09:58:44 UTC

Hi Marius,
[...]

Post by Pekka Paalanen
yes, sending one event per connector is a good design, but see
below if we actually might want to extend that to creating an
object per connector with "per-attribute" events for pieces of
information.

And object would allow handling both cases the same way when building
the lease request.

To make sure I understand this "object" leasing. Instead of advertising
    <event name="connector">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector. This allows the
client to
        choose which connector the compositor should lease. It can
either
        use connector name, monitor name, or if that's not sufficient
to parse
        EDID blob.
      </description>
      <arg name="conn_name" type="string" summary="connector name" />
      <arg name="eisa_id" type="string" summary="eisa id"/>
      <arg name="monitor_name" type="string" summary="monitor name"/>
      <arg name="pnp_id" type="string" summary="pnp id"/>
      <arg name="serial_num" type="string" summary="serial number"/>
      <arg name="edid" type="array" summary="EDID blob"/>
    </event>
    <event name="connector_object">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector object.
      </description>
      <arg name="conn_obj" type="new_id"
interface="zwp_kms_lease_connector_v1"
      summary="the new temporary" />
    </event>
Then that interface is used to query info (like EDID/conn_name/monitor
name)

Yes, that is my understanding as well.

I expect that for all currently known use cases the connector name (like
xdg_output.name) and the monitor vendor / product id / serial string
should be sufficient. Maybe the native resolution and refresh rate could
be useful as well.

I would suggest leaving the raw EDID out for now, a request for it can
be added later to zwp_kms_lease_connector_v1, if necessary. It won't be
possible to pass otentially huge complete EDIDs in an array argument.

Post by Marius-cristian Vlad
and using that information to ask/revoke the lease (based on
either connector name, or based on EDID)?

I imagine that object to be passed to the lease request's add_connector
request directly:

<interface name="zwp_kms_lease_request_v1">
...
<request name="add_connector">
<arg name="connector" type="object"
interface="zwp_kms_lease_connector_v1"
summary="a leasable connector to be added to the lease request"/>
</request>
</interface>

That way there is no need for separate requests "add_connector_by_name",
"add_connnector_by_vendor_product_serial" etc.

The same could potentially be done later with a "zwp_kms_lease_plane_v1"
object and "add_plane" request.

regards
Philipp

Marius-cristian Vlad

2018-08-24 12:10:25 UTC

Post by Daniel Stone
Hi Marius,
[...]

And object would allow handling both cases the same way when building
the lease request.

To make sure I understand this "object" leasing. Instead of
advertising
    <event name="connector">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector. This allows
the
           client to
        choose which connector the compositor should lease. It can
either
        use connector name, monitor name, or if that's not sufficient
to parse
        EDID blob.
      </description>
      <arg name="conn_name" type="string" summary="connector name" />
      <arg name="eisa_id" type="string" summary="eisa id"/>
      <arg name="monitor_name" type="string" summary="monitor name"/>
      <arg name="pnp_id" type="string" summary="pnp id"/>
      <arg name="serial_num" type="string" summary="serial number"/>
      <arg name="edid" type="array" summary="EDID blob"/>
    </event>
    <event name="connector_object">
      <description summary="advertise a leasable connector">
        This event advertises a leasable connector object.
      </description>
      <arg name="conn_obj" type="new_id"
interface="zwp_kms_lease_connector_v1"
      summary="the new temporary" />
    </event>
Then that interface is used to query info (like
EDID/conn_name/monitor
name)

Yes, that is my understanding as well.
I expect that for all currently known use cases the connector name (like
xdg_output.name) and the monitor vendor / product id / serial string
should be sufficient. Maybe the native resolution and refresh rate could
be useful as well.
I would suggest leaving the raw EDID out for now, a request for it can
be added later to zwp_kms_lease_connector_v1, if necessary. It won't be
possible to pass otentially huge complete EDIDs in an array argument.

Post by Marius-cristian Vlad
and using that information to ask/revoke the lease (based on
either connector name, or based on EDID)?

I imagine that object to be passed to the lease request's
add_connector
<interface name="zwp_kms_lease_request_v1">
    ...
    <request name="add_connector">
      <arg name="connector" type="object"
           interface="zwp_kms_lease_connector_v1"
           summary="a leasable connector to be added to the lease
request"/>
    </request>
</interface>
That way there is no need for separate requests
"add_connector_by_name",
"add_connnector_by_vendor_product_serial" etc.
The same could potentially be done later with a
"zwp_kms_lease_plane_v1"
object and "add_plane" request.

Got it. Will give it a test and see how it goes.

Post by Daniel Stone
regards
Philipp

Marius-cristian Vlad

2018-08-23 15:14:21 UTC

Post by Philipp Zabel
Hi,
[...]

Post by Philipp Zabel
Why not just send the connectors one by one, a single event with all
relevant information for each?

Hmm, okay, I'll try do that.

I'm wondering what should be used to identify a connector to a
hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
other APIs that may request leases on the application's behalf.
What could be passed into a Vulkan function to request the lease and
retrieve the corresponding VkDisplayKHR object? wl_display and
make/model/serial strings? wl_display and drm connector name?
Or is there need for an object describing a leasable connector,
similar to wl_output?
Or should the leasing be done by the application itself, outside of
Vulkan, and just the zwp_kms_lease_v1 object be passed in?

Post by Philipp Zabel
Especially EDID info would be most useful for finding the right VR
headset.

Post by Marius Vlad
Secondly, when doing a modeset, the client requires a valid mode
(drmModeModeInfo). I'm currently unsure how to pass this back to
the client.
The obvious type="object" interface="drmModeModeInfo" fails to
link
and instead
I rely on the fact that a) the client can retrieve IDs from the lease using
lease API (drmModeGetLease()) which gives a connector id --
or alternately can also use a wl_array to pass that,
and b) the client can use the leased fd to iterate over
connectors.
Matching those two would allow to get a valid
drmModeModeInfo object to pass it when modesetting (for more info
see the client implementation provided at the end).
Question is, is this an acceptable way of doing it? Do note
that
this
can only be "used" after the lease has been created.

Can't the client query available modes on the passed connector
via
the
leased fd?

That's how the client does it now, it uses the leased fd to query
available modes. Presumably the client should have/receive all the data
from the compositor....

If there was a "leasable connector" object instead of just the
connector
event, that could report modes and EDID data as events, like
wl_output.
I'm not sure if that is a good idea or unnecessary complication.

Post by Marius Vlad
A server/client implementation to match this protocol
can be found at https://emea01.safelinks.protection.outlook.com
/?ur
l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%
2Fco
mmits%2Fnew-drm-lease&data=02%7C01%7Cmarius-
cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C68
6ea1
d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&sda
ta=G
4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3D&reserved=0

This crashed for me in drm_lease_manager_create_lease_req with a NULL
pointer dereference because head->output == NULL for an
unconnected
head.
Also I noticed zwm_kms_lease_request_v1_implementation is missing the
.destroy request callback.

Good catch, fixed. You need to fetch it again.

Thank you, it works now. I've poked at it a bit, and noticed that the
compositor crashes if the same lease is requested again before it was
- if a second weston-egl-simple-lease is started while the first one
is still running,
- if weston-egl-simple-lease is stopped by a VT switch (because the
next pageflip fails) and started again after switching back to
weston,
- if weston-egl-simple-lease is killed with SIGTERM and started again.
In the last case the last frame the last frame of the already killed
application was still visible on the output.
I've also tried pulling and replugging the cable on the leased

Yes, all of those have to be fixed. I'll send an updated version of
the implementation for a new version of the protocol.

Thanks for testing this!

Post by Philipp Zabel
weston: compositor/main.c:1726: drm_process_layoutput: Assertion
`output->output->enabled' failed.
regards
Philipp

Philipp Zabel

2018-08-23 06:49:13 UTC

On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
[...]

Post by Marius Vlad
+ <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) and also to
+ revoke the lease.
+ </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="create">
+ <description summary="create">
+ Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
+ Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
+ </description>
+ <arg name="connector" type="string" summary="connector output name"/>

Instead of submitting the connector identification as an argument to the
create request, would adding a separate add_connector request be more
extensible?

That would allow to request a single lease for multiple connectors,
or to add a mechanism to request adding overlay planes to the lease.

regards
Philipp

Marius Vlad

2018-08-29 11:00:06 UTC

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-***@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. This has multiple
ramification like what happens when the client un-expectingly dies and doesn't
revoking the lease, or when hot-plugging the connector and weston tries to get
a hold of a connector previously leased? It could be there might be other
situations where the compositor will need to revoke the lease.

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?

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-***@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
+ 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.
+
+ 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. 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.
+
+ 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.
+
+ 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
+ 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>
+
+ <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.
+ </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>
+
+ <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>
+
+ <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>

--
2.9.3

Philipp Zabel

2018-08-30 07:03:06 UTC

Hi Marius,

Disclaimer: I don't feel very qualified to comment on the protocol
design, take this with a grain of salt.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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
+
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
+
+ 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'?

Post by Marius Vlad
+ 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.

Post by Marius Vlad
+ 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'?

Post by Marius Vlad
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?

Post by Marius Vlad
+
+ 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.

Post by Marius Vlad
+
+ 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'?

Post by Marius Vlad
+ 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.

Post by Marius Vlad
+ <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.

Post by Marius Vlad
+ </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.

Post by Marius Vlad
+ <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?

Post by Marius Vlad
+ <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

Marius-cristian Vlad

2018-08-30 11:01:29 UTC

Excuse my MUA, but I have no other means to reply atm.

-----Original Message-----
From: Philipp Zabel <***@gmail.com>
Sent: Thursday, August 30, 2018 10:03 AM
To: Marius-cristian Vlad <marius-***@nxp.com>; wayland-***@lists.freedesktop.org
Cc: ***@keithp.com; ***@de.adit-jv.com; ***@fooishbar.org; ***@gmail.com; sardemff7+***@sardemff7.net; ***@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 😊.

Post by 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
- 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
- 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)
- 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)
---
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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.

Post by Marius Vlad
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
+
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
+
+ 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.

'Tree of DRM masters' sounds like subleasing is still supported.
I think that got removed.

Mvlad: Right, have to check that.

'and have to be "added" to a lease request'?

Mvlad: Sure do.

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.

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.

'can be leasable'?

Mvlad: typo 😊

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.

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?

Post by Marius Vlad
+ </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.

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.

regards
Philipp

Philipp Zabel

2018-08-30 12:38:23 UTC

On Thu, 2018-08-30 at 11:01 +0000, Marius-cristian Vlad wrote:
[...]

Post by Marius Vlad
One interesting question is how to handle the situation when the
client deliberately, or not, holds the lease indefinitely.

when this

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
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),

The compositor has to close lease_fd after sending it to the client via
zwp_kms_lease_v1_send_leased_fd. Otherwise there's two open references
to the lease drm_master, and closing only the client's reference will
not cause it to be destroyed.

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.

If there is only one open lease_fd reference in the client, there is no
need for the revoke request. The client can just close the fd and the
compositor will revoke the lease object after gets the hotplug event and
notices the lessee is gone.

Now if the client keeps the leased_fd open but requests to destroy the
zwp_kms_lease_v1 object, the compositor must revoke the DRM lease, same
as when a VT switch happens or when the connector gets unplugged.

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.

My understanding is that if the client dies, the last open fd
referencing the lease drm_master is closed

Post by Marius Vlad
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.

Post by Marius Vlad
or when hot-plugging the connector and weston tries to get a hold of
a connector previously leased?

Could that be via the revoked event on the zwp_kms_lease_v1?

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.

Right. If at some point the client suddenly fails to pageflip, it should
be able to cope.

[...]

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.

I think the connector should bring its CRTC and a primary plane.

At least for devices that have overlay planes that can move between
CRTCs, overlay planes should not be included implicitly without the
client asking for them, because the compositor may have use for them.

I suppose for devices that have planes fixed to CRTCs it wouldn't hurt
to just lease all planes for the connected CRTC, but for consistency I
wouldn't add unrequested overlay planes here either.

Overlay planes could be listed as zwp_kms_plane_v1 objects, just as
connectors.

regards
Philipp

Marius-cristian Vlad

2018-08-31 10:09:28 UTC

Post by Philipp Zabel
[...]

One interesting question is how to handle the situation when the
client deliberately, or not, holds the lease indefinitely.

when this

The compositor has to close lease_fd after sending it to the client via
zwp_kms_lease_v1_send_leased_fd. Otherwise there's two open
references
to the lease drm_master, and closing only the client's reference will
not cause it to be destroyed.

Yes, you are correct. I mistakenly though I can only close it after the
client sends the revoke request.

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.

If there is only one open lease_fd reference in the client, there is no
need for the revoke request. The client can just close the fd and the
compositor will revoke the lease object after gets the hotplug event and
notices the lessee is gone.
Now if the client keeps the leased_fd open but requests to destroy the
zwp_kms_lease_v1 object, the compositor must revoke the DRM lease, same
as when a VT switch happens or when the connector gets unplugged.

Alright I'll re-work this part and send an update.

My understanding is that if the client dies, the last open fd
referencing the lease drm_master is closed

Yep, I've tested it now and that's how it works.

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?

Could that be via the revoked event on the zwp_kms_lease_v1?

I'd say yes.

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.

Right. If at some point the client suddenly fails to pageflip, it should
be able to cope.
[...]

I think the connector should bring its CRTC and a primary plane.
At least for devices that have overlay planes that can move between
CRTCs, overlay planes should not be included implicitly without the
client asking for them, because the compositor may have use for them.
I suppose for devices that have planes fixed to CRTCs it wouldn't hurt
to just lease all planes for the connected CRTC, but for consistency I
wouldn't add unrequested overlay planes here either.
Overlay planes could be listed as zwp_kms_plane_v1 objects, just as
connectors.

Uhum, alright. I'll have to test this as well. Thanks for the feedback.

Will send an update soon.

Post by Philipp Zabel
regards
Philipp

Marius Vlad

2018-09-04 14:39:02 UTC

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:
- send also information (gratuitously) about the connector once the client has it
a zwp_kms_lease_connector_v1 object (Philipp Zabel)
- use the destructor of zwp_kms_lease_v1 to revoke the lease,
and remove the zwp_kms_lease_request_v1::revoke request, as this
will no longer be necessary (Philipp Zabel)
- once the client obtains a zwp_kms_lease_v1 object, using the temporary
zwp_kms_lease_request_v1 object is no longer necessary so the client
can freely call its destructor (Philipp Zabel)
- polished the main commentary to reflect these changes and fixed some
minor typos (Philipp Zabel)
- removed 'revoked' event entirely as it adds complexity without adding
too much benefit.

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 manager interface to explain how the protocol
is designed to work

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-***@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

Makefile.am | 1 +
unstable/drm-lease/README | 4 +
unstable/drm-lease/drm-lease-unstable-v1.xml | 226 +++++++++++++++++++++++++++
3 files changed, 231 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-***@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..9fcaea5
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,226 @@
+<?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 (revokes) the lease, or
+ the new 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.
+
+ 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" 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
+ 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.
+
+ 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">
+ 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 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>
+
+ <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.
+ </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>
+
+ </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>
+
+ </interface>
+</protocol>

--
2.9.3

Philipp Zabel

2018-09-05 06:55:46 UTC

Hi Marius,

thank you for the update!

[...]

Post by Marius Vlad
- 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.

Post by Marius Vlad
---
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.

[...]

Post by Marius Vlad
+ 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.

Post by Marius Vlad
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/

Post by Marius Vlad
+ 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.

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

Post by Marius Vlad
+ 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"?

Post by Marius Vlad
+ 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.

Post by Marius Vlad
+
+ <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.

Post by Marius Vlad
+ </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.

Post by Marius Vlad
+ </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

Marius-cristian Vlad

2018-09-06 10:45:34 UTC

_______________________________________________
wayland-devel mailing list
wayland-***@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-deve

Marius-cristian Vlad

2018-09-06 11:29:06 UTC

[resending... maybe this time works]

Post by Daniel Stone
Hi Marius,
thank you for the update!

Thank you for taking the time to look over this.

[...]

Post by Marius Vlad
- removed 'revoked' event entirely as it adds complexity without adding
too much benefit.

Post by Marius Vlad
---
An implementation for this version can be found at
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
gitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fcommits%2Fdrm-
lease-advertise-each-connnector-object-
fixes&data=02%7C01%7Cmarius-
cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&sdata=M
UrWEApwiGgSvcz8yKiT9Tm8Ij3UpGs6Ai5ioTtuWDI%3D&reserved=0

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
[output]
name=HDMI-A-1
leasable=on
mode=off
crashes the compositor.

Yes, I've never taken this into consideration. I assume this is the
case for HMDs where by default the connector will be disconnected? The
output contains the scanout_plane which contains the plane id, and
obviously for a disconnected output this will not be the case.

I fixed the crash by checking if the output is set, but most likely
this not the proper fix, if this is required.

Post by Daniel Stone
[...]

Post by Marius Vlad
+ 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.

Care to explain a bit how exactly does the client reaches that state?

"connector_added", "connected_add_failed" are events that shrink the
window in which the advertised connectors might be different when
actually adding the connector.

Post by Marius Vlad
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/

It's a confusion of terminology. Objects in compositor and objects
for the lease creation.

Post by Marius Vlad
+      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 client closes the leased drm fd.
- The compositor revokes the lease because objects in the lease become
unavailable for leasing.

Yep, I'll add that.

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

Okay will modify.

Post by Marius Vlad
+        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"?

Idem.

Post by Marius Vlad
+        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.

Alright, will gratuitously send the file descriptor as well.

Post by Marius Vlad
+
+    <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.

Yes, will include it.

Post by Marius Vlad
+      </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.

Okay, I'll add that as well.

Post by Marius Vlad
+  </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.

I've kept these events because "failed" event seemed to generic for me,
and explained a little bit in the beginning another reason why I've
kept them.

Post by Daniel Stone
regards
Philipp

Philipp Zabel

2018-09-06 13:36:00 UTC

Hi Marius,

Post by Philipp Zabel
[...]

Post by Marius Vlad
- 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.

[...]

Post by Philipp Zabel
Currently, all heads without enabled output are skipped, and trying
[output]
name=HDMI-A-1
leasable=on
mode=off
crashes the compositor.

Yes, I've never taken this into consideration. I assume this is the
case for HMDs where by default the connector will be disconnected?

Yes, in fact if the current implementation [1] is accepted, it will by
default behave exactly as if "mode=off" was set for non-desktop displays
in weston.ini.

[1] https://gitlab.freedesktop.org/wayland/weston/merge_requests/12

Post by Marius-cristian Vlad
The output contains the scanout_plane which contains the plane id,
and obviously for a disconnected output this will not be the case.

I see, that is a problem. drm_output contains the crtc_id as well, so do
we just need a way to attach a drm_head to a disabled drm_output?

[...]

Post by Philipp Zabel
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.

Care to explain a bit how exactly does the client reaches that state?

I was thinking first about a hypothetical presentation application that
gets notified as soon as a projector is plugged into a non-desktop
connector. That way one could start the presentation program and plug in
the connector in either order and never have the desktop spill out onto
the projector.

[...]