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