[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 <[email protected]> > > --- > > > > 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 > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
