[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