[igt-dev] [PATCH i-g-t v34 39/40] lib/intel_bufops: address review comments (clarify buffer ownership rules)
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Fri Sep 11 08:07:56 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 3cfabb7a..26021da8 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 97bfd324..f3d55688 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