[PATCH wayland-protocols v4] unstable/drm-lease: DRM lease protocol support

[Marius-cristian Vlad]

On Thu, 2018-08-30 at 14:38 +0200, Philipp Zabel wrote:
> On Thu, 2018-08-30 at 11:01 +0000, Marius-cristian Vlad wrote:
> [...]
> > > 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 
> 
> when this
> 
> > happens and revoke leases to lost lessees.
> > 
> > Mvlad: That's what I expected to see, but I did some brief tests
> > here,
> > and it could the our lease kernel version could a little bit older,
> > but when SIGKILL'ing the client, the DRM subsystem doesn't revoke
> > the
> > lease, or calls drm_lease_destroy. I suppose the following happens:
> > when creating the lease the compositor will hold a reference of the
> > leased fd -- well technically the DRM leases part does -- (and ofc
> > I
> > can't close it then because the client won't be able to use it),
> 
> The compositor has to close lease_fd after sending it to the client
> via
> zwp_kms_lease_v1_send_leased_fd. Otherwise there's two open
> references
> to the lease drm_master, and closing only the client's reference will
> not cause it to be destroyed.

Yes, you are correct. I mistakenly though I can only close it after the
client sends the revoke request.
 

> 
> >  we pass that thru weston (over the unix socket), the client will
> > also
> > hold a reference for that leased fd, so basically the lease will
> > only
> > be destroyed when that ref counter hits 0. In the implementation I
> > close the leased fd deliberately on the compositor side, when I get
> > the revoke request from the client.
> 
> If there is only one open lease_fd reference in the client, there is
> no
> need for the revoke request. The client can just close the fd and the
> compositor will revoke the lease object after gets the hotplug event
> and
> notices the lessee is gone.
> 
> Now if the client keeps the leased_fd open but requests to destroy
> the
> zwp_kms_lease_v1 object, the compositor must revoke the DRM lease,
> same
> as when a VT switch happens or when the connector gets unplugged.

Alright I'll re-work this part and send an update. 

> 
> >  There's no issue when the client behaves, but only when the client
> > dies abruptly, and it won't notify the compositor that it is no
> > longer
> > using the lease. I've only seen this once when you brought it up,
> > so I
> > need to test it further to be sure about this.
> 
> My understanding is that if the client dies, the last open fd
> referencing the lease drm_master is closed

Yep, I've tested it now and that's how it works. 

> 
> > >  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.
> > 
> > Mvlad: I'm not really sure on this, see my above comment. 
> > 
> > >  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.
> > 
> > mvlad: Which means that the client would have to be notified when
> > that happens, so it can shut down cleanly. 
> 
> Could that be via the revoked event on the zwp_kms_lease_v1?

I'd say yes.

> 
> > Somehow the client (in its rendering part perhaps) has to check if
> > the lease is still valid. Guess we can also
> > verify errno, so it should not complicate the client that much.
> 
> Right. If at some point the client suddenly fails to pageflip, it
> should
> be able to cope.
> 
> [...]
> > Mvlad: This relates also the above comments as well. Need to verify
> > if
> > indeed it works like this. But a follow up question here regarding
> > planes: 
> > We've discussed this quite extensively, and I've concluded that it
> > is
> > much easier to gather the objects ids required to create the lease,
> > directly in the compositor as it already has that information. I
> > kind
> > of assume that the connector object will also provide the CRTC and
> > implicitly the planes it can drive.
> 
> I think the connector should bring its CRTC and a primary plane.
> 
> At least for devices that have overlay planes that can move between
> CRTCs, overlay planes should not be included implicitly without the
> client asking for them, because the compositor may have use for them.
> 
> I suppose for devices that have planes fixed to CRTCs it wouldn't
> hurt
> to just lease all planes for the connected CRTC, but for consistency
> I
> wouldn't add unrequested overlay planes here either.
> 
> Overlay planes could be listed as zwp_kms_plane_v1 objects, just as
> connectors.

Uhum, alright. I'll have to test this as well. Thanks for the feedback.
 

Will send an update soon. 

> 
> regards
> Philipp