Hi Marius, Disclaimer: I don't feel very qualified to comment on the protocol design, take this with a grain of salt.
Am Mittwoch, den 29.08.2018, 14:00 +0300 schrieb Marius Vlad: > Simple protocol extension to manage DRM lease. Based on the work by Keith > Packard in [1], respectively [2]. > > [1] > https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f > > Changes since v3: > - instead of advertising all leasable connectors at once, advertise each > connector for each leasable output. Use an object to represent a > leasable connector and provide a request to add that leasable connector > when creating the lease (Philipp Zabel) > - add two events to handle add_connector request > - add some comments in the manager interface to explain how the protocol > can/should be used > > Changes since v2: > - advertise connectors when creating a lease request object (Pekka Paalanen) > - when revoking the lease use the lease object instead of lessee id (Pekka > Paalanen) > - various grammar/spelling fixes (Pekka Paalanen) > > Changes since v1: > - added manager: advertise lease capability and manage the lease (Daniel > Stone) > - add request(s) for adding connector/crtc/plane to behave more like dmabuf > (Daniel Stone) > > Signed-off-by: Marius Vlad <marius-cristian.v...@nxp.com> > --- > A (rather incomplete) implementation for this version can be found at > https://gitlab.freedesktop.org/marius.vlad0/weston/tree/drm-lease-advertise-each-connnector-object > > One interesting question is how to handle the situation when the client > deliberately, or not, holds the lease indefinitely. The client implicitly cancels the lease by closing the lease fd (or by having it closed automatically when it is terminated). The compositor will get a hotplug event then, and it has to check the lessee list happens and revoke leases to lost lessees. > This has multiple > ramification like what happens when the client un-expectingly dies and doesn't > revoking the lease, See above, a lease terminates when its last fd is closed. > or when hot-plugging the connector and weston tries to get > a hold of a connector previously leased? I'd say hot-unplugging a connector that is part of a lease should cause this lease to be canceled. > It could be there might be other > situations where the compositor will need to revoke the lease. The VT switch case, for example. If the compositor has to give up drm_master temporarily, it will have to revoke all leases first. > In the implementation we could deny the compositor to get a hold of the > connector when hot-plugging that output, if that's desirable, and presumably > establish a way to detect periodically if the lease is still in use. > > Alternatively, shouldn't we use something like a ping/pong approach in which > the client (in rendering part), sends periodically an alive signal? I don't think the compositor should care what the lessee is doing with its lease, only whether it is still holding the fd. > Makefile.am | 1 + > unstable/drm-lease/README | 4 + > unstable/drm-lease/drm-lease-unstable-v1.xml | 244 > +++++++++++++++++++++++++++ > 3 files changed, 249 insertions(+) > create mode 100644 unstable/drm-lease/README > create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml > > diff --git a/Makefile.am b/Makefile.am > index 6394e26..5d13dca 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -4,6 +4,7 @@ unstable_protocols = > \ > unstable/pointer-gestures/pointer-gestures-unstable-v1.xml > \ > unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml > \ > unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml > \ > + unstable/drm-lease/drm-lease-unstable-v1.xml > \ > unstable/text-input/text-input-unstable-v1.xml > \ > unstable/text-input/text-input-unstable-v3.xml > \ > unstable/input-method/input-method-unstable-v1.xml > \ > diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README > new file mode 100644 > index 0000000..a25600c > --- /dev/null > +++ b/unstable/drm-lease/README > @@ -0,0 +1,4 @@ > +Linux DRM lease > + > +Maintainers: > +Marius Vlad <marius-cristian.v...@nxp.com> > diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml > b/unstable/drm-lease/drm-lease-unstable-v1.xml > new file mode 100644 > index 0000000..40eedb4 > --- /dev/null > +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml > @@ -0,0 +1,244 @@ > +<?xml version="1.0" encoding="UTF-8"?> > +<protocol name="drm_lease_unstable_v1"> > + > +<copyright> > + Copyright 2018 NXP > + > + Permission is hereby granted, free of charge, to any person obtaining a > + copy of this software and associated documentation files (the > "Software"), > + to deal in the Software without restriction, including without limitation > + the rights to use, copy, modify, merge, publish, distribute, sublicense, > + and/or sell copies of the Software, and to permit persons to whom the > + Software is furnished to do so, subject to the following conditions: > + > + The above copyright notice and this permission notice (including the next > + paragraph) shall be included in all copies or substantial portions of the > + Software. > + > + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > OR > + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + DEALINGS IN THE SOFTWARE. > +</copyright> > + > + <interface name="zwp_kms_lease_manager_v1" version="1"> > + <description summary="lease manager"> > + This interface makes use of DRM lease written by Keith Packard. > + > + A DRM master can create another DRM master and ``lease'' resources it > has > + control over to the new DRM master. Once leased, resources can not be > + controlled by the owner unless the owner cancels the lease, or the new Should this say 'unless the owner revokes the lease'? > + DRM master is closed. > + > + A lease is a contract between the Lessor (DRM master which has leased > out > + resources to one or more other DRM masters) and a Lessee > + (DRM master which controls resources leased from another DRM master). > This > + contract specifies which resources may be controlled by the Lessee. > + > + The Lessee can issue modesetting/page-flipping atomic operations etc., > + just like a Lessor using the leased file-descriptor passed by the > Lessor. > + > + Besides the leased file-descriptor, an integer is used to uniquely > + identify a Lessee within the tree of DRM masters descending from a > single > + Owner. Once the Lessee has finished with the resources it had used, the > + Lessee ID can be used to revoke that lease. 'Tree of DRM masters' sounds like subleasing is still supported. I think that got removed. > + Upon connecting to the compositor all leasable connectors will be > + advertised to the client. These connectors are represented by > + zwp_kms_lease_connector_v1 interface, and have to be "added" before > + creating a lease object. 'and have to be "added" to a lease request'? > This instructs the compositor to use that > + connector when creating a lease. The client can receive multiple events > + for multiple leasable connectors and needs a way to discern between > + various leasable connectors. zwp_kms_lease_connector_v1 provides > requests > + and events to retrieve additional information specific to that > connector > + object. Is it required/preferred to hide the common connector properties behind requests, or should the connector object send these events unsolicitedly? > + > + zwp_kms_lease_request_v1::created event will provide a lease object > + represented by zwp_kms_lease_v1 interface. The client will then use > + this lease object to retrieve the file-descriptor (leased fd) and > + use it to perform mode-setting/atomic operations. > + Whilst the operation to revoke a lease requires a lesse id, in our > case, > + the ::revoke request will require the previous obtained lease object > to be > + used when revoking the lease. Closing the fd should terminate the lease as well, causing the compositor to revoke it. > + > + Warning! The protocol described in this file is experimental and > + backward incompatible changes may be made. Backward compatible changes > + may be added together with the corresponding interface version bump. > + Backward incompatible changes are done by bumping the version number in > + the protocol and interface names and resetting the interface version. > + Once the protocol is to be declared stable, the 'z' prefix and the > + version number in the protocol and interface names are removed and the > + interface version number is reset. > + </description> > + > + <request name="destroy" type="destructor"> > + <description summary="destroys the manager"> > + This has no effect on any remaining objects created through this > + interface. > + </description> > + </request> > + > + <request name="create_lease_req"> > + <description summary="create a temporary object for managing the > lease"> > + Create an object for temporarily storing all the KMS resources to be > leased. > + </description> > + <arg name="params_id" type="new_id" > interface="zwp_kms_lease_request_v1" > + summary="the new temporary"/> > + </request> > + > + <event name="connector"> > + <description summary="advertise which connector can be used to request > a lease"> > + This event advertises a leasable connector object. When creating a > + lease pass this object to zwp_kms_lease_request_v1::add_connector. As > + multiple connectors can leasable (based on compositor policy), the 'can be leasable'? > + client can request additional information using > + zwp_kms_lease_connector_v1 interface in order to distinguish between > + different leasable connectors. After the client has added (using > + zwp_kms_lease_request_v1::add_connector) a leasable > + connector object, zwp_kms_lease_request_v1::create request should be > + called for creating a zwp_kms_lease_v1 lease object. > + </description> > + <arg name="conn_obj" type="new_id" > interface="zwp_kms_lease_connector_v1" > + summary="the new temporary" /> > + </event> > + > + </interface> > + > + <interface name="zwp_kms_lease_v1" version="1"> > + <description summary="the lease object itself"> > + This interface represents the lease object and encapsulates the lease > + properties. This objected is sent by zwp_kms_lease_request_v1::created > + event. Use this object to retrieve lease properties. > + </description> > + > + <request name="destroy" type="destructor"> > + <description summary="destroys the temporary lease request object"> > + This has no effect on any remaining objects created through this > + interface. > + </description> > + </request> > + > + <request name="retrieve_leased_fd"> > + <description summary="request to retrieve info about the lease"> > + Request to retrieve the leased file-descriptor. > + </description> > + </request> > + > + <event name="leased_fd"> > + <description summary="returns information about the lease"> > + This event returns the leased fd which is required for modesetting > + or querying page flips/atomic modesetting. > + The client can use the leased fd to find out DRM-related information > + like connector ID, CRTC ID, and plane ID using drmModeGetLease(). > + Using that information it can derive a suitable mode useful > + when performing a modeset. > + </description> > + <arg name="leased_fd" type="fd" summary="leased fd" /> > + </event> > + </interface> > + > + <interface name="zwp_kms_lease_connector_v1" version="1"> > + <description summary="zwp_kms_lease_connector_v1"> > + This interface describes a connector object advertised by > + zwp_kms_lease_manager_v1::connector. The client can distinguish between > + different leasable connectors by requesting additional information for > + that connector. > + </description> > + > + <request name="retrieve_name"> > + <description summary="name"> > + Request to retrieve connector output name on which the leasable > + connector object is based on. > + </description> > + </request> Is this necessary? It seems to me that the name event could just be sent without request to new listeners. > + <event name="name"> > + <description summary="name"> > + Event sent when requesting connector output name. > + </description> > + <arg name="conn_name" type="string" summary="connector name" /> > + </event> > + </interface> > + > + <interface name="zwp_kms_lease_request_v1" version="1"> > + <description summary="lease request object"> > + This interface is used for managing zwp_kms_lease_v1 object. It is used > + to create a zwp_kms_lease_v1 object (the actual lease object), to > revoke > + it, and to specify from which connector the lease should be created. I'm not sure about the revoking part, actually. I expected this to handle more like zwp_linux_buffer_params_v1, as a single-use object to create a zwp_kms_lease_v1 once (after adding connectors and, possibly later, planes). After successful lease creation (or failure) this could be destroyed. Since the zwp_kms_lease_v1 itself has a destructor, and the actual lease can be terminated by closing the fd anyway, I see no reason to keep the original zwp_kms_lease_request_v1 around. > + </description> > + > + <request name="destroy" type="destructor"> > + <description summary="destroys the lease request object"> > + This has no effect on any remaining objects created through this > + interface. > + </description> > + </request> > + > + <request name="add_connector"> > + <description summary="add a leasable connector object"> > + This request is used to create the lease object using the leasable > + connector object, and must be called before > zwp_kms_lease_request_v1::create. > + </description> > + <arg name="conn_obj" type="object" > interface="zwp_kms_lease_connector_v1" > + summary="a leasable connector added to the lease"/> > + </request> > + > + <request name="create"> > + <description summary="create the lease"> > + This request will create a zwp_kms_lease_v1 object based on > + zwp_kms_lease_connector_v1 that was added using > + zwp_kms_lease_request_v1::add_connector. > + </description> > + </request> > + > + <request name="revoke"> > + <description summary="revoke"> > + Revoke the lease, using the zwp_kms_lease_v1 objected received in > + zwp_kms_lease_request_v1::created event. > + </description> > + <arg name="lease_obj" type="object" interface="zwp_kms_lease_v1" > + summary="lease object to remove" /> > + </request> See above, I think this is something the client shouldn't have to do. > + <event name="created"> > + <description summary="lease created successfully"> > + This event indicates that the lease has been created. It provides a > + zwp_kms_lease_v1 object used for retrieving the file-descriptor > + representing the lease. > + </description> > + <arg name="lease_obj" type="new_id" interface="zwp_kms_lease_v1" > + summary="the lease obj"/> > + </event> > + > + <event name="failed"> > + <description summary="lease could not be created"> > + This event indicates that the lease could not be created/revoked. > + </description> > + </event> > + > + <event name="revoked"> > + <description summary="lease revoked successfully"> > + This event indicates that the lease has been revoked. > + </description> > + </event> Should this be moved to the zwp_kms_lease_v1? > + <event name="connector_add_failed"> > + <description summary="failed to add connector"> > + This event indicates that the leasable connector object specified in > + zwp_kms_lease_request_v1::add_connector couldn't be added. > + </description> > + </event> > + > + <event name="connector_added"> > + <description summary="connector was added successfully"> > + This event indicates that the leasable connector object specified in > + zwp_kms_lease_request_v1::add_connector has been added successfully. > + </description> > + </event> > + > + </interface> > +</protocol> regards Philipp _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
