[PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Daniel Stone]

Hi Marius,

On 25 January 2018 at 11:10, Marius-cristian Vlad
<marius-cristian.vlad at nxp.com> wrote:
> >> +struct drm_display {
> >> +       uint32_t connector_id;
> >> +       uint32_t crtc_id;
> >> +       uint32_t plane_id;
> >> +};
> >
> > This seems to get stored but not used after that?
>
> [mvlad] A helper struct to store the objects we want to pass. Easier to pass as a whole.

If you're setting a flag in drm_output (see below), it might be easier
to pass a pointer to the drm_output, and then take a list of
drm_planes?

> >> +       if (!choosen_output) {
> >> +               weston_log("No valid output found to lease!\n");

Oh also, as another matter of general principle: until we have useful
debugging logs, please don't log things which can be provoked by the
client, like this. If you want to get more verbose information out,
adding a string parameter to the 'failed' event would let you
communicate errors back down to the client, which you could also see
via the WAYLAND_DEBUG logs.

> >> +       weston_log("Lease leased_id = %u created, on output %s\n",
> >> +                  lease->leased_id, choosen_output->base.name);

Logging for failure definitely seems too verbose as well. ;)

> >> +       wl_list_insert(&leases, &lease->list);
> >> +       drm_output_destroy(&choosen_output->base);
> >> +
> >> +       zwp_drm_lease_v1_send_created(resource, lease->leased_fd,
> >> +                                     lease->leased_id); }
> >
> > Rather than destroying the output, I'd be much more comfortable marking it as disabled or so. There is also a race here: in the DRM backend, output destruction can be deferred, when a pageflip has not yet completed.
>
> [mvlad] Assuming that you are referring to weston_output_disable, the backends output disable callback
> -- drm_output_disable will eventually disable the output. With the output disabled the client will no longer be able to use the connector. It could also be that my testing version of the client assumes that the output will not be disabled (hence forth
> it can question the leased fd for connector/crtc/plane). From my tests destroying the output seem to be the most reliable.

Right. What I mean is that if we schedule a pageflip from Weston and a
client requests a lease before the pageflip completes, then
drm_output_destroy() will perform a delayed destroy, potentially
shutting down the CRTC. This can happen _after_ the resource has been
leased to the client, which would probably be quite confusing.

When I said 'marking it as disabled or so', what I meant was below: a
flag in the drm_output struct which notes that the output has been
leased to a client and should not be used. I think the most robust way
would be if we did this (so the DRM compositor would know not to try
to use the output), and also allowed lease creation to be deferred.
This would allow us to only give the lease to the client after Weston
has cleaned up all its use of those resources; it would also
potentially allow other users to call out to an external policy
manager (which may not be immediate) to determine if the lease should
be allowed, and help prevent race conditions when two clients are
trying to hand over a lease between themselves.

> >> +                       wl_signal_emit(&compositor->wake_signal, compositor);
> >> +                       wl_event_source_timer_update(compositor->idle_source,
> >> +
> >> + compositor->idle_time * 1000);
> >
> > I assume this is just to force a repaint. If the existing compositor API doesn't quite work for this, we should create API which does, or make sure enabling the output does the right thing. Are you using desktop-shell, or ... ?
>
> [mvlad] Indeed. What I've observed is that it could be some time until the repaint fires and in that time the fb of the client can still be present on that output. Forcing a repaint seems to fix that. There's also a longer explanation: If the client destroyes the fb this would cause the connector to be disabled. If weston can reclaim the connector after it has been disabled there's no issue. I will need to check this once more, it might not be needed after all.

Right. If we create/enable a new Weston output, this should result in
repaint happening by itself: just like it does with hotplug now.

Thanks again!

Cheers,
Daniel