[PATCH wayland-protocols v5] unstable/drm-lease: DRM lease protocol support
Marius-cristian Vlad
marius-cristian.vlad at nxp.com
Thu Sep 6 11:29:06 UTC 2018
[resending... maybe this time works]
On Wed, 2018-09-05 at 08:55 +0200, Philipp Zabel wrote:
> Hi Marius,
>
> thank you for the update!
Thank you for taking the time to look over this.
>
> Am Dienstag, den 04.09.2018, 17:39 +0300 schrieb Marius Vlad:
> > Simple protocol extension to manage DRM lease. Based on the work by
> > Keith
> > Packard in [1], respectively [2].
> >
> > [1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fcgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc417153538
> > 9d72e9135c9615cecd07b346fd6d7e&data=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&sdata=p
> > qLcIrKTlgSJ0fVyfdXOXO4Re%2BV6OrNQGccIhMSn0Os%3D&reserved=0
> > [2] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
> > linux.git%2Fcommit%2F%3Fh%3Dv4.15-
> > rc9%26id%3D62884cd386b876638720ef88374b31a84ca7ee5f&data=02%7C0
> > 1%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&sdata=A
> > 3EfXGUUJMl%2FFQHw8LVHPn9Ptu%2BXKAr90EIwnHpLhzw%3D&reserved=0
> >
> > Changes since v4:
>
> [...]
> > - removed 'revoked' event entirely as it adds complexity without
> > adding
> > too much benefit.
>
> The client will notice this via the leased drm fd sooner or later
> anyway, so it seems that this event is not strictly necessary. I
> wonder
> if there is any value in letting the client know immediately, though.
> For example, if a client displays mostly static content (like a
> presentation running on a non-desktop projector), it could be a long
> while until the next page flip attempt.
>
> > Signed-off-by: Marius Vlad <marius-cristian.vlad at nxp.com>
> > ---
> >
> > An implementation for this version can be found at
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fcommits%2Fdrm-
> > lease-advertise-each-connnector-object-
> > fixes&data=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055&sdata=M
> > UrWEApwiGgSvcz8yKiT9Tm8Ij3UpGs6Ai5ioTtuWDI%3D&reserved=0
> >
>
> I tried the current weston implementation with this protocol version,
> and I think the leased property and lease object have to be moved
> from
> output to head.
> Currently, all heads without enabled output are skipped, and trying
> to
> lease a disabled monitor with:
>
> [output]
> name=HDMI-A-1
> leasable=on
> mode=off
>
> crashes the compositor.
Yes, I've never taken this into consideration. I assume this is the
case for HMDs where by default the connector will be disconnected? The
output contains the scanout_plane which contains the plane id, and
obviously for a disconnected output this will not be the case.
I fixed the crash by checking if the output is set, but most likely
this not the proper fix, if this is required.
>
> [...]
> > + Upon connecting to the compositor all leasable connectors
> > will be
> > + advertised to the client.
>
> What happens if a client has received the full list of leasable
> connectors and then one of those becomes unavailable, either because
> it
> is physically unplugged, or because it is leased to another client?
> Is the client notified about this?
>
> What happens if a new connector becomes available, either because it
> is
> physically plugged in, or because another client cancels its lease?
> This could be handled by sending the connector advertisement (again),
> in which case leasable connectors could appear any time, not only
> upon
> connecting.
Care to explain a bit how exactly does the client reaches that state?
"connector_added", "connected_add_failed" are events that shrink the
window in which the advertised connectors might be different when
actually adding the connector.
>
> > These connectors are represented by
> > + zwp_kms_lease_connector_v1 interface, and have to be "added"
> > to a lease
> > + request object before creating a lease object. This
> > instructs the
> > + compositor to use that connector when creating a lease. The
> > client can
> > + receive multiple events for multiple leasable connectors and
> > needs a way
> > + to discern between various leasable connectors. When
> > receiving this
> > + connector object, events will be sent gratuitously so that
> > the
> > + client can properly choose which connector wants to use.
> > +
> > + A lease request object is represented by
> > zwp_kms_lease_request_v1
> > + interface and is used temporarily as a storage place for
> > objects
>
>
> s/objects/object/
It's a confusion of terminology. Objects in compositor and objects
for the lease creation.
>
> > + IDs. Once the client has a lease object it can freely call
> > its
> > + destructor, zwp_kms_lease_request_v1::destroy.
> > +
> > + zwp_kms_lease_request_v1::created event will provide a lease
> > object
> > + represented by zwp_kms_lease_v1 interface. The client will
> > then use
> > + this lease object to retrieve the file-descriptor (leased
> > fd) and
> > + use it to perform mode-setting/atomic operations.
> > +
> > + Whilst the operation to revoke a lease requires a lesse id,
> > in our case,
> > + calling the destructor for the lease object will cause the
> > lease to be
> > + revoked.
>
> The lessee ID detail is not necessary in the wayland protocol
> description. The above paragraph could be replaced with a list of
> ways
> the lease can be terminated:
>
> - The client closes the leased drm fd.
> - The compositor revokes the lease because objects in the lease
> become
> unavailable for leasing.
Yep, I'll add that.
>
> > +
> > + Warning! The protocol described in this file is experimental
> > and
> > + backward incompatible changes may be made. Backward
> > compatible changes
> > + may be added together with the corresponding interface
> > version bump.
> > + Backward incompatible changes are done by bumping the
> > version number in
> > + the protocol and interface names and resetting the interface
> > version.
> > + Once the protocol is to be declared stable, the 'z' prefix
> > and the
> > + version number in the protocol and interface names are
> > removed and the
> > + interface version number is reset.
> > + </description>
> > +
> > + <request name="destroy" type="destructor">
> > + <description summary="destroys the manager">
> > + Destroys the manager object and should be called last.
> > + </description>
> > + </request>
> > +
> > + <request name="create_lease_req">
> > + <description summary="create a temporary object for managing
> > the lease">
>
> Maybe "create a temporary object for managing a lease"?
> There could be multiple leases.
>
Okay will modify.
> > + Create an object for temporarily storing all the KMS
> > resources to be leased.
> > + </description>
> > + <arg name="params_id" type="new_id"
> > interface="zwp_kms_lease_request_v1"
> > + summary="the new temporary"/>
> > + </request>
> > +
> > + <event name="connector">
> > + <description summary="advertise which connector can be used
> > to request a lease">
>
> Maybe "advertise a connector that can be used to request a lease"?
Idem.
>
> > + This event advertises a leasable connector object. When
> > creating a
> > + lease pass this object to
> > zwp_kms_lease_request_v1::add_connector. As
> > + multiple connectors can be leasable (based on compositor
> > policy),
> > + zwp_kms_lease_connector_v1 will send gratuitously events
> > in order
> > + to distinguish between different leasable connectors.
> > + After the client has added (using
> > + zwp_kms_lease_request_v1::add_connector) a leasable
> > + connector object, zwp_kms_lease_request_v1::create request
> > should be
> > + called for creating a zwp_kms_lease_v1 lease object.
> > + </description>
> > + <arg name="conn_obj" type="new_id"
> > interface="zwp_kms_lease_connector_v1"
> > + summary="the new temporary" />
> > + </event>
> > +
> > + </interface>
> > +
> > + <interface name="zwp_kms_lease_v1" version="1">
> > + <description summary="the lease object itself">
> > + This interface represents the lease object and encapsulates
> > the lease
> > + properties. This objected is sent by
> > zwp_kms_lease_request_v1::created
> > + event. Use this object to retrieve lease properties.
> > + </description>
> > +
> > + <request name="destroy" type="destructor">
> > + <description summary="destroys the temporary lease request
> > object">
> > + This request ask the compositor to revoke the lease,
> > destroy the lease
> > + object and free temporary storage.
> > + </description>
> > + </request>
> > +
> > + <request name="retrieve_leased_fd">
> > + <description summary="request to retrieve info about the
> > lease">
> > + Request to retrieve the leased file-descriptor.
> > + </description>
> > + </request>
>
> Same issue as for the connector "name" event, this request could be
> removed if the "leased_fd" event is just sent automatically when the
> listener is bound.
Alright, will gratuitously send the file descriptor as well.
>
> > +
> > + <event name="leased_fd">
> > + <description summary="returns information about the lease">
> > + This event returns the leased fd which is required for
> > modesetting
> > + or querying page flips/atomic modesetting.
> > + The client can use the leased fd to find out DRM-related
> > information
> > + like connector ID, CRTC ID, and plane ID using
> > drmModeGetLease().
> > + Using that information it can derive a suitable mode
> > useful
> > + when performing a modeset.
>
> It would be useful to mention that closing the leased fd can be used
> to
> terminate the lease.
Yes, will include it.
>
> > + </description>
> > + <arg name="leased_fd" type="fd" summary="leased fd" />
> > + </event>
> > +
> > + </interface>
> > +
> > + <interface name="zwp_kms_lease_connector_v1" version="1">
> > + <description summary="zwp_kms_lease_connector_v1">
> > + This interface describes a connector object advertised by
> > + zwp_kms_lease_manager_v1::connector.
> > + </description>
> > +
> > + <event name="name">
> > + <description summary="name">
> > + Event sent gratuitously to the client representing the
> > connector name.
> > + </description>
> > + <arg name="conn_name" type="string" summary="connector name"
> > />
> > + </event>
>
> It would be useful to have an optional event that contains
> vendor/product id, and serial from EDID, if available.
Okay, I'll add that as well.
>
> > + </interface>
> > +
> > + <interface name="zwp_kms_lease_request_v1" version="1">
> > + <description summary="lease request object">
> > + This interface is used for managing zwp_kms_lease_v1 object.
> > It is used
> > + to create a zwp_kms_lease_v1 object (the actual lease
> > object), to revoke
> > + it, and to specify from which connector the lease should be
> > created.
> > + </description>
> > +
> > + <request name="destroy" type="destructor">
> > + <description summary="destroys the lease request object">
> > + This destroys the lease request interface, and can be
> > called once
> > + the client obtained a zwp_kms_lease_v1 object.
> > + </description>
> > + </request>
> > +
> > + <request name="add_connector">
> > + <description summary="add a leasable connector object">
> > + This request is used to create the lease object using the
> > leasable
> > + connector object, and must be called before
> > zwp_kms_lease_request_v1::create.
> > + </description>
> > + <arg name="conn_obj" type="object"
> > interface="zwp_kms_lease_connector_v1"
> > + summary="a leasable connector added to the lease"/>
> > + </request>
> > +
> > + <request name="create">
> > + <description summary="create the lease">
> > + This request will create a zwp_kms_lease_v1 object based on
> > + zwp_kms_lease_connector_v1 that was added using
> > + zwp_kms_lease_request_v1::add_connector.
> > + </description>
> > + </request>
> > +
> > + <event name="created">
> > + <description summary="lease created successfully">
> > + This event indicates that the lease has been created. It
> > provides a
> > + zwp_kms_lease_v1 object used for retrieving the file-
> > descriptor
> > + representing the lease.
> > + </description>
> > + <arg name="lease_obj" type="new_id"
> > interface="zwp_kms_lease_v1"
> > + summary="the lease obj"/>
> > + </event>
> > +
> > + <event name="failed">
> > + <description summary="lease could not be created">
> > + This event indicates that the lease could not be created.
> > + </description>
> > + </event>
> > +
> > + <event name="connector_add_failed">
> > + <description summary="failed to add connector">
> > + This event indicates that the leasable connector object
> > specified in
> > + zwp_kms_lease_request_v1::add_connector couldn't be added.
> > + </description>
> > + </event>
> > +
> > + <event name="connector_added">
> > + <description summary="connector was added successfully">
> > + This event indicates that the leasable connector object
> > specified in
> > + zwp_kms_lease_request_v1::add_connector has been added
> > successfully.
> > + </description>
> > + </event>
>
> Not sure if the "connector_add_failed" and "connector_added" events
> are
> necessary. If something is wrong, the following "create" request can
> just return the "failed" event.
I've kept these events because "failed" event seemed to generic for me,
and explained a little bit in the beginning another reason why I've
kept them.
>
> regards
> Philipp
>
More information about the wayland-devel
mailing list