[igt-dev] [PATCH i-g-t v33 40/40] lib/intel_bufops: address review comments (clarify buffer ownership rules)

Zbigniew Kempczyński zbigniew.kempczynski at intel.com
Mon Aug 31 13:30:49 UTC 2020


To avoid intel_buf handle leakage we need to clarify of buffer ownership
rules. So we currently have:

1. intel_buf_init(), intel_buf_create() which create handle and take
   ownership of such handle.
2. intel_buf_init_using_handle(), intel_buf_create_using_handle() which
   take bo handle from the caller and doesn't take ownership.

intel_buf_close()/intel_buf_destroy() will honour ownership and skip
closing handle if buffer is not owned.

To take/release buffer ownership intel_buf_set_ownership() can be used.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
---
 lib/igt_draw.c                  | 14 +++---
 lib/igt_fb.c                    | 15 ++++---
 lib/intel_bufops.c              | 75 ++++++++++++++++++++++++++++++---
 lib/intel_bufops.h              | 14 ++++++
 tests/i915/gem_concurrent_all.c | 41 ++++++++++--------
 tests/kms_big_fb.c              | 33 ++++++++-------
 tests/kms_psr.c                 |  6 ++-
 7 files changed, 144 insertions(+), 54 deletions(-)

diff --git a/lib/igt_draw.c b/lib/igt_draw.c
index 462738ef..7a30340f 100644
--- a/lib/igt_draw.c
+++ b/lib/igt_draw.c
@@ -508,19 +508,21 @@ static void draw_rect_pwrite(int fd, struct buf_data *buf,
 static struct intel_buf *create_buf(int fd, struct buf_ops *bops,
 				    struct buf_data *from, uint32_t tiling)
 {
-	struct intel_buf *buf = calloc(sizeof(*buf), 1);
+	struct intel_buf *buf;
 	uint32_t handle, name, width, height;
 
-	igt_assert(buf);
-
 	width = from->stride / (from->bpp / 8);
 	height = from->size / from->stride;
 
 	name = gem_flink(fd, from->handle);
 	handle = gem_open(fd, name);
-	intel_buf_init_using_handle(bops, handle, buf,
-				    width, height, from->bpp, 0,
-				    tiling, 0);
+
+	buf = intel_buf_create_using_handle(bops, handle,
+					    width, height, from->bpp, 0,
+					    tiling, 0);
+
+	/* Make sure we close handle on destroy path */
+	intel_buf_set_ownership(buf, true);
 
 	return buf;
 }
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 748c9c5a..813341ee 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2106,14 +2106,17 @@ static struct intel_buf *create_buf(struct fb_blit_upload *blit,
 
 	bo_name = gem_flink(blit->fd, fb->gem_handle);
 	handle = gem_open(blit->fd, bo_name);
-	buf = calloc(1, sizeof(*buf));
-	igt_assert(buf);
-	intel_buf_init_using_handle(blit->bops, handle, buf, fb->width,
-				    fb->height, fb->plane_bpp[0], 0,
-				    igt_fb_mod_to_tiling(fb->modifier),
-				    compression);
+
+	buf = intel_buf_create_using_handle(blit->bops, handle,
+					    fb->width, fb->height,
+					    fb->plane_bpp[0], 0,
+					    igt_fb_mod_to_tiling(fb->modifier),
+					    compression);
 	intel_buf_set_name(buf, name);
 
+	/* Make sure we close handle on destroy path */
+	intel_buf_set_ownership(buf, true);
+
 	buf->format_is_yuv = igt_format_is_yuv(fb->drm_format);
 	buf->format_is_yuv_semiplanar =
 		igt_format_is_yuv_semiplanar(fb->drm_format);
diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
index 714e1aaa..57aa2a40 100644
--- a/lib/intel_bufops.c
+++ b/lib/intel_bufops.c
@@ -786,13 +786,13 @@ static void __intel_buf_init(struct buf_ops *bops,
  * @buf: pointer to intel_buf structure to be filled
  * @width: surface width
  * @height: surface height
- * @bpp: bits-per-pixel (8 / 16 / 32)
+ * @bpp: bits-per-pixel (8 / 16 / 32 / 64)
  * @alignment: alignment of the stride for linear surfaces
  * @tiling: surface tiling
  * @compression: surface compression type
  *
  * Function creates new BO within intel_buf structure and fills all
- * structure fields.
+ * structure fields. Takes bo handle ownership.
  *
  * Note. For X / Y if GPU supports fences HW tiling is configured.
  */
@@ -803,6 +803,8 @@ void intel_buf_init(struct buf_ops *bops,
 {
 	__intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
 			 tiling, compression);
+
+	intel_buf_set_ownership(buf, true);
 }
 
 /**
@@ -810,14 +812,17 @@ void intel_buf_init(struct buf_ops *bops,
  * @bops: pointer to buf_ops
  * @buf: pointer to intel_buf structure
  *
- * Function closes gem BO inside intel_buf.
+ * Function closes gem BO inside intel_buf if bo is owned by intel_buf.
+ * For handle passed from the caller intel_buf doesn't take ownership and
+ * doesn't close it in close()/destroy() paths.
  */
 void intel_buf_close(struct buf_ops *bops, struct intel_buf *buf)
 {
 	igt_assert(bops);
 	igt_assert(buf);
 
-	gem_close(bops->fd, buf->handle);
+	if (buf->is_owner)
+		gem_close(bops->fd, buf->handle);
 }
 
 /**
@@ -827,7 +832,7 @@ void intel_buf_close(struct buf_ops *bops, struct intel_buf *buf)
  * @buf: pointer to intel_buf structure to be filled
  * @width: surface width
  * @height: surface height
- * @bpp: bits-per-pixel (8 / 16 / 32)
+ * @bpp: bits-per-pixel (8 / 16 / 32 / 64)
  * @alignment: alignment of the stride for linear surfaces
  * @tiling: surface tiling
  * @compression: surface compression type
@@ -836,8 +841,8 @@ void intel_buf_close(struct buf_ops *bops, struct intel_buf *buf)
  * (with all its metadata - width, height, ...). Useful if BO was created
  * outside.
  *
- * Note: intel_buf_close() can be used to close the BO handle, but caller
- * must be aware to not close the BO twice.
+ * Note: intel_buf_close() can be used because intel_buf is aware it is not
+ * buffer owner so it won't close it underneath.
  */
 void intel_buf_init_using_handle(struct buf_ops *bops,
 				 uint32_t handle,
@@ -849,6 +854,19 @@ void intel_buf_init_using_handle(struct buf_ops *bops,
 			 req_tiling, compression);
 }
 
+/**
+ * intel_buf_create
+ * @bops: pointer to buf_ops
+ * @width: surface width
+ * @height: surface height
+ * @bpp: bits-per-pixel (8 / 16 / 32 / 64)
+ * @alignment: alignment of the stride for linear surfaces
+ * @tiling: surface tiling
+ * @compression: surface compression type
+ *
+ * Function creates intel_buf with created BO handle. Takes ownership of the
+ * buffer.
+ */
 struct intel_buf *intel_buf_create(struct buf_ops *bops,
 				   int width, int height,
 				   int bpp, int alignment,
@@ -867,12 +885,55 @@ struct intel_buf *intel_buf_create(struct buf_ops *bops,
 	return buf;
 }
 
+/**
+ * intel_buf_create_using_handle
+ * @bops: pointer to buf_ops
+ * @handle: BO handle created by the caller
+ * @width: surface width
+ * @height: surface height
+ * @bpp: bits-per-pixel (8 / 16 / 32 / 64)
+ * @alignment: alignment of the stride for linear surfaces
+ * @tiling: surface tiling
+ * @compression: surface compression type
+ *
+ * Function creates intel_buf with passed BO handle from the caller. Doesn't
+ * take ownership of the buffer. close()/destroy() paths doesn't close
+ * passed handle unless buffer will take ownership using set_ownership().
+ */
+struct intel_buf *intel_buf_create_using_handle(struct buf_ops *bops,
+						uint32_t handle,
+						int width, int height,
+						int bpp, int alignment,
+						uint32_t req_tiling,
+						uint32_t compression)
+{
+	struct intel_buf *buf;
+
+	igt_assert(bops);
+
+	buf = calloc(1, sizeof(*buf));
+	igt_assert(buf);
+
+	intel_buf_init_using_handle(bops, handle, buf, width, height, bpp,
+				    alignment, req_tiling, compression);
+
+	return buf;
+}
+
+/**
+ * intel_buf_destroy
+ * @buf: intel_buf
+ *
+ * Function frees intel_buf memory. It closes bo handle if intel_buf has
+ * buffer ownership.
+ */
 void intel_buf_destroy(struct intel_buf *buf)
 {
 	igt_assert(buf);
 	igt_assert(buf->ptr == NULL);
 
 	intel_buf_close(buf->bops, buf);
+
 	free(buf);
 }
 
diff --git a/lib/intel_bufops.h b/lib/intel_bufops.h
index 0a8536ba..8debe7f2 100644
--- a/lib/intel_bufops.h
+++ b/lib/intel_bufops.h
@@ -11,6 +11,7 @@ struct buf_ops;
 #define INTEL_BUF_NAME_MAXSIZE 32
 struct intel_buf {
 	struct buf_ops *bops;
+	bool is_owner;
 	uint32_t handle;
 	uint32_t tiling;
 	uint32_t bpp;
@@ -107,6 +108,11 @@ void linear_to_intel_buf(struct buf_ops *bops, struct intel_buf *buf,
 bool buf_ops_has_hw_fence(struct buf_ops *bops, uint32_t tiling);
 bool buf_ops_has_tiling_support(struct buf_ops *bops, uint32_t tiling);
 
+static inline void intel_buf_set_ownership(struct intel_buf *buf, bool is_owner)
+{
+	buf->is_owner = is_owner;
+}
+
 void intel_buf_init(struct buf_ops *bops, struct intel_buf *buf,
 		    int width, int height, int bpp, int alignment,
 		    uint32_t tiling, uint32_t compression);
@@ -122,6 +128,14 @@ struct intel_buf *intel_buf_create(struct buf_ops *bops,
 				   int width, int height,
 				   int bpp, int alignment,
 				   uint32_t req_tiling, uint32_t compression);
+
+struct intel_buf *intel_buf_create_using_handle(struct buf_ops *bops,
+						uint32_t handle,
+						int width, int height,
+						int bpp, int alignment,
+						uint32_t req_tiling,
+						uint32_t compression);
+
 void intel_buf_destroy(struct intel_buf *buf);
 
 void *intel_buf_cpu_map(struct intel_buf *buf, bool write);
diff --git a/tests/i915/gem_concurrent_all.c b/tests/i915/gem_concurrent_all.c
index 48b0315d..3b17ffd9 100644
--- a/tests/i915/gem_concurrent_all.c
+++ b/tests/i915/gem_concurrent_all.c
@@ -157,7 +157,7 @@ static void can_create_normal(const struct create *create, unsigned count)
 
 #if HAVE_CREATE_PRIVATE
 static struct intel_buf *
-create_private_bo(buf_ops *bops, uint32_t width, uint32_t height,
+create_private_bo(struct buf_ops *bops, uint32_t width, uint32_t height,
 		  uint32_t tiling, uint64_t size)
 {
 	struct intel_buf *buf;
@@ -170,9 +170,10 @@ create_private_bo(buf_ops *bops, uint32_t width, uint32_t height,
 	name = gem_flink(fd, handle);
 	buf_handle = gem_open(fd, name);
 
-	buf = calloc(1, sizeof(*buf));
-	intel_buf_init_using_handle(bops, buf_handle, buf, width, height,
-				    bpp, 0, tiling, 0);
+	buf = intel_buf_create_using_handle(bops, buf_handle,
+					    width, height, bpp, 0, tiling, 0);
+	intel_buf_set_ownership(buf, true);
+
 	gem_close(fd, handle);
 
 	return buf;
@@ -186,7 +187,7 @@ static void can_create_private(const struct create *create, unsigned count)
 
 #if HAVE_CREATE_STOLEN
 static struct intel_buf *
-create_stolen_bo(buf_ops *bops, uint32_t width, uint32_t height,
+create_stolen_bo(struct buf_ops *bops, uint32_t width, uint32_t height,
 		 uint32_t tiling, uint64_t size)
 {
 	struct intel_buf *buf;
@@ -199,9 +200,10 @@ create_stolen_bo(buf_ops *bops, uint32_t width, uint32_t height,
 	name = gem_flink(fd, handle);
 	buf_handle = gem_open(fd, name);
 
-	buf = calloc(1, sizeof(*buf));
-	intel_buf_init_using_handle(bops, buf_handle, buf, width, height,
-				    bpp, 0, tiling, 0);
+	buf = intel_buf_create_using_handle(bops, buf_handle,
+					    width, height, bpp, 0, tiling, 0);
+	intel_buf_set_ownership(buf, true);
+
 	gem_close(fd, handle);
 
 	return buf;
@@ -293,9 +295,10 @@ userptr_create_bo(const struct buffers *b)
 	userptr.user_ptr = to_user_pointer(ptr);
 
 	do_or_die(drmIoctl(fd, DRM_IOCTL_I915_GEM_USERPTR, &userptr));
-	buf = calloc(1, sizeof(*buf));
-	intel_buf_init_using_handle(b->bops, userptr.handle, buf, b->width,
-				    b->height, 32, 0, I915_TILING_NONE, 0);
+	buf = intel_buf_create_using_handle(b->bops, userptr.handle,
+					    b->width, b->height, 32, 0,
+					    I915_TILING_NONE, 0);
+	intel_buf_set_ownership(buf, true);
 
 	buf->ptr = (void *) from_user_pointer(userptr.user_ptr);
 
@@ -332,6 +335,7 @@ userptr_release_bo(struct intel_buf *buf)
 	igt_assert(buf->ptr);
 
 	munmap(buf->ptr, buf->surface[0].size);
+	buf->ptr = NULL;
 
 	intel_buf_destroy(buf);
 }
@@ -390,9 +394,10 @@ dmabuf_create_bo(const struct buffers *b)
 	igt_assert(args.fd != -1);
 
 	handle = prime_fd_to_handle(buf_ops_get_fd(b->bops), args.fd);
-	buf = calloc(1, sizeof(*buf));
-	intel_buf_init_using_handle(b->bops, handle, buf, b->width,
-				    b->height, 32, 0, I915_TILING_NONE, 0);
+	buf = intel_buf_create_using_handle(b->bops, handle,
+					    b->width, b->height, 32, 0,
+					    I915_TILING_NONE, 0);
+	intel_buf_set_ownership(buf, true);
 
 	dmabuf = malloc(sizeof(*dmabuf));
 	igt_assert(dmabuf);
@@ -495,10 +500,10 @@ vgem_create_bo(const struct buffers *b)
 	igt_assert(args.fd != -1);
 
 	handle = prime_fd_to_handle(buf_ops_get_fd(b->bops), args.fd);
-	buf = calloc(1, sizeof(*buf));
-	intel_buf_init_using_handle(b->bops, handle, buf,
-				    vgem.width, vgem.height, vgem.bpp,
-				    0, I915_TILING_NONE, 0);
+	buf = intel_buf_create_using_handle(b->bops, handle,
+					    vgem.width, vgem.height, vgem.bpp,
+					    0, I915_TILING_NONE, 0);
+	intel_buf_set_ownership(buf, true);
 
 	dmabuf = malloc(sizeof(*dmabuf));
 	igt_assert(dmabuf);
diff --git a/tests/kms_big_fb.c b/tests/kms_big_fb.c
index a9907ea3..175ffa88 100644
--- a/tests/kms_big_fb.c
+++ b/tests/kms_big_fb.c
@@ -50,11 +50,11 @@ typedef struct {
 	struct intel_bb *ibb;
 } data_t;
 
-static void init_buf(data_t *data,
-		     struct intel_buf *buf,
-		     const struct igt_fb *fb,
-		     const char *buf_name)
+static struct intel_buf *init_buf(data_t *data,
+				  const struct igt_fb *fb,
+				  const char *buf_name)
 {
+	struct intel_buf *buf;
 	uint32_t name, handle, tiling, stride, width, height, bpp, size;
 
 	igt_assert_eq(fb->offsets[0], 0);
@@ -68,14 +68,17 @@ static void init_buf(data_t *data,
 
 	name = gem_flink(data->drm_fd, fb->gem_handle);
 	handle = gem_open(data->drm_fd, name);
-	intel_buf_init_using_handle(data->bops, handle, buf, width, height, bpp,
-				    0, tiling, 0);
+	buf = intel_buf_create_using_handle(data->bops, handle, width, height,
+					    bpp, 0, tiling, 0);
 	intel_buf_set_name(buf, buf_name);
+	intel_buf_set_ownership(buf, true);
+
+	return buf;
 }
 
 static void fini_buf(struct intel_buf *buf)
 {
-	intel_buf_close(buf->bops, buf);
+	intel_buf_destroy(buf);
 }
 
 static void copy_pattern(data_t *data,
@@ -83,10 +86,10 @@ static void copy_pattern(data_t *data,
 			 struct igt_fb *src_fb, int sx, int sy,
 			 int w, int h)
 {
-	struct intel_buf src = {}, dst = {};
+	struct intel_buf *src, *dst;
 
-	init_buf(data, &src, src_fb, "big fb src");
-	init_buf(data, &dst, dst_fb, "big fb dst");
+	src = init_buf(data, src_fb, "big fb src");
+	dst = init_buf(data, dst_fb, "big fb dst");
 
 	gem_set_domain(data->drm_fd, dst_fb->gem_handle,
 		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
@@ -99,7 +102,7 @@ static void copy_pattern(data_t *data,
 	 * rendered with the blitter/render engine.
 	 */
 	if (data->render_copy) {
-		data->render_copy(data->ibb, 0, &src, sx, sy, w, h, &dst, dx, dy);
+		data->render_copy(data->ibb, 0, src, sx, sy, w, h, dst, dx, dy);
 	} else {
 		w = min(w, src_fb->width - sx);
 		w = min(w, dst_fb->width - dx);
@@ -107,12 +110,12 @@ static void copy_pattern(data_t *data,
 		h = min(h, src_fb->height - sy);
 		h = min(h, dst_fb->height - dy);
 
-		intel_bb_blt_copy(data->ibb, &src, sx, sy, src.surface[0].stride,
-				  &dst, dx, dy, dst.surface[0].stride, w, h, dst.bpp);
+		intel_bb_blt_copy(data->ibb, src, sx, sy, src->surface[0].stride,
+				  dst, dx, dy, dst->surface[0].stride, w, h, dst->bpp);
 	}
 
-	fini_buf(&dst);
-	fini_buf(&src);
+	fini_buf(dst);
+	fini_buf(src);
 
 	/* intel_bb cache doesn't know when objects dissappear, so
 	 * let's purge the cache */
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 7343e270..cd158e29 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -150,8 +150,10 @@ static struct intel_buf *create_buf_from_fb(data_t *data,
 
 	name = gem_flink(data->drm_fd, fb->gem_handle);
 	handle = gem_open(data->drm_fd, name);
-	intel_buf_init_using_handle(data->bops, handle, buf,
-				    width, height, bpp, 0, tiling, 0);
+	buf = intel_buf_create_using_handle(data->bops, handle,
+					    width, height, bpp, 0, tiling, 0);
+	intel_buf_set_ownership(buf, true);
+
 	return buf;
 }
 
-- 
2.26.0



More information about the igt-dev mailing list