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

Submitted by Marius-cristian Vlad on Sept. 4, 2018, 2:39 p.m.

Details

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

Commit Message

Marius-cristian Vlad Sept. 4, 2018, 2:39 p.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 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-cristian.vlad@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

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

Comments

Philipp Zabel Sept. 5, 2018, 6:55 a.m.
Hi Marius,

thank you for the update!

Am Dienstag, den 04.09.2018, 17:39 +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 v4:
[...]
> - 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.

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

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.

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

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

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

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

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

> +        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"?

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

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

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

> +  </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 Sept. 6, 2018, 10:45 a.m.

Marius-cristian Vlad Sept. 6, 2018, 11:29 a.m.
[resending... maybe this time works]

On Wed, 2018-09-05 at 08:55 +0200, Philipp Zabel wrote:
> Hi Marius,

> 

> thank you for the update!

Thank you for taking the time to look over this.

> 

> Am Dienstag, den 04.09.2018, 17:39 +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%2

> > F%2Fcgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc417153538

> > 9d72e9135c9615cecd07b346fd6d7e&amp;data=02%7C01%7Cmarius-

> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1

> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&amp;sdata=p

> > qLcIrKTlgSJ0fVyfdXOXO4Re%2BV6OrNQGccIhMSn0Os%3D&amp;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&amp;data=02%7C0

> > 1%7Cmarius-

> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1

> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&amp;sdata=A

> > 3EfXGUUJMl%2FFQHw8LVHPn9Ptu%2BXKAr90EIwnHpLhzw%3D&amp;reserved=0

> > 

> > Changes since v4:

> 

> [...]

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

> 

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

> > ---

> > 

> > 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&amp;data=02%7C01%7Cmarius-

> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1

> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&amp;sdata=M

> > UrWEApwiGgSvcz8yKiT9Tm8Ij3UpGs6Ai5ioTtuWDI%3D&amp;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

> lease a disabled monitor with:

> 

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

> 

> [...]

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

> 

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

> 

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


Yep, I'll add that.

> 

> > +

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

> 

> Maybe "create a temporary object for managing a lease"?

> There could be multiple leases.

> 

Okay will modify.

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

> 

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

> 

> > +

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

> 

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

> 

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

> 

> regards

> Philipp

>
Philipp Zabel Sept. 6, 2018, 1:36 p.m.
Hi Marius,

On Thu, 2018-09-06 at 11:29 +0000, Marius-cristian Vlad wrote:
> > [...]
> > > - 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.

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

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

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

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

It would be great to add a note that these don't have to be waited for,
i.e. that it is possible to just call:

  -> "add_connector"
  -> "add_connector"
  -> "create"

without inserting round-trips to wait for these events in-between.

regards
Philipp