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

Submitted by Marius-cristian Vlad on Aug. 20, 2018, 8 p.m.

Details

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

Commit Message

Marius-cristian Vlad Aug. 20, 2018, 8 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

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

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

Changes since v1:
- added manager: advertise lease capability and manage the lease (Daniel Stone)
- add request(s) for adding connector/crtc/plane to behave more like dmabuf (Daniel Stone)
---
Some caveats while at it. This is just in RFC form adapted from the comments
I've  mostly got from Pekka Paalanen. It most certainly doesn't address all the issues
brought up: like multi-node card environments/system, doing a TEST_ONLY
commit before giving out a lease, or takes hot-plugging into consideration. 
As I was side-tracked to other things and
being on hiatus for a while, I wanted first to get some impressions first if
indeed this is the right approach and do some incremental updates afterwards.

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

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

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

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

 Makefile.am                                  |   1 +
 unstable/drm-lease/README                    |   4 +
 unstable/drm-lease/drm-lease-unstable-v1.xml | 173 +++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 unstable/drm-lease/README
 create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml

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..6cb3c0a
--- /dev/null
+++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
@@ -0,0 +1,173 @@ 
+<?xml version="1.0" encoding="UTF-8"?>
+<protocol name="drm_lease_unstable_v1">
+
+<copyright>
+    Copyright 2018 NXP
+
+    Permission is hereby granted, free of charge, to any person obtaining a
+    copy of this software and associated documentation files (the "Software"),
+    to deal in the Software without restriction, including without limitation
+    the rights to use, copy, modify, merge, publish, distribute, sublicense,
+    and/or sell copies of the Software, and to permit persons to whom the
+    Software is furnished to do so, subject to the following conditions:
+
+    The above copyright notice and this permission notice (including the next
+    paragraph) shall be included in all copies or substantial portions of the
+    Software.
+
+    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+    DEALINGS IN THE SOFTWARE.
+</copyright>
+
+  <interface name="zwp_kms_lease_manager_v1" version="1">
+    <description summary="lease manager">
+      This interface makes use of DRM lease written by Keith Packard.
+
+      A DRM master can create another DRM master and ``lease'' resources it has
+      control over to the new DRM master. Once leased, resources can not be
+      controlled by the owner unless the owner cancels the lease, or the new
+      DRM master is closed.
+
+      A lease is a contract between the Lessor (DRM master which has leased out
+      resources to one or more other DRM masters) and a Lessee
+      (DRM master which controls resources leased from another DRM master). This
+      contract specifies which resources may be controlled by the Lessee.
+
+      The Lessee can issue modesetting/page-flipping atomic operations etc.,
+      just like a Lessor using the leased file-descriptor passed by the Lessor.
+
+      Besides the leased file-descriptor, an integer is used to uniquely
+      identify a Lessee within the tree of DRM masters descending from a single
+      Owner. Once the Lessee has finished with the resources it had used, the
+      Lessee ID can be used to revoke that lease.
+
+      Warning! The protocol described in this file is experimental and
+      backward incompatible changes may be made. Backward compatible changes
+      may be added together with the corresponding interface version bump.
+      Backward incompatible changes are done by bumping the version number in
+      the protocol and interface names and resetting the interface version.
+      Once the protocol is to be declared stable, the 'z' prefix and the
+      version number in the protocol and interface names are removed and the
+      interface version number is reset.
+    </description>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroys the manager">
+        This has no effect on any remaining objects created through this
+        interface.
+      </description>
+    </request>
+
+    <request name="create_lease_req">
+      <description summary="create a temporary object for managing the lease">
+        Create an object for temporarily storing all the KMS resources to be leased.
+      </description>
+      <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"
+           summary="the new temporary"/>
+    </request>
+
+    <event name="connectors">
+      <description summary="advertise which connectors can be used to request creation for a lease">
+        This event advertises leasable connectors. When multiple connectors are
+        advertised, the client should properly parse and choose one of connectors
+        A client trying to create a lease using zwp_kms_lease_request_v1::create
+        request would use this connector name.
+      </description>
+      <arg name="connectors" type="string" summary="list of connector entries, split by '|' char"/>
+      <arg name="connectors_entries" type="uint" summary="number of connectors found in connectors string"/>
+    </event>
+
+  </interface>
+
+  <interface name="zwp_kms_lease_v1" version="1">
+    <description summary="the lease object itself">
+      This interface represents the lease object and encapsulates the lease
+      properties. This objected is sent by zwp_kms_lease_request_v1::created
+      event. Use this object to retrieve lease properties.
+    </description>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroys the temporary lease request object">
+        This has no effect on any remaining objects created through this
+        interface.
+      </description>
+    </request>
+
+    <request name="get_lease_info">
+      <description summary="request to retrieve info about the lease">
+        Request to retrieve lease information, like leased fd, monitor
+        and connector name.
+      </description>
+    </request>
+
+    <event name="lease_info">
+      <description summary="returns information about the lease">
+        This event returns (among other information about the connector leased)
+        the leased fd which is required for modesetting or queying page flips.
+        The client can use the leased fd to find out DRM-related information
+        like connector, CRTC, and plane ID using drmModeGetLease(). Using that
+        information it can derive a suitable mode useful when performing modeset.
+      </description>
+      <arg name="leased_fd" type="fd" summary="leased fd" />
+      <arg name="conn_name" type="string" summary="connector name of the lease" />
+      <arg name="monitor_name" type="string" summary="monitor name" />
+    </event>
+  </interface>
+
+  <interface name="zwp_kms_lease_request_v1" version="1">
+    <description summary="lease request object">
+      This interface is used for managing zwp_kms_lease_v1 object. It is used
+      to create a zwp_kms_lease_v1 object (the actual lease object) and also to
+      revoke the lease.
+    </description>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroys the lease request object">
+        This has no effect on any remaining objects created through this
+        interface.
+      </description>
+    </request>
+
+  <request name="create">
+    <description summary="create">
+      Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
+      Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
+    </description>
+    <arg name="connector" type="string" summary="connector output name"/>
+  </request>
+
+  <request name="revoke">
+    <description summary="revoke">
+      Revoke the lease, using the zwp_kms_lease_v1 objected received in
+      zwp_kms_lease_request_v1::created event.
+    </description>
+    <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1" summary="lease object to remove" />
+  </request>
+
+  <event name="created">
+    <description summary="lease created successfully">
+    This event indicates that the lease has been created. It provides a zwp_kms_lease_v1 object
+    used for retrieving the fd representing the lease.
+    </description>
+    <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1" summary="the lease obj"/>
+  </event>
+
+  <event name="failed">
+    <description summary="lease could not be created">
+      This event indicates that the lease could not be created/revoked.
+    </description>
+  </event>
+
+  <event name="revoked">
+    <description summary="lease revoked successfully">
+      This event indicates that the lease has been revoked.
+    </description>
+  </event>
+
+  </interface>
+</protocol>

Comments

Philipp Zabel Aug. 22, 2018, 7:17 a.m.
Hi Marius,

On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
> Simple protocol extension to manage DRM lease. Based on the work by Keith
> Packard in [1], respectively [2].
> 
> [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f
> 
> Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>
> 
> Changes since v2:
> - advertise connector names when creating a lease request object (Pekka Paalanen)
> - when revoking the lease use the lease object instead of lessee id (Pekka Paalanen)
> - various grammar/spelling fixes (Pekka Paalanen)
> 
> Changes since v1:
> - added manager: advertise lease capability and manage the lease (Daniel Stone)
> - add request(s) for adding connector/crtc/plane to behave more like dmabuf (Daniel Stone)
> ---
> Some caveats while at it. This is just in RFC form adapted from the comments
> I've  mostly got from Pekka Paalanen. It most certainly doesn't address all the issues
> brought up: like multi-node card environments/system, doing a TEST_ONLY
> commit before giving out a lease, or takes hot-plugging into consideration. 
> As I was side-tracked to other things and
> being on hiatus for a while, I wanted first to get some impressions first if
> indeed this is the right approach and do some incremental updates afterwards.
> 
> The are some issues which I'd like to point out from the beginning, which
> require some further comments. In no particular order:
> 
> I've found that I can't pass a wl_array as a way to advertise various
> information to the client. (wl_array works fine with integers not with
> allocated data). It would probably be better to advertise also
> monitor name, other EDID info or available modes, but at the moment I only 
> join-split a delimiter between connector names and send it out as a string.  
> While it is ugly I'm not aware of a way to send this information back to the
> client in the form of a list. 
>
> The client is aware before hand of this delimiter and has the number of entries:
> can easily choose or pick one of the connectors. It could be that there are
> some other ways this can be achieved, I welcome any kind of feedback here.

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

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

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

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

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

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

>  Makefile.am                                  |   1 +
>  unstable/drm-lease/README                    |   4 +
>  unstable/drm-lease/drm-lease-unstable-v1.xml | 173 +++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 unstable/drm-lease/README
>  create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6394e26..5d13dca 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -4,6 +4,7 @@ unstable_protocols =								\
>  	unstable/pointer-gestures/pointer-gestures-unstable-v1.xml		\
>  	unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml		\
>  	unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml			\
> +	unstable/drm-lease/drm-lease-unstable-v1.xml				\
>  	unstable/text-input/text-input-unstable-v1.xml				\
>  	unstable/text-input/text-input-unstable-v3.xml				\
>  	unstable/input-method/input-method-unstable-v1.xml			\
> diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
> new file mode 100644
> index 0000000..a25600c
> --- /dev/null
> +++ b/unstable/drm-lease/README
> @@ -0,0 +1,4 @@
> +Linux DRM lease
> +
> +Maintainers:
> +Marius Vlad <marius-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..6cb3c0a
> --- /dev/null
> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> @@ -0,0 +1,173 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="drm_lease_unstable_v1">
> +
> +<copyright>
> +    Copyright 2018 NXP
> +
> +    Permission is hereby granted, free of charge, to any person obtaining a
> +    copy of this software and associated documentation files (the "Software"),
> +    to deal in the Software without restriction, including without limitation
> +    the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +    and/or sell copies of the Software, and to permit persons to whom the
> +    Software is furnished to do so, subject to the following conditions:
> +
> +    The above copyright notice and this permission notice (including the next
> +    paragraph) shall be included in all copies or substantial portions of the
> +    Software.
> +
> +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +    DEALINGS IN THE SOFTWARE.
> +</copyright>
> +
> +  <interface name="zwp_kms_lease_manager_v1" version="1">
> +    <description summary="lease manager">
> +      This interface makes use of DRM lease written by Keith Packard.
> +
> +      A DRM master can create another DRM master and ``lease'' resources it has
> +      control over to the new DRM master. Once leased, resources can not be
> +      controlled by the owner unless the owner cancels the lease, or the new
> +      DRM master is closed.
> +
> +      A lease is a contract between the Lessor (DRM master which has leased out
> +      resources to one or more other DRM masters) and a Lessee
> +      (DRM master which controls resources leased from another DRM master). This
> +      contract specifies which resources may be controlled by the Lessee.
> +
> +      The Lessee can issue modesetting/page-flipping atomic operations etc.,
> +      just like a Lessor using the leased file-descriptor passed by the Lessor.
> +
> +      Besides the leased file-descriptor, an integer is used to uniquely
> +      identify a Lessee within the tree of DRM masters descending from a single
> +      Owner. Once the Lessee has finished with the resources it had used, the
> +      Lessee ID can be used to revoke that lease.
> +
> +      Warning! The protocol described in this file is experimental and
> +      backward incompatible changes may be made. Backward compatible changes
> +      may be added together with the corresponding interface version bump.
> +      Backward incompatible changes are done by bumping the version number in
> +      the protocol and interface names and resetting the interface version.
> +      Once the protocol is to be declared stable, the 'z' prefix and the
> +      version number in the protocol and interface names are removed and the
> +      interface version number is reset.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the manager">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +    <request name="create_lease_req">
> +      <description summary="create a temporary object for managing the lease">
> +        Create an object for temporarily storing all the KMS resources to be leased.
> +      </description>
> +      <arg name="params_id" type="new_id" interface="zwp_kms_lease_request_v1"
> +           summary="the new temporary"/>
> +    </request>
> +
> +    <event name="connectors">
> +      <description summary="advertise which connectors can be used to request creation for a lease">
> +        This event advertises leasable connectors. When multiple connectors are
> +        advertised, the client should properly parse and choose one of connectors
> +        A client trying to create a lease using zwp_kms_lease_request_v1::create
> +        request would use this connector name.
> +      </description>
> +      <arg name="connectors" type="string" summary="list of connector entries, split by '|' char"/>
> +      <arg name="connectors_entries" type="uint" summary="number of connectors found in connectors string"/>
> +    </event>
> +
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_v1" version="1">
> +    <description summary="the lease object itself">
> +      This interface represents the lease object and encapsulates the lease
> +      properties. This objected is sent by zwp_kms_lease_request_v1::created
> +      event. Use this object to retrieve lease properties.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the temporary lease request object">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +    <request name="get_lease_info">
> +      <description summary="request to retrieve info about the lease">
> +        Request to retrieve lease information, like leased fd, monitor
> +        and connector name.
> +      </description>
> +    </request>
> +
> +    <event name="lease_info">
> +      <description summary="returns information about the lease">
> +        This event returns (among other information about the connector leased)
> +        the leased fd which is required for modesetting or queying page flips.
> +        The client can use the leased fd to find out DRM-related information
> +        like connector, CRTC, and plane ID using drmModeGetLease(). Using that
> +        information it can derive a suitable mode useful when performing modeset.
> +      </description>
> +      <arg name="leased_fd" type="fd" summary="leased fd" />
> +      <arg name="conn_name" type="string" summary="connector name of the lease" />
> +      <arg name="monitor_name" type="string" summary="monitor name" />
> +    </event>
> +  </interface>
> +
> +  <interface name="zwp_kms_lease_request_v1" version="1">
> +    <description summary="lease request object">
> +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> +      to create a zwp_kms_lease_v1 object (the actual lease object) and also to
> +      revoke the lease.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the lease request object">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +  <request name="create">

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

> +    <description summary="create">
> +      Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
> +      Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
> +    </description>
> +    <arg name="connector" type="string" summary="connector output name"/>
> +  </request>
> +
> +  <request name="revoke">
> +    <description summary="revoke">
> +      Revoke the lease, using the zwp_kms_lease_v1 objected received in
> +      zwp_kms_lease_request_v1::created event.
> +    </description>
> +    <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1" summary="lease object to remove" />
> +  </request>
> +
> +  <event name="created">
> +    <description summary="lease created successfully">
> +    This event indicates that the lease has been created. It provides a zwp_kms_lease_v1 object
> +    used for retrieving the fd representing the lease.
> +    </description>
> +    <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1" summary="the lease obj"/>
> +  </event>
> +
> +  <event name="failed">
> +    <description summary="lease could not be created">
> +      This event indicates that the lease could not be created/revoked.
> +    </description>
> +  </event>
> +
> +  <event name="revoked">
> +    <description summary="lease revoked successfully">
> +      This event indicates that the lease has been revoked.
> +    </description>
> +  </event>
> +
> +  </interface>
> +</protocol>

regards
Philipp
Marius-cristian Vlad Aug. 22, 2018, 11:12 a.m.
On Wed, 2018-08-22 at 09:17 +0200, Philipp Zabel wrote:
> Hi Marius,


Hi,

> 

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

> > 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%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1

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

> > gPbYibU3CTjKcSMJaiqBSP7FfAJD2h%2BLPYiE%2FXJ8z0%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%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1

> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&amp;sdata=S

> > h%2B1TbPeqbUnaYF%2FwenCZIKTcR9P7l585Ellujk0Bq8%3D&amp;reserved=0

> > 

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

> > 

> > Changes since v2:

> > - advertise connector names when creating a lease request object

> > (Pekka Paalanen)

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

> > (Pekka Paalanen)

> > - various grammar/spelling fixes (Pekka Paalanen)

> > 

> > Changes since v1:

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

> > (Daniel Stone)

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

> > like dmabuf (Daniel Stone)

> > ---

> > Some caveats while at it. This is just in RFC form adapted from the

> > comments

> > I've  mostly got from Pekka Paalanen. It most certainly doesn't

> > address all the issues

> > brought up: like multi-node card environments/system, doing a

> > TEST_ONLY

> > commit before giving out a lease, or takes hot-plugging into

> > consideration. 

> > As I was side-tracked to other things and

> > being on hiatus for a while, I wanted first to get some impressions

> > first if

> > indeed this is the right approach and do some incremental updates

> > afterwards.

> > 

> > The are some issues which I'd like to point out from the beginning,

> > which

> > require some further comments. In no particular order:

> > 

> > I've found that I can't pass a wl_array as a way to advertise

> > various

> > information to the client. (wl_array works fine with integers not

> > with

> > allocated data). It would probably be better to advertise also

> > monitor name, other EDID info or available modes, but at the moment

> > I only 

> > join-split a delimiter between connector names and send it out as a

> > string.  

> > While it is ugly I'm not aware of a way to send this information

> > back to the

> > client in the form of a list. 

> > 

> > The client is aware before hand of this delimiter and has the

> > number of entries:

> > can easily choose or pick one of the connectors. It could be that

> > there are

> > some other ways this can be achieved, I welcome any kind of

> > feedback here.

> 

> Why not just send the connectors one by one, a single event with all

> relevant information for each?


Hmm, okay, I'll try do that. 

> 

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

> headset.

> 

> > Secondly, when doing a modeset, the client requires a valid mode

> > (drmModeModeInfo).  I'm currently unsure how to pass this back to

> > the client.

> > The obvious type="object" interface="drmModeModeInfo" fails to link

> > and instead

> > I rely on the fact that a) the client can retrieve IDs from the

> > lease using

> > lease API (drmModeGetLease()) which gives a connector id -- 

> > or alternately can also use a wl_array to pass that,

> > and b) the client can use the leased fd to iterate over connectors.

> > Matching those two would allow to get a valid

> > drmModeModeInfo object to pass it when modesetting (for more info

> > see the client implementation provided at the end).

> > Question is, is this an acceptable way of doing it? Do note that

> > this

> > can only be "used" after the lease has been created.

> 

> Can't the client query available modes on the passed connector via

> the

> leased fd?


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


> 

> > A server/client implementation to match this protocol 

> > can be found at https://emea01.safelinks.protection.outlook.com/?ur

> > l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fco

> > mmits%2Fnew-drm-lease&amp;data=02%7C01%7Cmarius-

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

> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&amp;sdata=G

> > 4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3D&amp;reserved=0

> 

> This crashed for me in drm_lease_manager_create_lease_req with a NULL

> pointer dereference because head->output == NULL for an unconnected

> head.

> Also I noticed zwm_kms_lease_request_v1_implementation is missing the

> .destroy request callback.


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

> 

> >  Makefile.am                                  |   1 +

> >  unstable/drm-lease/README                    |   4 +

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

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

> >  3 files changed, 178 insertions(+)

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

> >  create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml

> > 

> > diff --git a/Makefile.am b/Makefile.am

> > index 6394e26..5d13dca 100644

> > --- a/Makefile.am

> > +++ b/Makefile.am

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

> > 				\

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

> > 		\

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

> > 		\

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

> > 	\

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

> > 		\

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

> > 		\

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

> > 		\

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

> > 	\

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

> > new file mode 100644

> > index 0000000..a25600c

> > --- /dev/null

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

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

> > +Linux DRM lease

> > +

> > +Maintainers:

> > +Marius Vlad <marius-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..6cb3c0a

> > --- /dev/null

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

> > @@ -0,0 +1,173 @@

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

> > +<protocol name="drm_lease_unstable_v1">

> > +

> > +<copyright>

> > +    Copyright 2018 NXP

> > +

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

> > obtaining a

> > +    copy of this software and associated documentation files (the

> > "Software"),

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

> > limitation

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

> > sublicense,

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

> > whom the

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

> > conditions:

> > +

> > +    The above copyright notice and this permission notice

> > (including the next

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

> > portions of the

> > +    Software.

> > +

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

> > KIND, EXPRESS OR

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

> > MERCHANTABILITY,

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

> > EVENT SHALL

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

> > DAMAGES OR OTHER

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

> > OTHERWISE, ARISING

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

> > OTHER

> > +    DEALINGS IN THE SOFTWARE.

> > +</copyright>

> > +

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

> > +    <description summary="lease manager">

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

> > Packard.

> > +

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

> > resources it has

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

> > can not be

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

> > or the new

> > +      DRM master is closed.

> > +

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

> > has leased out

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

> > +      (DRM master which controls resources leased from another DRM

> > master). This

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

> > Lessee.

> > +

> > +      The Lessee can issue modesetting/page-flipping atomic

> > operations etc.,

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

> > by the Lessor.

> > +

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

> > uniquely

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

> > from a single

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

> > had used, the

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

> > +

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

> > and

> > +      backward incompatible changes may be made. Backward

> > compatible changes

> > +      may be added together with the corresponding interface

> > version bump.

> > +      Backward incompatible changes are done by bumping the

> > version number in

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

> > version.

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

> > and the

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

> > removed and the

> > +      interface version number is reset.

> > +    </description>

> > +

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

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

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

> > through this

> > +        interface.

> > +      </description>

> > +    </request>

> > +

> > +    <request name="create_lease_req">

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

> > the lease">

> > +        Create an object for temporarily storing all the KMS

> > resources to be leased.

> > +      </description>

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

> > interface="zwp_kms_lease_request_v1"

> > +           summary="the new temporary"/>

> > +    </request>

> > +

> > +    <event name="connectors">

> > +      <description summary="advertise which connectors can be used

> > to request creation for a lease">

> > +        This event advertises leasable connectors. When multiple

> > connectors are

> > +        advertised, the client should properly parse and choose

> > one of connectors

> > +        A client trying to create a lease using

> > zwp_kms_lease_request_v1::create

> > +        request would use this connector name.

> > +      </description>

> > +      <arg name="connectors" type="string" summary="list of

> > connector entries, split by '|' char"/>

> > +      <arg name="connectors_entries" type="uint" summary="number

> > of connectors found in connectors string"/>

> > +    </event>

> > +

> > +  </interface>

> > +

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

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

> > +      This interface represents the lease object and encapsulates

> > the lease

> > +      properties. This objected is sent by

> > zwp_kms_lease_request_v1::created

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

> > +    </description>

> > +

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

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

> > object">

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

> > through this

> > +        interface.

> > +      </description>

> > +    </request>

> > +

> > +    <request name="get_lease_info">

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

> > lease">

> > +        Request to retrieve lease information, like leased fd,

> > monitor

> > +        and connector name.

> > +      </description>

> > +    </request>

> > +

> > +    <event name="lease_info">

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

> > +        This event returns (among other information about the

> > connector leased)

> > +        the leased fd which is required for modesetting or queying

> > page flips.

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

> > information

> > +        like connector, CRTC, and plane ID using

> > drmModeGetLease(). Using that

> > +        information it can derive a suitable mode useful when

> > performing modeset.

> > +      </description>

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

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

> > of the lease" />

> > +      <arg name="monitor_name" type="string" summary="monitor

> > name" />

> > +    </event>

> > +  </interface>

> > +

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

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

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

> > It is used

> > +      to create a zwp_kms_lease_v1 object (the actual lease

> > object) and also to

> > +      revoke the lease.

> > +    </description>

> > +

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

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

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

> > through this

> > +        interface.

> > +      </description>

> > +    </request>

> > +

> > +  <request name="create">

> 

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

> 

> > +    <description summary="create">

> > +      Create a lease using the connector output name received in

> > zwp_kms_lease_manager_v1::connectors.

> > +      Any attempt to use an incorrect connector name will reswult

> > in zwp_kms_lease_request_v1::failed.

> > +    </description>

> > +    <arg name="connector" type="string" summary="connector output

> > name"/>

> > +  </request>

> > +

> > +  <request name="revoke">

> > +    <description summary="revoke">

> > +      Revoke the lease, using the zwp_kms_lease_v1 objected

> > received in

> > +      zwp_kms_lease_request_v1::created event.

> > +    </description>

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

> > interface="zwp_kms_lease_v1" summary="lease object to remove" />

> > +  </request>

> > +

> > +  <event name="created">

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

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

> > provides a zwp_kms_lease_v1 object

> > +    used for retrieving the fd representing the lease.

> > +    </description>

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

> > interface="zwp_kms_lease_v1" summary="the lease obj"/>

> > +  </event>

> > +

> > +  <event name="failed">

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

> > +      This event indicates that the lease could not be

> > created/revoked.

> > +    </description>

> > +  </event>

> > +

> > +  <event name="revoked">

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

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

> > +    </description>

> > +  </event>

> > +

> > +  </interface>

> > +</protocol>

> 

> regards

> Philipp
Keith Packard Aug. 22, 2018, 4:24 p.m.
Marius-cristian Vlad <marius-cristian.vlad@nxp.com> writes:


>> Can't the client query available modes on the passed connector via
>> the
>> leased fd?
>
> That's how the client does it now, it uses the leased fd to query
> available modes. Presumably the client should have/receive all the data
> from the compositor....

Just so you know -- basic mode information isn't sufficient to identify
the target headset. Systems have been attempting to identify the headset
assuming the pixel size of the device was unique, but there are now
headsets reporting 'normal' desktop sizes and so something based on
EDID is likely to be necessary.
Philipp Zabel Aug. 23, 2018, 6:12 a.m.
On Wed, 2018-08-22 at 09:24 -0700, Keith Packard wrote:
> Marius-cristian Vlad <marius-cristian.vlad@nxp.com> writes:
> 
> > > Can't the client query available modes on the passed connector via
> > > the
> > > leased fd?
> > 
> > That's how the client does it now, it uses the leased fd to query
> > available modes. Presumably the client should have/receive all the data
> > from the compositor....
> 
> Just so you know -- basic mode information isn't sufficient to identify
> the target headset. Systems have been attempting to identify the headset
> assuming the pixel size of the device was unique, but there are now
> headsets reporting 'normal' desktop sizes and so something based on
> EDID is likely to be necessary.

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

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

regards
Philipp
Philipp Zabel Aug. 23, 2018, 6:41 a.m.
Hi,

On Wed, 2018-08-22 at 11:12 +0000, Marius-cristian Vlad wrote:
[...]
> > Why not just send the connectors one by one, a single event with all
> > relevant information for each?
> 
> Hmm, okay, I'll try do that. 

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

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

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

> > Especially EDID info would be most useful for finding the right VR
> > headset.
> > 
> > > Secondly, when doing a modeset, the client requires a valid mode
> > > (drmModeModeInfo).  I'm currently unsure how to pass this back to
> > > the client.
> > > The obvious type="object" interface="drmModeModeInfo" fails to link
> > > and instead
> > > I rely on the fact that a) the client can retrieve IDs from the
> > > lease using
> > > lease API (drmModeGetLease()) which gives a connector id -- 
> > > or alternately can also use a wl_array to pass that,
> > > and b) the client can use the leased fd to iterate over connectors.
> > > Matching those two would allow to get a valid
> > > drmModeModeInfo object to pass it when modesetting (for more info
> > > see the client implementation provided at the end).
> > > Question is, is this an acceptable way of doing it? Do note that
> > > this
> > > can only be "used" after the lease has been created.
> > 
> > Can't the client query available modes on the passed connector via
> > the
> > leased fd?
> 
> That's how the client does it now, it uses the leased fd to query
> available modes. Presumably the client should have/receive all the data
> from the compositor....

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

> > > A server/client implementation to match this protocol 
> > > can be found at https://emea01.safelinks.protection.outlook.com/?ur
> > > l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fco
> > > mmits%2Fnew-drm-lease&amp;data=02%7C01%7Cmarius-
> > > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
> > > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&amp;sdata=G
> > > 4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3D&amp;reserved=0
> > 
> > This crashed for me in drm_lease_manager_create_lease_req with a NULL
> > pointer dereference because head->output == NULL for an unconnected
> > head.
> > Also I noticed zwm_kms_lease_request_v1_implementation is missing the
> > .destroy request callback.
> 
> Good catch, fixed. You need to fetch it again.

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

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

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

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

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

regards
Philipp
Philipp Zabel Aug. 23, 2018, 6:49 a.m.
On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
[...]
> +  <interface name="zwp_kms_lease_request_v1" version="1">
> +    <description summary="lease request object">
> +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> +      to create a zwp_kms_lease_v1 object (the actual lease object) and also to
> +      revoke the lease.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the lease request object">
> +        This has no effect on any remaining objects created through this
> +        interface.
> +      </description>
> +    </request>
> +
> +  <request name="create">
> +    <description summary="create">
> +      Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
> +      Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
> +    </description>
> +    <arg name="connector" type="string" summary="connector output name"/>

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

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

regards
Philipp
Pekka Paalanen Aug. 23, 2018, 11:39 a.m.
Sorry for the potentially duplicate email, my MUA messed up the CC list
on the first go.


On Thu, 23 Aug 2018 08:41:30 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Hi,
> 
> On Wed, 2018-08-22 at 11:12 +0000, Marius-cristian Vlad wrote:
> [...]  
> > > Why not just send the connectors one by one, a single event with all
> > > relevant information for each?    

Hi,

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

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

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

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

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

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

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

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

Is there a defined upper limit on EDID size?

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

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

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

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

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

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

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

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

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

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


On Thu, 23 Aug 2018 08:49:13 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
> [...]  
> > +  <interface name="zwp_kms_lease_request_v1" version="1">
> > +    <description summary="lease request object">
> > +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> > +      to create a zwp_kms_lease_v1 object (the actual lease object) and also to
> > +      revoke the lease.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroys the lease request object">
> > +        This has no effect on any remaining objects created through this
> > +        interface.
> > +      </description>
> > +    </request>
> > +
> > +  <request name="create">
> > +    <description summary="create">
> > +      Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
> > +      Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
> > +    </description>
> > +    <arg name="connector" type="string" summary="connector output name"/>    
> 
> Instead of submitting the connector identification as an argument to the
> create request, would adding a separate add_connector request be more
> extensible?
> 
> That would allow to request a single lease for multiple connectors,
> or to add a mechanism to request adding overlay planes to the lease.  

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

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


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


Thanks,
pq
Marius-cristian Vlad Aug. 23, 2018, 3:12 p.m.
On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote:
> Sorry for the potentially duplicate email, my MUA messed up the CC

> list

> on the first go.

> 

> 

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

> Philipp Zabel <p.zabel@pengutronix.de> wrote:

> 

> > Hi,

> > 

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

> > [...]  

> > > > Why not just send the connectors one by one, a single event

> > > > with all

> > > > relevant information for each?    

> 

> Hi,

> 

> yes, sending one event per connector is a good design, but see below

> if

> we actually might want to extend that to creating an object per

> connector with "per-attribute" events for pieces of information.

> 

> > > 

> > > Hmm, okay, I'll try do that.     

> > 

> > I'm wondering what should be used to identify a connector to a

> > hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to

> > other APIs that may request leases on the application's behalf.

> > 

> > What could be passed into a Vulkan function to request the lease

> > and

> > retrieve the corresponding VkDisplayKHR object? wl_display and

> > make/model/serial strings? wl_display and drm connector name?

> > Or is there need for an object describing a leasable connector,

> > similar to wl_output?  

> 

> One certainly cannot rely on wl_output, because that will not be

> present for outputs that are not currently used as part of the

> desktop.

> IOW, HMDs are likely never exposed as a wl_output.

> 

> > Or should the leasing be done by the application itself, outside of

> > Vulkan, and just the zwp_kms_lease_v1 object be passed in?  

> 

> These are good questions, I hope answers will be found for what the

> Vulkan and other APIs will actually need for advertising what they

> need

> to advertise.

> 

> > > > Especially EDID info would be most useful for finding the right

> > > > VR

> > > > headset.  

> 

> Sharing EDID could be tricky mechanically. If we assume that an

> average

> EDID blob is 256 bytes, it would be small enough to pass "inline" in

> Wayland, e.g. as a wl_array argument on an event. But since we want

> to

> provide it for all leasable connectors, the burst of data could grow

> bigger than the buffers in libwayland, and we start relying on the

> kernel socket buffers which are much larger usually.

> 

> Maybe it's ok to start with a wl_array while we are still in unstable

> protocol, but this question may need to be revisited in the future.

> 

> Is there a defined upper limit on EDID size?

> 

> Btw. since we may need to share EDID, and we may need applications to

> parse EDID to dig out what they need, I believe there will be a

> demand

> for a library to do that. Does any such exist already?


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

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

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

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

> 

> > > >     

> > > > > Secondly, when doing a modeset, the client requires a valid

> > > > > mode

> > > > > (drmModeModeInfo).  I'm currently unsure how to pass this

> > > > > back to

> > > > > the client.

> > > > > The obvious type="object" interface="drmModeModeInfo" fails

> > > > > to link

> > > > > and instead

> > > > > I rely on the fact that a) the client can retrieve IDs from

> > > > > the

> > > > > lease using

> > > > > lease API (drmModeGetLease()) which gives a connector id -- 

> > > > > or alternately can also use a wl_array to pass that,

> > > > > and b) the client can use the leased fd to iterate over

> > > > > connectors.

> > > > > Matching those two would allow to get a valid

> > > > > drmModeModeInfo object to pass it when modesetting (for more

> > > > > info

> > > > > see the client implementation provided at the end).

> > > > > Question is, is this an acceptable way of doing it? Do note

> > > > > that

> > > > > this

> > > > > can only be "used" after the lease has been created.    

> > > > 

> > > > Can't the client query available modes on the passed connector

> > > > via

> > > > the

> > > > leased fd?    

> > > 

> > > That's how the client does it now, it uses the leased fd to query

> > > available modes. Presumably the client should have/receive all

> > > the data

> > > from the compositor....    

> 

> The client should not receive "all" the data from the compositor

> via Wayland, but only the bits it needs to make the decision whether

> to

> attempt leasing a connector or not.

> 

> Modes are part of EDID, so if we send EDID, I think that's good

> enough.

> After all, we want to describe the monitor/HMD foremost, not what the

> kernel or the compositor have decided it can do (e.g. add standard

> modes the EDID might miss, or prune modes that cannot be used).

> That data does not need to be complete either, I think. It should

> allow

> coarse pruning, and yes, ideally it would allow choosing the right

> connectors immediately, but clients still have the opportunity to

> lease

> a connector and ask DRM for more details before making the decision.

> 

> In summary, I don't know if we need to send the whole EDID or just

> selected standard fields from EDID (make, model, serial, ...), and

> likewise I'm not sure there is need to send video mode info if that's

> not useful for choosing a connector/monitor.

> 

> After the client has chosen a connector and leased it, then it will

> receive everything it needs directly from the kernel, e.g. a video

> mode

> list.

> 

> > If there was a "leasable connector" object instead of just the

> > connector

> > event, that could report modes and EDID data as events, like

> > wl_output.

> > I'm not sure if that is a good idea or unnecessary complication.  

> 

> That depends on how much data we want to send. A single message has a

> quite limited size. libwayland buffering sets a hard limit at 4096

> bytes, but in practise we should use, say, 1024 bytes max. If that

> message includes EDID inline, that's already 128-512 bytes taken.

> 

> From protocol specification point of view, if the ad is just one

> event

> that carries all per-connector information, it cannot be extended. If

> we

> later want to include more information, we need to duplicate the

> event

> to add new arguments to it, because existing events cannot be

> modified

> without breaking all consumers. OTOH, if each piece of information is

> sent as a separate event on a "leasable connector" object, adding new

> pieces of information is much cleaner, no need to duplicate the

> existing events.

> 

> 

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

> Philipp Zabel <p.zabel@pengutronix.de> wrote:

> 

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

> > [...]  

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

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

> > > +      This interface is used for managing zwp_kms_lease_v1

> > > object. It is used

> > > +      to create a zwp_kms_lease_v1 object (the actual lease

> > > object) and also to

> > > +      revoke the lease.

> > > +    </description>

> > > +

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

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

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

> > > through this

> > > +        interface.

> > > +      </description>

> > > +    </request>

> > > +

> > > +  <request name="create">

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

> > > +      Create a lease using the connector output name received in

> > > zwp_kms_lease_manager_v1::connectors.

> > > +      Any attempt to use an incorrect connector name will

> > > reswult in zwp_kms_lease_request_v1::failed.

> > > +    </description>

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

> > > output name"/>    

> > 

> > Instead of submitting the connector identification as an argument

> > to the

> > create request, would adding a separate add_connector request be

> > more

> > extensible?

> > 

> > That would allow to request a single lease for multiple connectors,

> > or to add a mechanism to request adding overlay planes to the

> > lease.  

> 

> If there is any possibility that we might want to build a single

> lease

> request from additional bits (e.g. planes) in the future, then I

> would

> recommend the add_connector request on a tentative lease config

> object

> approach. The linux_dmabuf extension already uses the same pattern to

> have a variable number of things in the final request.

> 

> As said, one cannot add arguments to an existing request or event

> later

> - you have to replace the whole message if you want something added.

> 

> 

> Sorry, I'm a bit too busy to do a proper review of the protocol for

> now, but I hope these comments push it in a good direction.

> 

> 

> Thanks,

> pq
Marius-cristian Vlad Aug. 23, 2018, 3:14 p.m.
On Thu, 2018-08-23 at 08:41 +0200, Philipp Zabel wrote:
> Hi,

> 

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

> [...]

> > > Why not just send the connectors one by one, a single event with

> > > all

> > > relevant information for each?

> > 

> > Hmm, okay, I'll try do that. 

> 

> I'm wondering what should be used to identify a connector to a

> hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to

> other APIs that may request leases on the application's behalf.

> 

> What could be passed into a Vulkan function to request the lease and

> retrieve the corresponding VkDisplayKHR object? wl_display and

> make/model/serial strings? wl_display and drm connector name?

> Or is there need for an object describing a leasable connector,

> similar to wl_output?

> 

> Or should the leasing be done by the application itself, outside of

> Vulkan, and just the zwp_kms_lease_v1 object be passed in?

> 

> > > Especially EDID info would be most useful for finding the right

> > > VR

> > > headset.

> > > 

> > > > Secondly, when doing a modeset, the client requires a valid

> > > > mode

> > > > (drmModeModeInfo).  I'm currently unsure how to pass this back

> > > > to

> > > > the client.

> > > > The obvious type="object" interface="drmModeModeInfo" fails to

> > > > link

> > > > and instead

> > > > I rely on the fact that a) the client can retrieve IDs from the

> > > > lease using

> > > > lease API (drmModeGetLease()) which gives a connector id -- 

> > > > or alternately can also use a wl_array to pass that,

> > > > and b) the client can use the leased fd to iterate over

> > > > connectors.

> > > > Matching those two would allow to get a valid

> > > > drmModeModeInfo object to pass it when modesetting (for more

> > > > info

> > > > see the client implementation provided at the end).

> > > > Question is, is this an acceptable way of doing it? Do note

> > > > that

> > > > this

> > > > can only be "used" after the lease has been created.

> > > 

> > > Can't the client query available modes on the passed connector

> > > via

> > > the

> > > leased fd?

> > 

> > That's how the client does it now, it uses the leased fd to query

> > available modes. Presumably the client should have/receive all the

> > data

> > from the compositor....

> 

> If there was a "leasable connector" object instead of just the

> connector

> event, that could report modes and EDID data as events, like

> wl_output.

> I'm not sure if that is a good idea or unnecessary complication.

> 

> > > > A server/client implementation to match this protocol 

> > > > can be found at https://emea01.safelinks.protection.outlook.com

> > > > /?ur

> > > > l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%

> > > > 2Fco

> > > > mmits%2Fnew-drm-lease&amp;data=02%7C01%7Cmarius-

> > > > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C68

> > > > 6ea1

> > > > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187&amp;sda

> > > > ta=G

> > > > 4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3D&amp;reserved=0

> > > 

> > > This crashed for me in drm_lease_manager_create_lease_req with a

> > > NULL

> > > pointer dereference because head->output == NULL for an

> > > unconnected

> > > head.

> > > Also I noticed zwm_kms_lease_request_v1_implementation is missing

> > > the

> > > .destroy request callback.

> > 

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

> 

> Thank you, it works now. I've poked at it a bit, and noticed that the

> compositor crashes if the same lease is requested again before it was

> improperly revoked. This happened in three cases:

> 

> - if a second weston-egl-simple-lease is started while the first one

>   is still running,

> - if weston-egl-simple-lease is stopped by a VT switch (because the

>   next pageflip fails) and started again after switching back to

> weston,

> - if weston-egl-simple-lease is killed with SIGTERM and started

> again.

> 

> In the last case the last frame the last frame of the already killed

> application was still visible on the output.

> 

> I've also tried pulling and replugging the cable on the leased

> connector, which ends with a compositor assertion after replugging:


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

Thanks for testing this!

> 

> weston: compositor/main.c:1726: drm_process_layoutput: Assertion

> `output->output->enabled' failed.

> 

> regards

> Philipp
Pekka Paalanen Aug. 24, 2018, 8:26 a.m.
On Thu, 23 Aug 2018 15:12:03 +0000
Marius-cristian Vlad <marius-cristian.vlad@nxp.com> wrote:

> On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote:

> > > > > Especially EDID info would be most useful for finding the right
> > > > > VR
> > > > > headset.    
> > 
> > Sharing EDID could be tricky mechanically. If we assume that an
> > average
> > EDID blob is 256 bytes, it would be small enough to pass "inline" in
> > Wayland, e.g. as a wl_array argument on an event. But since we want
> > to
> > provide it for all leasable connectors, the burst of data could grow
> > bigger than the buffers in libwayland, and we start relying on the
> > kernel socket buffers which are much larger usually.
> > 
> > Maybe it's ok to start with a wl_array while we are still in unstable
> > protocol, but this question may need to be revisited in the future.
> > 
> > Is there a defined upper limit on EDID size?
> > 
> > Btw. since we may need to share EDID, and we may need applications to
> > parse EDID to dig out what they need, I believe there will be a
> > demand
> > for a library to do that. Does any such exist already?  
> 
> Isn't this overkill? Asking the client to parse that blob?
> 
> Wouldn't make more sense to add the fields Philipp mentions
> (manufacturer ID/product code) fields in drm_edid? The compositor
> already parses the EDID to provide/supply a serial number/monitor name.
> 
> If the client can ask the kernel (through the leased fd) and chooses
> what mode it wants, then what would be the point to send the whole
> EDID? 

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

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

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

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


Thanks,
pq
Philipp Zabel Aug. 24, 2018, 8:57 a.m.
Hi Pekka,

On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote:
> Sorry for the potentially duplicate email, my MUA messed up the CC list
> on the first go.
> 
> 
> On Thu, 23 Aug 2018 08:41:30 +0200
> Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > Hi,
> > 
> > On Wed, 2018-08-22 at 11:12 +0000, Marius-cristian Vlad wrote:
> > [...]  
> > > > Why not just send the connectors one by one, a single event with all
> > > > relevant information for each?    
> 
> Hi,
> 
> yes, sending one event per connector is a good design, but see below if
> we actually might want to extend that to creating an object per
> connector with "per-attribute" events for pieces of information.

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

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

> > > Hmm, okay, I'll try do that.     
> > 
> > I'm wondering what should be used to identify a connector to a
> > hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
> > other APIs that may request leases on the application's behalf.
> > 
> > What could be passed into a Vulkan function to request the lease and
> > retrieve the corresponding VkDisplayKHR object? wl_display and
> > make/model/serial strings? wl_display and drm connector name?
> > Or is there need for an object describing a leasable connector,
> > similar to wl_output?  
> 
> One certainly cannot rely on wl_output, because that will not be
> present for outputs that are not currently used as part of the desktop.
> IOW, HMDs are likely never exposed as a wl_output.

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

> > Or should the leasing be done by the application itself, outside of
> > Vulkan, and just the zwp_kms_lease_v1 object be passed in?  
> 
> These are good questions, I hope answers will be found for what the
> Vulkan and other APIs will actually need for advertising what they need
> to advertise.

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

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

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

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

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

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

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

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

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

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

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

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

Ok.

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

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

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

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

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

Yes.

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

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

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

Thank you for the detailed initial feedback.

regards
Philipp
Marius-cristian Vlad Aug. 24, 2018, 9:21 a.m.
On Fri, 2018-08-24 at 10:57 +0200, Philipp Zabel wrote:
> Hi Pekka,

> 

> On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote:

> > Sorry for the potentially duplicate email, my MUA messed up the CC

> > list

> > on the first go.

> > 

> > 

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

> > Philipp Zabel <p.zabel@pengutronix.de> wrote:

> > 

> > > Hi,

> > > 

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

> > > [...]  

> > > > > Why not just send the connectors one by one, a single event

> > > > > with all

> > > > > relevant information for each?    

> > 

> > Hi,

> > 

> > yes, sending one event per connector is a good design, but see

> > below if

> > we actually might want to extend that to creating an object per

> > connector with "per-attribute" events for pieces of information.

> 

> One reason for creating an object would be that in some cases (like

> VR)

> the client will want to match on vendor/model/serial from EDID, but

> in

> other scenarios EDID data might not even be available.

> When leasing a DPI or LVDS panel without EDID information, the

> connector

> would have to be identified via the connector name, for example.

> 

> And object would allow handling both cases the same way when building

> the lease request.


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

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

We do something like this:

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


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

Is this the right assumption?

> 

> > > > Hmm, okay, I'll try do that.     

> > > 

> > > I'm wondering what should be used to identify a connector to a

> > > hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or

> > > to

> > > other APIs that may request leases on the application's behalf.

> > > 

> > > What could be passed into a Vulkan function to request the lease

> > > and

> > > retrieve the corresponding VkDisplayKHR object? wl_display and

> > > make/model/serial strings? wl_display and drm connector name?

> > > Or is there need for an object describing a leasable connector,

> > > similar to wl_output?  

> > 

> > One certainly cannot rely on wl_output, because that will not be

> > present for outputs that are not currently used as part of the

> > desktop.

> > IOW, HMDs are likely never exposed as a wl_output.

> 

> Understood, it would have to be a new object. There could be more of

> them than wl_outputs, and they wouldn't carry the same information:

> output geometry obviously has no place here. But there is some

> overlap.

> The wl_output geometry event also contains "make" and "model"

> arguments    <event

> name="connector"><                                                   

>                           

>       <description summary="advertise a leasable

> connector"><                                             

>         This event advertises a leasable connector. This allows the

> client to<                            

>         choose which connector the compositor should lease. It can

> either<                                

>         use connector name, monitor name, or if that's not sufficient

> to parse<                           

>         EDID

> blob.<                                                               

>                         

>       </description><                                                

>                                      

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

> /><                                    

>       <arg name="eisa_id" type="string" summary="eisa

> id"/><                                              

>       <arg name="monitor_name" type="string" summary="monitor

> name"/><                                    

>       <arg name="pnp_id" type="string" summary="pnp

> id"/><                                                

>       <arg name="serial_num" type="string" summary="serial

> number"/><                                     

>       <arg name="edid" type="array" summary="EDID

> blob"/><                                                

>     </event><                                                        

>                                      

> (but is missing product id or serial). And I notice that

> zxdg_output_v1

> already has a "name" event that likely contains the connector name.

> Maybe the leasable connector object should report all of these as

> events.

> 

> > > Or should the leasing be done by the application itself, outside

> > > of

> > > Vulkan, and just the zwp_kms_lease_v1 object be passed in?  

> > 

> > These are good questions, I hope answers will be found for what the

> > Vulkan and other APIs will actually need for advertising what they

> > need

> > to advertise.

> 

> I suppose this is something that should be discussed on the Vulkan

> side

> first? Given we'd need a new extension there anyway, maybe the

> solution

> is not making this wayland specific at all and just adding a way to

> import the leased DRM fd.

> 

> > > > > Especially EDID info would be most useful for finding the

> > > > > right VR

> > > > > headset.  

> > 

> > Sharing EDID could be tricky mechanically. If we assume that an

> > average

> > EDID blob is 256 bytes, it would be small enough to pass "inline"

> > in

> > Wayland, e.g. as a wl_array argument on an event. But since we want

> > to

> > provide it for all leasable connectors, the burst of data could

> > grow

> > bigger than the buffers in libwayland, and we start relying on the

> > kernel socket buffers which are much larger usually.

> 

> I'm not sure if pushing the complete EDID is necessary. Maybe there

> should be an EDID request if preparsed vendor/product/serial or

> connector name are good enough to identify the to-be-leased connector

> in

> most cases.

> 

> > Maybe it's ok to start with a wl_array while we are still in

> > unstable

> > protocol, but this question may need to be revisited in the future.

> > 

> > Is there a defined upper limit on EDID size?

> 

> EDID contains an 8-bit number of extension blocks, there should be an

> upper limit of about 32 KiB.

> 

> > Btw. since we may need to share EDID, and we may need applications

> > to

> > parse EDID to dig out what they need, I believe there will be a

> > demand

> > for a library to do that. Does any such exist already?

> 

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

> 

> [...]

> > The client should not receive "all" the data from the compositor

> > via Wayland, but only the bits it needs to make the decision

> > whether to

> > attempt leasing a connector or not.

> 

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

> to

> lease.

> 

> > Modes are part of EDID, so if we send EDID, I think that's good

> > enough.

> > After all, we want to describe the monitor/HMD foremost, not what

> > the

> > kernel or the compositor have decided it can do (e.g. add standard

> > modes the EDID might miss, or prune modes that cannot be used).

> 

> What about legacy LVDS and DPI displays that have no EDID

> information?

> I expect those would be matched by connector name, but maybe there

> are

> also use cases where somebody would select a connector to lease

> depending on its native resolution or refresh rate.

> 

> > That data does not need to be complete either, I think. It should

> > allow

> > coarse pruning, and yes, ideally it would allow choosing the right

> > connectors immediately, but clients still have the opportunity to

> > lease

> > a connector and ask DRM for more details before making the

> > decision.

> 

> Ok.

> 

> > In summary, I don't know if we need to send the whole EDID or just

> > selected standard fields from EDID (make, model, serial, ...), and

> > likewise I'm not sure there is need to send video mode info if

> > that's

> > not useful for choosing a connector/monitor.

> 

> For the use cases that I can currently imagine (VR, special fixed

> displays, projectors on a special connector) either

> 

>   manufacturer id (make), product code (better than monitor name

>   string) and monitor serial number

> or

>   connector name

> 

> should be good enough. But as said, I'm not sure if there could be

> cases where just matching native resolution or refresh rate would

> be required.

> 

> > After the client has chosen a connector and leased it, then it will

> > receive everything it needs directly from the kernel, e.g. a video

> > mode

> > list.

> 

> Yes.

> 

> > > If there was a "leasable connector" object instead of just the

> > > connector

> > > event, that could report modes and EDID data as events, like

> > > wl_output.

> > > I'm not sure if that is a good idea or unnecessary

> > > complication.  

> > 

> > That depends on how much data we want to send. A single message has

> > a

> > quite limited size. libwayland buffering sets a hard limit at 4096

> > bytes, but in practise we should use, say, 1024 bytes max. If that

> > message includes EDID inline, that's already 128-512 bytes taken.

> > 

> > From protocol specification point of view, if the ad is just one

> > event

> > that carries all per-connector information, it cannot be extended.

> > If we

> > later want to include more information, we need to duplicate the

> > event

> > to add new arguments to it, because existing events cannot be

> > modified

> > without breaking all consumers. OTOH, if each piece of information

> > is

> > sent as a separate event on a "leasable connector" object, adding

> > new

> > pieces of information is much cleaner, no need to duplicate the

> > existing events.

> 

> If we had an object, we could start with an event that just carries

> model/vendor/serial and add an edid event (possibly even with an edid

> request) later, if necessary.

> 

> [...]

> > > Instead of submitting the connector identification as an argument

> > > to the

> > > create request, would adding a separate add_connector request be

> > > more

> > > extensible?

> > > 

> > > That would allow to request a single lease for multiple

> > > connectors,

> > > or to add a mechanism to request adding overlay planes to the

> > > lease.  

> > 

> > If there is any possibility that we might want to build a single

> > lease

> > request from additional bits (e.g. planes) in the future, then I

> > would

> > recommend the add_connector request on a tentative lease config

> > object

> > approach. The linux_dmabuf extension already uses the same pattern

> > to

> > have a variable number of things in the final request.

> > 

> > As said, one cannot add arguments to an existing request or event

> > later

> > - you have to replace the whole message if you want something

> > added.

> > 

> > Sorry, I'm a bit too busy to do a proper review of the protocol for

> > now, but I hope these comments push it in a good direction.

> 

> Thank you for the detailed initial feedback.

> 

> regards

> Philipphttps://www.youtube.com/watch?v=QulEM-Azzughttps://www.youtube

> .com/watch?v=QulEM-Azz
Philipp Zabel Aug. 24, 2018, 9:58 a.m.
Hi Marius,

On Fri, 2018-08-24 at 09:21 +0000, Marius-cristian Vlad wrote:
> On Fri, 2018-08-24 at 10:57 +0200, Philipp Zabel wrote:
[...]
> > > yes, sending one event per connector is a good design, but see
> > > below if we actually might want to extend that to creating an
> > > object per connector with "per-attribute" events for pieces of
> > > information.
> > 
> > And object would allow handling both cases the same way when building
> > the lease request.
> 
> To make sure I understand this "object" leasing. Instead of advertising
> connectors this way:
> 
>     <event name="connector">
>       <description summary="advertise a leasable connector">
>         This event advertises a leasable connector. This allows the    
>            client to
>         choose which connector the compositor should lease. It can
> either
>         use connector name, monitor name, or if that's not sufficient
> to parse
>         EDID blob.
>       </description>
>       <arg name="conn_name" type="string" summary="connector name" />
>       <arg name="eisa_id" type="string" summary="eisa id"/>
>       <arg name="monitor_name" type="string" summary="monitor name"/>
>       <arg name="pnp_id" type="string" summary="pnp id"/>
>       <arg name="serial_num" type="string" summary="serial number"/>
>       <arg name="edid" type="array" summary="EDID blob"/>
>     </event>
> 
> We do something like this:
> 
>     <event name="connector_object">
>       <description summary="advertise a leasable connector">
>         This event advertises a leasable connector object.
>       </description>
>       <arg name="conn_obj" type="new_id"
> interface="zwp_kms_lease_connector_v1"
>       summary="the new temporary" />
>     </event>
> 
> 
> Then that interface is used to query info (like EDID/conn_name/monitor
> name)

Yes, that is my understanding as well.

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

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

> and using that information to ask/revoke the lease (based on
> either connector name, or based on EDID)?

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

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

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

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

regards
Philipp
Marius-cristian Vlad Aug. 24, 2018, 12:10 p.m.
On Fri, 2018-08-24 at 11:58 +0200, Philipp Zabel wrote:
> Hi Marius,

> 

> On Fri, 2018-08-24 at 09:21 +0000, Marius-cristian Vlad wrote:

> > On Fri, 2018-08-24 at 10:57 +0200, Philipp Zabel wrote:

> 

> [...]

> > > > yes, sending one event per connector is a good design, but see

> > > > below if we actually might want to extend that to creating an

> > > > object per connector with "per-attribute" events for pieces of

> > > > information.

> > > 

> > > And object would allow handling both cases the same way when

> > > building

> > > the lease request.

> > 

> > To make sure I understand this "object" leasing. Instead of

> > advertising

> > connectors this way:

> > 

> >     <event name="connector">

> >       <description summary="advertise a leasable connector">

> >         This event advertises a leasable connector. This allows

> > the    

> >            client to

> >         choose which connector the compositor should lease. It can

> > either

> >         use connector name, monitor name, or if that's not

> > sufficient

> > to parse

> >         EDID blob.

> >       </description>

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

> > />

> >       <arg name="eisa_id" type="string" summary="eisa id"/>

> >       <arg name="monitor_name" type="string" summary="monitor

> > name"/>

> >       <arg name="pnp_id" type="string" summary="pnp id"/>

> >       <arg name="serial_num" type="string" summary="serial

> > number"/>

> >       <arg name="edid" type="array" summary="EDID blob"/>

> >     </event>

> > 

> > We do something like this:

> > 

> >     <event name="connector_object">

> >       <description summary="advertise a leasable connector">

> >         This event advertises a leasable connector object.

> >       </description>

> >       <arg name="conn_obj" type="new_id"

> > interface="zwp_kms_lease_connector_v1"

> >       summary="the new temporary" />

> >     </event>

> > 

> > 

> > Then that interface is used to query info (like

> > EDID/conn_name/monitor

> > name)

> 

> Yes, that is my understanding as well.

> 

> I expect that for all currently known use cases the connector name

> (like

> xdg_output.name) and the monitor vendor / product id / serial string

> should be sufficient. Maybe the native resolution and refresh rate

> could

> be useful as well.

> 

> I would suggest leaving the raw EDID out for now, a request for it

> can

> be added later to zwp_kms_lease_connector_v1, if necessary. It won't

> be

> possible to pass otentially huge complete EDIDs in an array argument.

> 

> > and using that information to ask/revoke the lease (based on

> > either connector name, or based on EDID)?

> 

> I imagine that object to be passed to the lease request's

> add_connector

> request directly:

> 

>   <interface name="zwp_kms_lease_request_v1">

>     ...

>     <request name="add_connector">

>       <arg name="connector" type="object"

>            interface="zwp_kms_lease_connector_v1"

>            summary="a leasable connector to be added to the lease

> request"/>

>     </request>

>   </interface>

> 

> That way there is no need for separate requests

> "add_connector_by_name",

> "add_connnector_by_vendor_product_serial" etc.

> 

> The same could potentially be done later with a

> "zwp_kms_lease_plane_v1"

> object and "add_plane" request.


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


> 

> regards

> Philipp