[wayland-protocols,v4] unstable/drm-lease: DRM lease protocol support

Submitted by Marius-cristian Vlad on Aug. 29, 2018, 11 a.m.

Details

Message ID 20180829110006.20995-1-marius-cristian.vlad@nxp.com
State New
Series "RFC: unstable: DRM lease support"
Headers show

Commit Message

Marius-cristian Vlad Aug. 29, 2018, 11 a.m.
Simple protocol extension to manage DRM lease. Based on the work by Keith
Packard in [1], respectively [2].

[1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f

Changes since v3:
- instead of advertising all leasable connectors at once, advertise each
connector for each leasable output.  Use an object to represent a
leasable connector and provide a request to add that leasable connector
when creating the lease (Philipp Zabel)
- add two events to handle add_connector request
- add some comments in the manager interface to explain how the protocol
can/should be used

Changes since v2:
- advertise connectors when creating a lease request object (Pekka Paalanen)
- when revoking the lease use the lease object instead of lessee id (Pekka Paalanen)
- various grammar/spelling fixes (Pekka Paalanen)

Changes since v1:
- added manager: advertise lease capability and manage the lease (Daniel Stone)
- add request(s) for adding connector/crtc/plane to behave more like dmabuf (Daniel Stone)

Signed-off-by: Marius Vlad <marius-cristian.vlad@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

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 6394e26..5d13dca 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -4,6 +4,7 @@  unstable_protocols =								\
 	unstable/pointer-gestures/pointer-gestures-unstable-v1.xml		\
 	unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml		\
 	unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml			\
+	unstable/drm-lease/drm-lease-unstable-v1.xml				\
 	unstable/text-input/text-input-unstable-v1.xml				\
 	unstable/text-input/text-input-unstable-v3.xml				\
 	unstable/input-method/input-method-unstable-v1.xml			\
diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
new file mode 100644
index 0000000..a25600c
--- /dev/null
+++ b/unstable/drm-lease/README
@@ -0,0 +1,4 @@ 
+Linux DRM lease
+
+Maintainers:
+Marius Vlad <marius-cristian.vlad@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>

Comments

Philipp Zabel Aug. 30, 2018, 7:03 a.m.
Hi Marius,

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

Am Mittwoch, den 29.08.2018, 14:00 +0300 schrieb Marius Vlad:
> Simple protocol extension to manage DRM lease. Based on the work by Keith
> Packard in [1], respectively [2].
> 
> [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f
> 
> Changes since v3:
> - instead of advertising all leasable connectors at once, advertise each
> connector for each leasable output.  Use an object to represent a
> leasable connector and provide a request to add that leasable connector
> when creating the lease (Philipp Zabel)
> - add two events to handle add_connector request
> - add some comments in the manager interface to explain how the protocol
> can/should be used
> 
> Changes since v2:
> - advertise connectors when creating a lease request object (Pekka Paalanen)
> - when revoking the lease use the lease object instead of lessee id (Pekka Paalanen)
> - various grammar/spelling fixes (Pekka Paalanen)
> 
> Changes since v1:
> - added manager: advertise lease capability and manage the lease (Daniel Stone)
> - add request(s) for adding connector/crtc/plane to behave more like dmabuf (Daniel Stone)
> 
> Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>
> ---
> A (rather incomplete) implementation for this version can be found at
> https://gitlab.freedesktop.org/marius.vlad0/weston/tree/drm-lease-advertise-each-connnector-object
> 
> One interesting question is how to handle the situation when the client
> deliberately, or not, holds the lease indefinitely.

The client implicitly cancels the lease by closing the lease fd (or by
having it closed automatically when it is terminated). The compositor
will get a hotplug event then, and it has to check the lessee list
happens and revoke leases to lost lessees.

>  This has multiple
> ramification like what happens when the client un-expectingly dies and doesn't
> revoking the lease,

See above, a lease terminates when its last fd is closed.

>  or when hot-plugging the connector and weston tries to get
> a hold of a connector previously leased?

I'd say hot-unplugging a connector that is part of a lease should cause
this lease to be canceled.

>  It could be there might be other
> situations where the compositor will need to revoke the lease. 

The VT switch case, for example. If the compositor has to give up
drm_master temporarily, it will have to revoke all leases first.

> In the implementation we could deny the compositor to get a hold of the
> connector when hot-plugging that output, if that's desirable, and presumably
> establish a way to detect periodically if the lease is still in use.
>
> Alternatively, shouldn't we use something like a ping/pong approach in which
> the client (in rendering part), sends periodically an alive signal?

I don't think the compositor should care what the lessee is doing with
its lease, only whether it is still holding the fd.

>  Makefile.am                                  |   1 +
>  unstable/drm-lease/README                    |   4 +
>  unstable/drm-lease/drm-lease-unstable-v1.xml | 244 +++++++++++++++++++++++++++
>  3 files changed, 249 insertions(+)
>  create mode 100644 unstable/drm-lease/README
>  create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6394e26..5d13dca 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -4,6 +4,7 @@ unstable_protocols =								\
>  	unstable/pointer-gestures/pointer-gestures-unstable-v1.xml		\
>  	unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml		\
>  	unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml			\
> +	unstable/drm-lease/drm-lease-unstable-v1.xml				\
>  	unstable/text-input/text-input-unstable-v1.xml				\
>  	unstable/text-input/text-input-unstable-v3.xml				\
>  	unstable/input-method/input-method-unstable-v1.xml			\
> diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
> new file mode 100644
> index 0000000..a25600c
> --- /dev/null
> +++ b/unstable/drm-lease/README
> @@ -0,0 +1,4 @@
> +Linux DRM lease
> +
> +Maintainers:
> +Marius Vlad <marius-cristian.vlad@nxp.com>
> diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml b/unstable/drm-lease/drm-lease-unstable-v1.xml
> new file mode 100644
> index 0000000..40eedb4
> --- /dev/null
> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> @@ -0,0 +1,244 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="drm_lease_unstable_v1">
> +
> +<copyright>
> +    Copyright 2018 NXP
> +
> +    Permission is hereby granted, free of charge, to any person obtaining a
> +    copy of this software and associated documentation files (the "Software"),
> +    to deal in the Software without restriction, including without limitation
> +    the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +    and/or sell copies of the Software, and to permit persons to whom the
> +    Software is furnished to do so, subject to the following conditions:
> +
> +    The above copyright notice and this permission notice (including the next
> +    paragraph) shall be included in all copies or substantial portions of the
> +    Software.
> +
> +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +    DEALINGS IN THE SOFTWARE.
> +</copyright>
> +
> +  <interface name="zwp_kms_lease_manager_v1" version="1">
> +    <description summary="lease manager">
> +      This interface makes use of DRM lease written by Keith Packard.
> +
> +      A DRM master can create another DRM master and ``lease'' resources it has
> +      control over to the new DRM master. Once leased, resources can not be
> +      controlled by the owner unless the owner cancels the lease, or the new

Should this say 'unless the owner revokes the lease'?

> +      DRM master is closed.
> +
> +      A lease is a contract between the Lessor (DRM master which has leased out
> +      resources to one or more other DRM masters) and a Lessee
> +      (DRM master which controls resources leased from another DRM master). This
> +      contract specifies which resources may be controlled by the Lessee.
> +
> +      The Lessee can issue modesetting/page-flipping atomic operations etc.,
> +      just like a Lessor using the leased file-descriptor passed by the Lessor.
> +
> +      Besides the leased file-descriptor, an integer is used to uniquely
> +      identify a Lessee within the tree of DRM masters descending from a single
> +      Owner. Once the Lessee has finished with the resources it had used, the
> +      Lessee ID can be used to revoke that lease.

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

> +      Upon connecting to the compositor all leasable connectors will be
> +      advertised to the client. These connectors are represented by
> +      zwp_kms_lease_connector_v1 interface, and have to be "added" before
> +      creating a lease object.

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

>  This instructs the compositor to use that
> +      connector when creating a lease. The client can receive multiple events
> +      for multiple leasable connectors and needs a way to discern between
> +      various leasable connectors. zwp_kms_lease_connector_v1 provides requests
> +      and events to retrieve additional information specific to that connector
> +      object.

Is it required/preferred to hide the common connector properties behind
requests, or should the connector object send these events
unsolicitedly?

> +
> +      zwp_kms_lease_request_v1::created event will provide a lease object
> +      represented by zwp_kms_lease_v1 interface. The client will then use
> +      this lease object to retrieve the file-descriptor (leased fd) and
> +      use it to perform mode-setting/atomic operations.
> +      Whilst the operation to revoke a lease requires a lesse id, in our case,
> +      the ::revoke request will require the previous obtained lease object to be
> +      used when revoking the lease.

Closing the fd should terminate the lease as well, causing the
compositor to revoke it.

> +
> +      Warning! The protocol described in this file is experimental and
> +      backward incompatible changes may be made. Backward compatible changes
> +      may be added together with the corresponding interface version bump.
> +      Backward incompatible changes are done by bumping the version number in
> +      the protocol and interface names and resetting the interface version.
> +      Once the protocol is to be declared stable, the 'z' prefix and the
> +      version number in the protocol and interface names are removed and the
> +      interface version number is reset.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the manager">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +    <request name="create_lease_req">
> +      <description summary="create a temporary object for managing the lease">
> +        Create an object for temporarily storing all the KMS resources to be leased.
> +      </description>
> +      <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"
> +           summary="the new temporary"/>
> +    </request>
> +
> +    <event name="connector">
> +      <description summary="advertise which connector can be used to request a lease">
> +        This event advertises a leasable connector object. When creating a
> +        lease pass this object to zwp_kms_lease_request_v1::add_connector. As
> +        multiple connectors can leasable (based on compositor policy), the

'can be leasable'?

> +        client can request additional information using
> +        zwp_kms_lease_connector_v1 interface in order to distinguish between
> +        different leasable connectors. After the client has added (using
> +        zwp_kms_lease_request_v1::add_connector) a leasable
> +        connector object, zwp_kms_lease_request_v1::create request should be
> +        called for creating a zwp_kms_lease_v1 lease object.
> +      </description>
> +      <arg name="conn_obj" type="new_id" interface="zwp_kms_lease_connector_v1"
> +      summary="the new temporary" />
> +    </event>
> +
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_v1" version="1">
> +    <description summary="the lease object itself">
> +      This interface represents the lease object and encapsulates the lease
> +      properties. This objected is sent by zwp_kms_lease_request_v1::created
> +      event. Use this object to retrieve lease properties.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the temporary lease request object">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +    <request name="retrieve_leased_fd">
> +      <description summary="request to retrieve info about the lease">
> +        Request to retrieve the leased file-descriptor.
> +      </description>
> +    </request>
> +
> +    <event name="leased_fd">
> +      <description summary="returns information about the lease">
> +        This event returns the leased fd which is required for modesetting
> +        or querying page flips/atomic modesetting.
> +        The client can use the leased fd to find out DRM-related information
> +        like connector ID, CRTC ID, and plane ID using drmModeGetLease().
> +        Using that information it can derive a suitable mode useful
> +        when performing a modeset.
> +      </description>
> +      <arg name="leased_fd" type="fd" summary="leased fd" />
> +    </event>
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_connector_v1" version="1">
> +    <description summary="zwp_kms_lease_connector_v1">
> +      This interface describes a connector object advertised by
> +      zwp_kms_lease_manager_v1::connector. The client can distinguish between
> +      different leasable connectors by requesting additional information for
> +      that connector.
> +    </description>
> +
> +    <request name="retrieve_name">
> +      <description summary="name">
> +         Request to retrieve connector output name on which the leasable
> +         connector object is based on.
> +      </description>
> +    </request>

Is this necessary? It seems to me that the name event could just be
sent without request to new listeners.

> +    <event name="name">
> +      <description summary="name">
> +         Event sent when requesting connector output name.
> +      </description>
> +      <arg name="conn_name" type="string" summary="connector name" />
> +    </event>
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_request_v1" version="1">
> +    <description summary="lease request object">
> +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> +      to create a zwp_kms_lease_v1 object (the actual lease object), to revoke
> +      it, and to specify from which connector the lease should be created.

I'm not sure about the revoking part, actually.

I expected this to handle more like zwp_linux_buffer_params_v1, as a
single-use object to create a zwp_kms_lease_v1 once (after adding
connectors and, possibly later, planes). After successful lease
creation (or failure) this could be destroyed.

Since the zwp_kms_lease_v1 itself has a destructor, and the actual
lease can be terminated by closing the fd anyway, I see no reason to
keep the original zwp_kms_lease_request_v1 around.

> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the lease request object">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +  <request name="add_connector">
> +     <description summary="add a leasable connector object">
> +       This request is used to create the lease object using the leasable
> +       connector object, and must be called before zwp_kms_lease_request_v1::create.
> +     </description>
> +     <arg name="conn_obj" type="object" interface="zwp_kms_lease_connector_v1"
> +     summary="a leasable connector added to the lease"/>
> +  </request>
> +
> +  <request name="create">
> +     <description summary="create the lease">
> +       This request will create a zwp_kms_lease_v1 object based on
> +       zwp_kms_lease_connector_v1 that was added using
> +       zwp_kms_lease_request_v1::add_connector.
> +     </description>
> +  </request>
> +
> +  <request name="revoke">
> +    <description summary="revoke">
> +      Revoke the lease, using the zwp_kms_lease_v1 objected received in
> +      zwp_kms_lease_request_v1::created event.
> +    </description>
> +    <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1"
> +    summary="lease object to remove" />
> +  </request>

See above, I think this is something the client shouldn't have to do.

> +  <event name="created">
> +    <description summary="lease created successfully">
> +      This event indicates that the lease has been created. It provides a
> +      zwp_kms_lease_v1 object used for retrieving the file-descriptor
> +      representing the lease.
> +    </description>
> +    <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1"
> +    summary="the lease obj"/>
> +  </event>
> +
> +  <event name="failed">
> +    <description summary="lease could not be created">
> +      This event indicates that the lease could not be created/revoked.
> +    </description>
> +  </event>
> +
> +  <event name="revoked">
> +    <description summary="lease revoked successfully">
> +      This event indicates that the lease has been revoked.
> +    </description>
> +  </event>

Should this be moved to the zwp_kms_lease_v1?

> +  <event name="connector_add_failed">
> +    <description summary="failed to add connector">
> +      This event indicates that the leasable connector object specified in
> +      zwp_kms_lease_request_v1::add_connector couldn't be added.
> +    </description>
> +  </event>
> +
> +  <event name="connector_added">
> +    <description summary="connector was added successfully">
> +      This event indicates that the leasable connector object specified in
> +      zwp_kms_lease_request_v1::add_connector has been added successfully.
> +    </description>
> +  </event>
> +
> +  </interface>
> +</protocol>

regards
Philipp
Marius-cristian Vlad Aug. 30, 2018, 11:01 a.m.
Excuse my MUA, but I have no other means to reply atm.

-----Original Message-----
From: Philipp Zabel <philipp.zabel@gmail.com> 

Sent: Thursday, August 30, 2018 10:03 AM
To: Marius-cristian Vlad <marius-cristian.vlad@nxp.com>; wayland-devel@lists.freedesktop.org
Cc: keithp@keithp.com; eucan@de.adit-jv.com; daniel@fooishbar.org; ppaalanen@gmail.com; sardemff7+wayland@sardemff7.net; p.zabel@pengutronix.de
Subject: Re: [PATCH wayland-protocols v4] unstable/drm-lease: DRM lease protocol support

Hi Marius,

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

mvlad: The more comments the better I say ūüėä.

Am Mittwoch, den 29.08.2018, 14:00 +0300 schrieb Marius Vlad:
> Simple protocol extension to manage DRM lease. Based on the work by 

> Keith Packard in [1], respectively [2].

> 

> [1] 

> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgi

> t.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc4171535389d72e9135c

> 9615cecd07b346fd6d7e&amp;data=02%7C01%7Cmarius-cristian.vlad%40nxp.com

> %7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635

> %7C0%7C0%7C636712093917097791&amp;sdata=5xcj%2BmYAkgmahSPWIQOWgtjLpaOy

> jEzqAVEINvanfrc%3D&amp;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&amp;data=02%7C01%7Cmarius-cristian.vlad%40nxp.com%7Ceba3e65e2d19

> 47d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6367

> 12093917097791&amp;sdata=s%2FUhj6o7JUe7Is1GO3NZUphEwXg1BJeGAvVhVbr9gaU

> %3D&amp;reserved=0

> 

> Changes since v3:

> - instead of advertising all leasable connectors at once, advertise 

> each connector for each leasable output.  Use an object to represent a 

> leasable connector and provide a request to add that leasable 

> connector when creating the lease (Philipp Zabel)

> - add two events to handle add_connector request

> - add some comments in the manager interface to explain how the 

> protocol can/should be used

> 

> Changes since v2:

> - advertise connectors when creating a lease request object (Pekka 

> Paalanen)

> - when revoking the lease use the lease object instead of lessee id 

> (Pekka Paalanen)

> - various grammar/spelling fixes (Pekka Paalanen)

> 

> Changes since v1:

> - added manager: advertise lease capability and manage the lease 

> (Daniel Stone)

> - add request(s) for adding connector/crtc/plane to behave more like 

> dmabuf (Daniel Stone)

> 

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

> ---

> A (rather incomplete) implementation for this version can be found at

> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit

> lab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Ftree%2Fdrm-lease-adverti

> se-each-connnector-object&amp;data=02%7C01%7Cmarius-cristian.vlad%40nx

> p.com%7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c3

> 01635%7C0%7C0%7C636712093917097791&amp;sdata=G4kPsx742yO4%2FY5swFgVLSk

> AMk5%2Fa9gnd7Z7%2BLSSgKU%3D&amp;reserved=0

> 

> One interesting question is how to handle the situation when the 

> client deliberately, or not, holds the lease indefinitely.


The client implicitly cancels the lease by closing the lease fd (or by having it closed automatically when it is terminated). The compositor will get a hotplug event then, and it has to check the lessee list happens and revoke leases to lost lessees.

Mvlad: That's what I expected to see, but I did some brief tests here, and it could the our lease kernel version could a little bit older, but when SIGKILL'ing the client, the DRM subsystem doesn't revoke the lease, or calls drm_lease_destroy. I suppose the following happens: when creating the lease the compositor will hold a reference of the leased fd -- well technically the DRM leases part does -- (and ofc I can't close it then because the client won't be able to use it), we pass that thru weston (over the unix socket), the client will also hold a reference for that leased fd, so basically the lease will only be destroyed when that ref counter hits 0. In the implementation I close the leased fd deliberately on the compositor side, when I get the revoke request from the client. There's no issue when the client behaves, but only when the client dies abruptly, and it won't notify the compositor that it is no longer using the lease. I've only seen this once when you brought it up, so I need to test it further to be sure about this.

>  This has multiple

> ramification like what happens when the client un-expectingly dies and 

> doesn't revoking the lease,


See above, a lease terminates when its last fd is closed.

Mvlad: I'm not really sure on this, see my above comment. 

>  or when hot-plugging the connector and weston tries to get a hold of 

> a connector previously leased?


I'd say hot-unplugging a connector that is part of a lease should cause this lease to be canceled.

mvlad: Which means that the client would have to be notified when that happens, so it can shut down cleanly. 
Somehow the client (in its rendering part perhaps) has to check if the lease is still valid. Guess we can also
verify errno, so it should not complicate the client that much.
 

>  It could be there might be other

> situations where the compositor will need to revoke the lease. 


The VT switch case, for example. If the compositor has to give up drm_master temporarily, it will have to revoke all leases first.

Mvlad: Yep. 

> In the implementation we could deny the compositor to get a hold of 

> the connector when hot-plugging that output, if that's desirable, and 

> presumably establish a way to detect periodically if the lease is still in use.

>

> Alternatively, shouldn't we use something like a ping/pong approach in 

> which the client (in rendering part), sends periodically an alive signal?


I don't think the compositor should care what the lessee is doing with its lease, only whether it is still holding the fd.

Mvlad: Indeed. I believe we have some methods exposed to verify what current leases are being in use, question 
Is when we do this. There are some options, but will the need to see which are the best.   

>  Makefile.am                                  |   1 +

>  unstable/drm-lease/README                    |   4 +

>  unstable/drm-lease/drm-lease-unstable-v1.xml | 244 

> +++++++++++++++++++++++++++

>  3 files changed, 249 insertions(+)

>  create mode 100644 unstable/drm-lease/README  create mode 100644 

> unstable/drm-lease/drm-lease-unstable-v1.xml

> 

> diff --git a/Makefile.am b/Makefile.am index 6394e26..5d13dca 100644

> --- a/Makefile.am

> +++ b/Makefile.am

> @@ -4,6 +4,7 @@ unstable_protocols =								\

>  	unstable/pointer-gestures/pointer-gestures-unstable-v1.xml		\

>  	unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml		\

>  	unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml			\

> +	unstable/drm-lease/drm-lease-unstable-v1.xml				\

>  	unstable/text-input/text-input-unstable-v1.xml				\

>  	unstable/text-input/text-input-unstable-v3.xml				\

>  	unstable/input-method/input-method-unstable-v1.xml			\

> diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README new 

> file mode 100644 index 0000000..a25600c

> --- /dev/null

> +++ b/unstable/drm-lease/README

> @@ -0,0 +1,4 @@

> +Linux DRM lease

> +

> +Maintainers:

> +Marius Vlad <marius-cristian.vlad@nxp.com>

> diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml 

> b/unstable/drm-lease/drm-lease-unstable-v1.xml

> new file mode 100644

> index 0000000..40eedb4

> --- /dev/null

> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml

> @@ -0,0 +1,244 @@

> +<?xml version="1.0" encoding="UTF-8"?> <protocol 

> +name="drm_lease_unstable_v1">

> +

> +<copyright>

> +    Copyright 2018 NXP

> +

> +    Permission is hereby granted, free of charge, to any person obtaining a

> +    copy of this software and associated documentation files (the "Software"),

> +    to deal in the Software without restriction, including without limitation

> +    the rights to use, copy, modify, merge, publish, distribute, sublicense,

> +    and/or sell copies of the Software, and to permit persons to whom the

> +    Software is furnished to do so, subject to the following conditions:

> +

> +    The above copyright notice and this permission notice (including the next

> +    paragraph) shall be included in all copies or substantial portions of the

> +    Software.

> +

> +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

> +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

> +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL

> +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

> +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING

> +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER

> +    DEALINGS IN THE SOFTWARE.

> +</copyright>

> +

> +  <interface name="zwp_kms_lease_manager_v1" version="1">

> +    <description summary="lease manager">

> +      This interface makes use of DRM lease written by Keith Packard.

> +

> +      A DRM master can create another DRM master and ``lease'' resources it has

> +      control over to the new DRM master. Once leased, resources can not be

> +      controlled by the owner unless the owner cancels the lease, or 

> + the new


Should this say 'unless the owner revokes the lease'? 

Mvlad: This was taken ad litteram from Keiths' description, but I guess he meant revoke. 


> +      DRM master is closed.

> +

> +      A lease is a contract between the Lessor (DRM master which has leased out

> +      resources to one or more other DRM masters) and a Lessee

> +      (DRM master which controls resources leased from another DRM master). This

> +      contract specifies which resources may be controlled by the Lessee.

> +

> +      The Lessee can issue modesetting/page-flipping atomic operations etc.,

> +      just like a Lessor using the leased file-descriptor passed by the Lessor.

> +

> +      Besides the leased file-descriptor, an integer is used to uniquely

> +      identify a Lessee within the tree of DRM masters descending from a single

> +      Owner. Once the Lessee has finished with the resources it had used, the

> +      Lessee ID can be used to revoke that lease.


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

Mvlad: Right, have to check that. 

> +      Upon connecting to the compositor all leasable connectors will be

> +      advertised to the client. These connectors are represented by

> +      zwp_kms_lease_connector_v1 interface, and have to be "added" before

> +      creating a lease object.


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

Mvlad: Sure do.

>  This instructs the compositor to use that

> +      connector when creating a lease. The client can receive multiple events

> +      for multiple leasable connectors and needs a way to discern between

> +      various leasable connectors. zwp_kms_lease_connector_v1 provides requests

> +      and events to retrieve additional information specific to that connector

> +      object.


Is it required/preferred to hide the common connector properties behind requests, or should the connector object send these events unsolicitedly?

Mvlad: This might even simplify client code even further. I'll modify client code to see, but I don't know at the moment.

> +

> +      zwp_kms_lease_request_v1::created event will provide a lease object

> +      represented by zwp_kms_lease_v1 interface. The client will then use

> +      this lease object to retrieve the file-descriptor (leased fd) and

> +      use it to perform mode-setting/atomic operations.

> +      Whilst the operation to revoke a lease requires a lesse id, in our case,

> +      the ::revoke request will require the previous obtained lease object to be

> +      used when revoking the lease.


Closing the fd should terminate the lease as well, causing the compositor to revoke it.

Mvlad: I believe the compositor will still (hold) a ref on the leased fd. See my initial comment.  

> +

> +      Warning! The protocol described in this file is experimental and

> +      backward incompatible changes may be made. Backward compatible changes

> +      may be added together with the corresponding interface version bump.

> +      Backward incompatible changes are done by bumping the version number in

> +      the protocol and interface names and resetting the interface version.

> +      Once the protocol is to be declared stable, the 'z' prefix and the

> +      version number in the protocol and interface names are removed and the

> +      interface version number is reset.

> +    </description>

> +

> +    <request name="destroy" type="destructor">

> +      <description summary="destroys the manager">

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

> +        interface.

> +      </description>

> +    </request>

> +

> +    <request name="create_lease_req">

> +      <description summary="create a temporary object for managing the lease">

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

> +      </description>

> +      <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"

> +           summary="the new temporary"/>

> +    </request>

> +

> +    <event name="connector">

> +      <description summary="advertise which connector can be used to request a lease">

> +        This event advertises a leasable connector object. When creating a

> +        lease pass this object to zwp_kms_lease_request_v1::add_connector. As

> +        multiple connectors can leasable (based on compositor 

> + policy), the


'can be leasable'?

Mvlad: typo ūüėä


> +        client can request additional information using

> +        zwp_kms_lease_connector_v1 interface in order to distinguish between

> +        different leasable connectors. After the client has added (using

> +        zwp_kms_lease_request_v1::add_connector) a leasable

> +        connector object, zwp_kms_lease_request_v1::create request should be

> +        called for creating a zwp_kms_lease_v1 lease object.

> +      </description>

> +      <arg name="conn_obj" type="new_id" interface="zwp_kms_lease_connector_v1"

> +      summary="the new temporary" />

> +    </event>

> +

> +  </interface>

> +

> +  <interface name="zwp_kms_lease_v1" version="1">

> +    <description summary="the lease object itself">

> +      This interface represents the lease object and encapsulates the lease

> +      properties. This objected is sent by zwp_kms_lease_request_v1::created

> +      event. Use this object to retrieve lease properties.

> +    </description>

> +

> +    <request name="destroy" type="destructor">

> +      <description summary="destroys the temporary lease request object">

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

> +        interface.

> +      </description>

> +    </request>

> +

> +    <request name="retrieve_leased_fd">

> +      <description summary="request to retrieve info about the lease">

> +        Request to retrieve the leased file-descriptor.

> +      </description>

> +    </request>

> +

> +    <event name="leased_fd">

> +      <description summary="returns information about the lease">

> +        This event returns the leased fd which is required for modesetting

> +        or querying page flips/atomic modesetting.

> +        The client can use the leased fd to find out DRM-related information

> +        like connector ID, CRTC ID, and plane ID using drmModeGetLease().

> +        Using that information it can derive a suitable mode useful

> +        when performing a modeset.

> +      </description>

> +      <arg name="leased_fd" type="fd" summary="leased fd" />

> +    </event>

> +  </interface>

> +

> +  <interface name="zwp_kms_lease_connector_v1" version="1">

> +    <description summary="zwp_kms_lease_connector_v1">

> +      This interface describes a connector object advertised by

> +      zwp_kms_lease_manager_v1::connector. The client can distinguish between

> +      different leasable connectors by requesting additional information for

> +      that connector.

> +    </description>

> +

> +    <request name="retrieve_name">

> +      <description summary="name">

> +         Request to retrieve connector output name on which the leasable

> +         connector object is based on.

> +      </description>

> +    </request>


Is this necessary? It seems to me that the name event could just be sent without request to new listeners.

Mvlad: I'll give it a try and see how this works. 

> +    <event name="name">

> +      <description summary="name">

> +         Event sent when requesting connector output name.

> +      </description>

> +      <arg name="conn_name" type="string" summary="connector name" />

> +    </event>

> +  </interface>

> +

> +  <interface name="zwp_kms_lease_request_v1" version="1">

> +    <description summary="lease request object">

> +      This interface is used for managing zwp_kms_lease_v1 object. It is used

> +      to create a zwp_kms_lease_v1 object (the actual lease object), to revoke

> +      it, and to specify from which connector the lease should be created.


I'm not sure about the revoking part, actually.

I expected this to handle more like zwp_linux_buffer_params_v1, as a single-use object to create a zwp_kms_lease_v1 once (after adding connectors and, possibly later, planes). After successful lease creation (or failure) this could be destroyed.

Since the zwp_kms_lease_v1 itself has a destructor, and the actual lease can be terminated by closing the fd anyway, I see no reason to keep the original zwp_kms_lease_request_v1 around.

Mvlad: This relates also the above comments as well. Need to verify if indeed it works like this. But a follow up question here regarding planes: 
We've discussed this quite extensively, and I've concluded that it is much easier to gather the objects ids required to create the lease, directly
In the compositor as it already has that information. I kind of assume that the connector object will also provide the CRTC and implicitly the planes it can drive. Given that, when extending the protocol the connector object will need to inform the client which planes can be leased. Is that correct?  


> +    </description>

> +

> +    <request name="destroy" type="destructor">

> +      <description summary="destroys the lease request object">

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

> +        interface.

> +      </description>

> +    </request>

> +

> +  <request name="add_connector">

> +     <description summary="add a leasable connector object">

> +       This request is used to create the lease object using the leasable

> +       connector object, and must be called before zwp_kms_lease_request_v1::create.

> +     </description>

> +     <arg name="conn_obj" type="object" interface="zwp_kms_lease_connector_v1"

> +     summary="a leasable connector added to the lease"/>  </request>

> +

> +  <request name="create">

> +     <description summary="create the lease">

> +       This request will create a zwp_kms_lease_v1 object based on

> +       zwp_kms_lease_connector_v1 that was added using

> +       zwp_kms_lease_request_v1::add_connector.

> +     </description>

> +  </request>

> +

> +  <request name="revoke">

> +    <description summary="revoke">

> +      Revoke the lease, using the zwp_kms_lease_v1 objected received in

> +      zwp_kms_lease_request_v1::created event.

> +    </description>

> +    <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1"

> +    summary="lease object to remove" />  </request>


See above, I think this is something the client shouldn't have to do.

Mvlad: Need to verify that.

> +  <event name="created">

> +    <description summary="lease created successfully">

> +      This event indicates that the lease has been created. It provides a

> +      zwp_kms_lease_v1 object used for retrieving the file-descriptor

> +      representing the lease.

> +    </description>

> +    <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1"

> +    summary="the lease obj"/>

> +  </event>

> +

> +  <event name="failed">

> +    <description summary="lease could not be created">

> +      This event indicates that the lease could not be created/revoked.

> +    </description>

> +  </event>

> +

> +  <event name="revoked">

> +    <description summary="lease revoked successfully">

> +      This event indicates that the lease has been revoked.

> +    </description>

> +  </event>


Should this be moved to the zwp_kms_lease_v1?

Mvlad: Kept it here because of the ::revoke request. If there's no need for revoke request, yes, it would make more
sense to have it zwp_kms_lease_v1 interface.  

> +  <event name="connector_add_failed">

> +    <description summary="failed to add connector">

> +      This event indicates that the leasable connector object specified in

> +      zwp_kms_lease_request_v1::add_connector couldn't be added.

> +    </description>

> +  </event>

> +

> +  <event name="connector_added">

> +    <description summary="connector was added successfully">

> +      This event indicates that the leasable connector object specified in

> +      zwp_kms_lease_request_v1::add_connector has been added successfully.

> +    </description>

> +  </event>

> +

> +  </interface>

> +</protocol>


regards
Philipp
Philipp Zabel Aug. 30, 2018, 12:38 p.m.
On Thu, 2018-08-30 at 11:01 +0000, Marius-cristian Vlad wrote:
[...]
> > 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 

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

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

> >  This has multiple
> > ramification like what happens when the client un-expectingly dies and 
> > doesn't revoking the lease,
> 
> See above, a lease terminates when its last fd is closed.
> 
> Mvlad: I'm not really sure on this, see my above comment. 
> 
> >  or when hot-plugging the connector and weston tries to get a hold of 
> > a connector previously leased?
> 
> I'd say hot-unplugging a connector that is part of a lease should cause this lease to be canceled.
> 
> mvlad: Which means that the client would have to be notified when that happens, so it can shut down cleanly. 

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 Aug. 31, 2018, 10:09 a.m.
On Thu, 2018-08-30 at 14:38 +0200, Philipp Zabel wrote:
> On Thu, 2018-08-30 at 11:01 +0000, Marius-cristian Vlad wrote:

> [...]

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

> 

> 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

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

> 

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

> 

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


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?

> > 

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

> 

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

> 

> [...]

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


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

Will send an update soon. 

> 

> regards

> Philipp