From f9ef4e64ea1238c313bef3bd519d4102707f4da6 Mon Sep 17 00:00:00 2001 From: Leandro Ribeiro Date: Wed, 17 Aug 2022 18:36:47 -0300 Subject: [PATCH] backend-drm: fix possible leak of struct drm_output Before this patch, we would leak the drm_output if there was a pending flip during shutdown. Now we destroy the drm_output even if there's a pending flip (only during shutdown, as we don't want to wait until flip completion to destroy the output). Also, it fixes a problem where weston_output_enable() is called right after weston_output_enable() or weston_output_disable() and it could fail to find available DRM objects (as they are only released after the flip completion). Signed-off-by: Leandro Ribeiro --- libweston/backend-drm/drm-internal.h | 1 + libweston/backend-drm/drm.c | 92 +++++++++++++++++++++++++--- libweston/backend-drm/fb.c | 11 +++- 3 files changed, 93 insertions(+), 11 deletions(-) diff --git a/libweston/backend-drm/drm-internal.h b/libweston/backend-drm/drm-internal.h index 3c7171d6..7f427018 100644 --- a/libweston/backend-drm/drm-internal.h +++ b/libweston/backend-drm/drm-internal.h @@ -395,6 +395,7 @@ enum drm_fb_type { struct drm_fb { enum drm_fb_type type; + struct drm_backend *backend; struct drm_device *scanout_device; int refcnt; diff --git a/libweston/backend-drm/drm.c b/libweston/backend-drm/drm.c index a5031c9a..debd3ef7 100644 --- a/libweston/backend-drm/drm.c +++ b/libweston/backend-drm/drm.c @@ -1875,13 +1875,7 @@ ret: static void drm_crtc_destroy(struct drm_crtc *crtc) { - /* TODO: address the issue below to be able to remove the comment - * from the assert. - * - * https://gitlab.freedesktop.org/wayland/weston/-/issues/421 - */ - - //assert(!crtc->output); + assert(!crtc->output); wl_list_remove(&crtc->link); drm_property_info_free(crtc->props_crtc, WDRM_CRTC__COUNT); @@ -2094,6 +2088,29 @@ drm_output_detach_crtc(struct drm_output *output) output->crtc = NULL; } +static bool +should_wait_drm_events(struct drm_device *device) +{ + struct weston_output *base; + struct drm_output *output; + + wl_list_for_each(base, &device->backend->compositor->output_list, link) { + /* We only care about outputs related to the DRM backend. */ + output = to_drm_output(base); + if (!output) + continue; + + /* We only care about the outputs on our device. */ + if (output->device != device) + continue; + + if (output->destroy_pending || output->disable_pending) + return true; + } + + return false; +} + static int drm_output_enable(struct weston_output *base) { @@ -2105,6 +2122,21 @@ drm_output_enable(struct weston_output *base) assert(output); assert(!output->virtual); + /* TODO: drop this hack when we rework the output configuration API. For + * now we need this because the frontend may call + * weston_output_destroy() or weston_output_disable() but it may have a + * pending page flip. In such case the destruction is delayed, but the + * frontend can't tell that. Until the flip completes, the DRM objects + * of the card (CRTC, planes, etc) won't be available, as they will + * still be attached to the output marked to be destroyed/disabled. So + * if the frontend calls weston_output_enable() for an output right + * after weston_output_destroy() or weston_output_disable(), it might + * not find DRM objects available and fail. So we spin here until the + * flip completes and the output gets destroyed/disabled and release the + * DRM objects. */ + while (should_wait_drm_events(device)) + on_drm_input(device->drm.fd, 0 /* unused mask */, device); + if (!output->format) { if (output->base.eotf_mode != WESTON_EOTF_MODE_SDR) output->format = @@ -2204,9 +2236,19 @@ drm_output_destroy(struct weston_output *base) assert(!output->virtual); if (output->page_flip_pending || output->atomic_complete_pending) { - output->destroy_pending = true; - weston_log("destroy output while page flip pending\n"); - return; + if (!base->compositor->shutting_down) { + /* We are not shutting down, so we can wait for flip + * completion. */ + output->destroy_pending = true; + weston_log("delaying output destruction because of a " \ + "pending flip, wait until it completes\n"); + return; + } else { + weston_log("destroying output %s (id %u) with a pending " \ + "flip, but as we are shutting down we can't " \ + "wait to destroy it when the flip completes... " \ + "destroying it now\n", base->name, base->id); + } } drm_output_set_cursor_view(output, NULL); @@ -3168,6 +3210,8 @@ drm_destroy(struct weston_backend *backend) struct weston_compositor *ec = b->compositor; struct drm_device *device = b->drm; struct weston_head *base, *next; + struct weston_output *output_base; + struct drm_output *output; struct drm_crtc *crtc, *crtc_tmp; struct drm_writeback *writeback, *writeback_tmp; @@ -3176,6 +3220,34 @@ drm_destroy(struct weston_backend *backend) wl_event_source_remove(b->udev_drm_source); wl_event_source_remove(b->drm_source); + /* We are shutting down. This function destroy the planes with + * destroy_sprites() and then calls weston_compositor_shutdown(), which + * will lead to calls to drm_output_destroy(). We are destroying the + * plane and its state_cur in drm_plane_destroy(), and that removes + * state_cur from the output->state_cur list, but not from the + * state_last list. + * + * This was not an issue because we'd leak the drm_output (and so + * output->last_state) during shutdown with a pending page flip. And if + * we were no shutting down, we'd always wait until flip completion + * before destroying an output (meaning that + * drm_output_update_complete() would be called and output->state_last + * would be freed and set to NULL). + * + * But now we don't leak the drm_output during shutdown with a pending + * flip anymore. So we must destroy output->state_last for every output + * before destroying the planes with destroy_sprites(), otherwise we'd + * leave output->state_last referring to a freed plane, leading to + * issues when trying to free it at a later point. + */ + wl_list_for_each(output_base, &ec->output_list, link) { + output = to_drm_output(output_base); + if (output && (output->page_flip_pending || output->atomic_complete_pending)) { + drm_output_state_free(output->state_last); + output->state_last = NULL; + } + } + destroy_sprites(b->drm); weston_log_scope_destroy(b->debug); diff --git a/libweston/backend-drm/fb.c b/libweston/backend-drm/fb.c index 0c2c0f6e..92041778 100644 --- a/libweston/backend-drm/fb.c +++ b/libweston/backend-drm/fb.c @@ -47,7 +47,12 @@ static void drm_fb_destroy(struct drm_fb *fb) { - if (fb->fb_id != 0) + /* TODO: do not leak the fb during shutdown. + * When we are shutting down, the CRTC may be scanning out our fb. If we + * destroy it, the CRTC may turn off. So leak that on purpose. What we + * need here is a libdrm function that we can use to tell DRM/KMS that + * it is free to destroy our fb once it stops using it. */ + if (fb->fb_id != 0 && !fb->backend->compositor->shutting_down) drmModeRmFB(fb->fd, fb->fb_id); free(fb); } @@ -264,6 +269,8 @@ drm_fb_create_dumb(struct drm_device *device, int width, int height, return NULL; fb->refcnt = 1; + fb->backend = device->backend; + fb->format = pixel_format_get_info(format); if (!fb->format) { weston_log("failed to look up format 0x%lx\n", @@ -418,6 +425,7 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, fb->refcnt = 1; fb->type = BUFFER_DMABUF; + fb->backend = device->backend; ARRAY_COPY(import_mod.fds, dmabuf->attributes.fd); ARRAY_COPY(import_mod.strides, dmabuf->attributes.stride); @@ -507,6 +515,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_device *device, fb->type = type; fb->refcnt = 1; + fb->backend = device->backend; fb->bo = bo; fb->fd = device->drm.fd;