drm: Fix leak of damage blob id

This moves the creation of the blob to be earlier, to when the damage is
calculated. It replaces the damage tracked inside of the plane state
with the blob id itself.

This should stop creating new blob ids for TEST_ONLY commits, and them
being leaked in general, as the blob ids are now freed with the plane
state.

The FB_DAMAGE_CLIPS property is now always set if it's supported, and
will be 0 in the case that we have no damage information, which
signifies full damage to the kernel.

Signed-off-by: Scott Anderson <scott.anderson@collabora.com>

diff --git a/libweston/backend-drm/drm-internal.h b/libweston/backend-drm/drm-internal.h
index 855baf1514e35159001334c8724d01bd51b5d9e2..49ce7ce00ebef719c1c9d7c958e0e8b978e4e590 100644 (file)
--- a/libweston/backend-drm/drm-internal.h
+++ b/libweston/backend-drm/drm-internal.h
@@ -418,7 +418,7 @@ struct drm_plane_state {
        /* We don't own the fd, so we shouldn't close it */
        int in_fence_fd;
 
-       pixman_region32_t damage; /* damage to kernel */
+       uint32_t damage_blob_id; /* damage to kernel */
 
        struct wl_list link; /* drm_output_state::plane_list */
 };

diff --git a/libweston/backend-drm/drm.c b/libweston/backend-drm/drm.c
index ca0f3a866643e07c477b08bb6be3efdde0a69c51..4e5f39cf6c392514f482dda92953d7ef0b684268 100644 (file)
--- a/libweston/backend-drm/drm.c
+++ b/libweston/backend-drm/drm.c
@@ -354,8 +354,13 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
        struct weston_compositor *c = output->base.compositor;
        struct drm_plane_state *scanout_state;
        struct drm_plane *scanout_plane = output->scanout_plane;
+       struct drm_property_info *damage_info =
+               &scanout_plane->props[WDRM_PLANE_FB_DAMAGE_CLIPS];
        struct drm_backend *b = to_drm_backend(c);
        struct drm_fb *fb;
+       pixman_region32_t scanout_damage;
+       pixman_box32_t *rects;
+       int n_rects;
 
        /* If we already have a client buffer promoted to scanout, then we don't
         * want to render. */
@@ -399,24 +404,46 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
        scanout_state->dest_w = output->base.current_mode->width;
        scanout_state->dest_h = output->base.current_mode->height;
 
-       pixman_region32_copy(&scanout_state->damage, damage);
+       pixman_region32_subtract(&c->primary_plane.damage,
+                                &c->primary_plane.damage, damage);
+
+       /* Don't bother calculating plane damage if the plane doesn't support it */
+       if (damage_info->prop_id == 0)
+               return;
+
+       pixman_region32_init(&scanout_damage);
+       pixman_region32_copy(&scanout_damage, damage);
+
        if (output->base.zoom.active) {
-               weston_matrix_transform_region(&scanout_state->damage,
+               weston_matrix_transform_region(&scanout_damage,
                                               &output->base.matrix,
-                                              &scanout_state->damage);
+                                              &scanout_damage);
        } else {
-               pixman_region32_translate(&scanout_state->damage,
+               pixman_region32_translate(&scanout_damage,
                                          -output->base.x, -output->base.y);
                weston_transformed_region(output->base.width,
                                          output->base.height,
                                          output->base.transform,
                                          output->base.current_scale,
-                                         &scanout_state->damage,
-                                         &scanout_state->damage);
+                                         &scanout_damage,
+                                         &scanout_damage);
        }
 
-       pixman_region32_subtract(&c->primary_plane.damage,
-                                &c->primary_plane.damage, damage);
+       assert(scanout_state->damage_blob_id == 0);
+
+       rects = pixman_region32_rectangles(&scanout_damage, &n_rects);
+
+       /*
+        * If this function fails, the blob id should still be 0.
+        * This tells the kernel there is no damage information, which means
+        * that it will consider the whole plane damaged. While this may
+        * affect efficiency, it should still produce correct results.
+        */
+       drmModeCreatePropertyBlob(b->drm.fd, rects,
+                                 sizeof(*rects) * n_rects,
+                                 &scanout_state->damage_blob_id);
+
+       pixman_region32_fini(&scanout_damage);
 }
 
 static int

diff --git a/libweston/backend-drm/kms.c b/libweston/backend-drm/kms.c
index 7576c0060d365e3165125e21f5013ff7583b7cbe..b349ec13a6640bde8a90b3f6a1275584f2f8d147 100644 (file)
--- a/libweston/backend-drm/kms.c
+++ b/libweston/backend-drm/kms.c
@@ -847,43 +847,6 @@ plane_add_prop(drmModeAtomicReq *req, struct drm_plane *plane,
        return (ret <= 0) ? -1 : 0;
 }
 
-
-static int
-plane_add_damage(drmModeAtomicReq *req, struct drm_backend *backend,
-                struct drm_plane_state *plane_state)
-{
-       struct drm_plane *plane = plane_state->plane;
-       struct drm_property_info *info =
-               &plane->props[WDRM_PLANE_FB_DAMAGE_CLIPS];
-       pixman_box32_t *rects;
-       uint32_t blob_id;
-       int n_rects;
-       int ret;
-
-       if (!pixman_region32_not_empty(&plane_state->damage))
-               return 0;
-
-       /*
-        * If a plane doesn't support fb damage blob property, kernel will
-        * perform full plane update.
-        */
-       if (info->prop_id == 0)
-               return 0;
-
-       rects = pixman_region32_rectangles(&plane_state->damage, &n_rects);
-
-       ret = drmModeCreatePropertyBlob(backend->drm.fd, rects,
-                                       sizeof(*rects) * n_rects, &blob_id);
-       if (ret != 0)
-               return ret;
-
-       ret = plane_add_prop(req, plane, WDRM_PLANE_FB_DAMAGE_CLIPS, blob_id);
-       if (ret != 0)
-               return ret;
-
-       return 0;
-}
-
 static bool
 drm_head_has_prop(struct drm_head *head,
                  enum wdrm_connector_property prop)
@@ -1043,7 +1006,9 @@ drm_output_apply_state_atomic(struct drm_output_state *state,
                                      plane_state->dest_w);
                ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_H,
                                      plane_state->dest_h);
-               ret |= plane_add_damage(req, b, plane_state);
+               if (plane->props[WDRM_PLANE_FB_DAMAGE_CLIPS].prop_id != 0)
+                       ret |= plane_add_prop(req, plane, WDRM_PLANE_FB_DAMAGE_CLIPS,
+                                             plane_state->damage_blob_id);
 
                if (plane_state->fb && plane_state->fb->format)
                        pinfo = plane_state->fb->format;

diff --git a/libweston/backend-drm/state-helpers.c b/libweston/backend-drm/state-helpers.c
index 7b1d9241d5cefbea330b378173360cfbb494b880..d105499338499c64430f2c0dc79ec6d2c4d00915 100644 (file)
--- a/libweston/backend-drm/state-helpers.c
+++ b/libweston/backend-drm/state-helpers.c
@@ -49,7 +49,6 @@ drm_plane_state_alloc(struct drm_output_state *state_output,
        state->plane = plane;
        state->in_fence_fd = -1;
        state->zpos = DRM_PLANE_ZPOS_INVALID_PLANE;
-       pixman_region32_init(&state->damage);
 
        /* Here we only add the plane state to the desired link, and not
         * set the member. Having an output pointer set means that the
@@ -82,7 +81,15 @@ drm_plane_state_free(struct drm_plane_state *state, bool force)
        state->output_state = NULL;
        state->in_fence_fd = -1;
        state->zpos = DRM_PLANE_ZPOS_INVALID_PLANE;
-       pixman_region32_fini(&state->damage);
+
+       /* Once the damage blob has been submitted, it is refcounted internally
+        * by the kernel, which means we can safely discard it.
+        */
+       if (state->damage_blob_id != 0) {
+               drmModeDestroyPropertyBlob(state->plane->backend->drm.fd,
+                                          state->damage_blob_id);
+               state->damage_blob_id = 0;
+       }
 
        if (force || state != state->plane->state_cur) {
                drm_fb_unref(state->fb);
@@ -99,12 +106,16 @@ struct drm_plane_state *
 drm_plane_state_duplicate(struct drm_output_state *state_output,
                          struct drm_plane_state *src)
 {
-       struct drm_plane_state *dst = malloc(sizeof(*dst));
+       struct drm_plane_state *dst = zalloc(sizeof(*dst));
        struct drm_plane_state *old, *tmp;
 
        assert(src);
        assert(dst);
        *dst = *src;
+       /* We don't want to copy this, because damage is transient, and only
+        * lasts for the duration of a single repaint.
+        */
+       dst->damage_blob_id = 0;
        wl_list_init(&dst->link);
 
        wl_list_for_each_safe(old, tmp, &state_output->plane_list, link) {
@@ -120,7 +131,6 @@ drm_plane_state_duplicate(struct drm_output_state *state_output,
        if (src->fb)
                dst->fb = drm_fb_ref(src->fb);
        dst->output_state = state_output;
-       pixman_region32_init(&dst->damage);
        dst->complete = false;
 
        return dst;