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

[Daniel Stone]

Hi Marius,
The protocol changes I suggested would require a fair bit of work
here, but I've enclosed a few comments on the implementation.

Also, do you have a client you're using for this somewhere, that we
could use to test?

On 24 January 2018 at 19:11, Marius Vlad <marius-cristian.vlad at nxp.com> wrote:
> @@ -1208,6 +1209,15 @@ drm_backend_output_configure(struct wl_listener *listener, void *data)
>
>         api->set_seat(output, seat);
>         free(seat);
> +#ifdef HAVE_DRM_LEASE
> +       char *lease;
> +       weston_config_section_get_string(section, "lease", &lease, "off");
> +       if (!strncmp(lease, "on", 2)) {
> +               output->lease = true;
> +               weston_log("Enabling lease on output %s\n", output->name);
> +       }
> +       free(lease);
> +#endif

Hm, doing this in generic code seems a bit odd.

> +#ifdef HAVE_DRM_LEASE
> +/* hold current leases given */
> +static struct wl_list leases;

This should be under drm_backend.

> +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?

> +struct drm_lease {
> +       int leased_fd;
> +       uint32_t leased_id;
> +       struct wl_list list;
> +};

The convention for embedded struct wl_list which is an element of a
list, rather than a list head, is to call it 'link'.

> +struct drm_lease_data {
> +       struct drm_backend *drm_backend;
> +       struct udev_device *drm_device;
> +};

This is a bit odd as we only have one udev device in the backend. Like
the lease list though, any data for leases should just live directly
in the drm_backend.

> +#ifdef HAVE_DRM_LEASE
> +static void
> +drm_lease_destroy(struct drm_backend *b)
> +{
> +       struct drm_lease *lease, *lease_iter;
> +
> +       wl_list_for_each_safe(lease, lease_iter, &leases, list) {
> +               if (drmModeRevokeLease(b->drm.fd, lease->leased_id) < 0) {
> +                       weston_log("Failed to revoke lease id %u\n",
> +                                  lease->leased_id);
> +                       continue;
> +               }
> +
> +               weston_log("Lease id %u revoked\n", lease->leased_id);
> +               wl_list_remove(&lease->list);
> +               free(lease);
> +       }
> +}
> +#endif

If individual leases were a separate object, you could have
drmModeRevokeLease inside the resource destruction handler; this would
then get called when either the client or the compositor was
destroyed.

> +static void
> +drm_lease_create(struct wl_client *client, struct wl_resource *resource)
> +{
> +       struct drm_lease_data *user_data = wl_resource_get_user_data(resource);
> +       struct weston_compositor *compositor = user_data->drm_backend->compositor;
> +       int drm_fd = user_data->drm_backend->drm.fd;
> +
> +       struct drm_output *output = NULL;
> +       struct drm_output *choosen_output = NULL;
> +
> +       uint32_t objects[3];
> +       uint32_t nobjects;
> +
> +       struct drm_lease *lease;
> +       struct drm_display display = {};
> +
> +       wl_list_for_each(output, &compositor->output_list, base.link) {
> +               struct weston_output *wet_output = &output->base;
> +               if (wet_output->lease) {
> +                       display.crtc_id = output->crtc_id;
> +                       display.connector_id = output->connector_id;
> +                       display.plane_id = output->scanout_plane->plane_id;
> +
> +                       choosen_output = output;
> +                       break;
> +               }
> +       }
> +
> +       if (!choosen_output) {
> +               weston_log("No valid output found to lease!\n");
> +               zwp_drm_lease_v1_send_failed(resource);
> +               return;
> +       }
> +
> +       drm_lease_save_objects(&display, objects, &nobjects);
> +       lease = zalloc(sizeof(*lease));
> +
> +       /* create lease */
> +       lease->leased_fd = drmModeCreateLease(drm_fd, objects, nobjects, 0,
> +                                             &lease->leased_id);
> +       if (lease->leased_fd < 0) {
> +               weston_log("Failed to create the lease!");
> +               free(lease);
> +               zwp_drm_lease_v1_send_failed(resource);
> +               return;
> +       }
> +
> +       weston_log("Lease leased_id = %u created, on output %s\n",
> +                  lease->leased_id, choosen_output->base.name);
> +
> +       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.

> +static void
> +drm_lease_revoke(struct wl_client *client, struct wl_resource *resource, uint32_t id)
> +{
> +       struct drm_lease *lease, *lease_iter;
> +       struct drm_lease_data *user_data = wl_resource_get_user_data(resource);
> +       struct weston_compositor *compositor = user_data->drm_backend->compositor;
> +       int drm_fd = user_data->drm_backend->drm.fd;
> +
> +       wl_list_for_each_safe(lease, lease_iter, &leases, list) {
> +               if (lease->leased_id == id) {
> +                       if (drmModeRevokeLease(drm_fd, lease->leased_id) < 0) {
> +                               goto fail;
> +                       }
> +
> +                       wl_list_remove(&lease->list);
> +                       zwp_drm_lease_v1_send_revoked(resource);
> +
> +                       free(lease);

As above, with separate resources, this would just be part of the
resource destruction handler.

> +                       /* re-initialize outputs */
> +                       update_outputs(user_data->drm_backend, user_data->drm_device);

Does this mean if update_outputs is called when a lease is active
(e.g. because another output was hotplugged), the backend will try to
take it over again? For me, this points towards needing a field in the
drm_output which indicates that it is currently reserved by a lease.

> +                       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 ... ?

> +static int
> +drm_lease_init(struct drm_backend *drm_backend, struct udev_device *drm_device)
> +{
> +       struct weston_compositor *compositor = drm_backend->compositor;
> +
> +       wl_list_init(&leases);
> +
> +       drm_lease_data.drm_backend = drm_backend;
> +       drm_lease_data.drm_device = drm_device;
> +
> +       if (!wl_global_create(compositor->wl_display,
> +                             &zwp_drm_lease_v1_interface, 1, &drm_lease_data,
> +                             __drm_lease_init))

Again, this should just be part of drm_backend. No need to hide your
code away! :)

Cheers,
Daniel