[PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.
Marius-cristian Vlad
marius-cristian.vlad at nxp.com
Thu Jan 25 11:10:04 UTC 2018
-----Original Message-----
From: Daniel Stone [mailto:daniel at fooishbar.org]
Sent: Thursday, January 25, 2018 12:07 AM
To: Marius-cristian Vlad <marius-cristian.vlad at nxp.com>
Cc: wayland-devel at lists.freedesktop.org; Pekka Paalanen <pekka.paalanen at collabora.co.uk>; Keith Packard <keithp at keithp.com>
Subject: Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.
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.
[mvlad] No problem. Thanks for taking the time to review it. I'll adjust it accordingly.
Also, do you have a client you're using for this somewhere, that we could use to test?
[mvlad] Indeed, would've make sense to add a test client for it. Will add it.
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.
[mvlad] Not sure what you mean. Checking for "lease" in config file or the ifdefs?
> +#ifdef HAVE_DRM_LEASE
> +/* hold current leases given */
> +static struct wl_list leases;
This should be under drm_backend.
[mvlad] Initially I wanted to have this isolated from the drm compositor, then I realized I needed some way
to disable/destroy the output. Having them inside drm_backend makes much more sense.
> +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.
> +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'.
[mvlad] OK, will fix that.
> +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.
[mvlad] Understood.
> +#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.
[mvlad] This sounds nice. Will give it a try.
> +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.
[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.
> +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.
[mvlad] Right, will do.
> + /* 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.
[mvlad] Oops, indeed it seems so. Yes, a field in drm_output seems the right way to do it. Thanks for spotting this.
> + 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.
> +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! :)
[mvlad] Right, will do.
Cheers,
Daniel
More information about the wayland-devel
mailing list