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>

4 files changed, 53 insertions, 51 deletions

diff --git a/libweston/backend-drm/drm-internal.h b/libweston/backend-drm/drm-internal.h
index 855baf15..49ce7ce0 100644
--- 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 ca0f3a86..4e5f39cf 100644
--- 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 7576c006..b349ec13 100644
--- 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 7b1d9241..d1054993 100644
--- 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;