[igt-dev] [PATCH i-g-t 17/35] lib/intel_batchbuffer: Add tracking intel_buf to intel_bb

["Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>]

From now on intel_bb starts tracking added/removed intel_bufs.
We're safe now regardless order of intel_buf close/destroy
or intel_bb destroy paths.

When intel_buf is closed/destroyed first and it was previously added
to intel_bb it calls the code which removes itself from intel_bb.
In destroy path we go over all tracked intel_bufs and clear tracking
information and buffer offset (it is set to INTEL_BUF_INVALID_ADDRESS).

Reset path is handled as follows:
- intel_bb_reset(ibb, false) - just clean objects array leaving
  cache / allocator state intact.
- intel_bb_reset(ibb, true) - purge cache as well as detach intel_bufs
  from intel_bb (release offsets from allocator).

Remove intel_bb_object_offset_to_buf() function as tracking intel_buf
updates (checks for allocator) their offsets after execbuf.

Alter api_intel_bb according to intel-bb changes.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/intel_batchbuffer.c   | 119 +++++++++++++++++++++++---------------
 lib/intel_batchbuffer.h   |   5 ++
 lib/intel_bufops.c        |  13 ++++-
 lib/intel_bufops.h        |   6 ++
 lib/media_spin.c          |   2 -
 tests/i915/api_intel_bb.c |   7 ---
 6 files changed, 96 insertions(+), 56 deletions(-)

diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 2d9c08d6a..b76a8bb87 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -1261,6 +1261,8 @@ static inline uint64_t __intel_bb_get_offset(struct intel_bb *ibb,
  * If we do reset without purging caches we use addresses from intel-bb cache
  * during execbuf objects construction.
  *
+ * If we do reset with purging caches allocator entires are freed as well.
+ *
  * Returns:
  *
  * Pointer the intel_bb, asserts on failure.
@@ -1317,6 +1319,8 @@ __intel_bb_create(int i915, uint32_t ctx, uint32_t size, bool do_relocs,
 				     false);
 	ibb->batch_offset = object->offset;
 
+	IGT_INIT_LIST_HEAD(&ibb->intel_bufs);
+
 	ibb->refcount = 1;
 
 	return ibb;
@@ -1494,6 +1498,15 @@ static void __intel_bb_destroy_cache(struct intel_bb *ibb)
 	ibb->root = NULL;
 }
 
+static void __intel_bb_remove_intel_bufs(struct intel_bb *ibb)
+{
+	struct intel_buf *entry, *tmp;
+
+	igt_list_for_each_entry_safe(entry, tmp, &ibb->intel_bufs, link) {
+		intel_bb_remove_intel_buf(ibb, entry);
+	}
+}
+
 /**
  * intel_bb_destroy:
  * @ibb: pointer to intel_bb
@@ -1507,6 +1520,7 @@ void intel_bb_destroy(struct intel_bb *ibb)
 	ibb->refcount--;
 	igt_assert_f(ibb->refcount == 0, "Trying to destroy referenced bb!");
 
+	__intel_bb_remove_intel_bufs(ibb);
 	__intel_bb_destroy_relocations(ibb);
 	__intel_bb_destroy_objects(ibb);
 	__intel_bb_destroy_cache(ibb);
@@ -1530,6 +1544,10 @@ void intel_bb_destroy(struct intel_bb *ibb)
  * @purge_objects_cache: if true destroy internal execobj and relocs + cache
  *
  * Recreate batch bo when there's no additional reference.
+ *
+ * When purge_object_cache == true we destroy cache as well as remove intel_buf
+ * from intel-bb tracking list. Removing intel_bufs releases their addresses
+ * in the allocator.
 */
 
 void intel_bb_reset(struct intel_bb *ibb, bool purge_objects_cache)
@@ -1555,8 +1573,10 @@ void intel_bb_reset(struct intel_bb *ibb, bool purge_objects_cache)
 	__intel_bb_destroy_objects(ibb);
 	__reallocate_objects(ibb);
 
-	if (purge_objects_cache)
+	if (purge_objects_cache) {
+		__intel_bb_remove_intel_bufs(ibb);
 		__intel_bb_destroy_cache(ibb);
+	}
 
 	/*
 	 * When we use allocators we're in no-reloc mode so we have to free
@@ -1861,22 +1881,13 @@ intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
 
 	object->offset = offset;
 
-	/* Limit current offset to gtt size */
-	if (offset != INTEL_BUF_INVALID_ADDRESS) {
-		object->offset = CANONICAL(offset & (ibb->gtt_size - 1));
-	} else {
-		object->offset = __intel_bb_get_offset(ibb,
-						       handle, size,
-						       object->alignment);
-	}
-
 	if (write)
 		object->flags |= EXEC_OBJECT_WRITE;
 
 	if (ibb->supports_48b_address)
 		object->flags |= EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 
-	if (ibb->uses_full_ppgtt && ibb->allocator_type == INTEL_ALLOCATOR_SIMPLE)
+	if (ibb->uses_full_ppgtt && !ibb->enforce_relocs)
 		object->flags |= EXEC_OBJECT_PINNED;
 
 	return object;
@@ -1913,6 +1924,9 @@ __intel_bb_add_intel_buf(struct intel_bb *ibb, struct intel_buf *buf,
 {
 	struct drm_i915_gem_exec_object2 *obj;
 
+	igt_assert(ibb);
+	igt_assert(buf);
+	igt_assert(!buf->ibb || buf->ibb == ibb);
 	igt_assert(ALIGN(alignment, 4096) == alignment);
 
 	if (!alignment) {
@@ -1937,6 +1951,13 @@ __intel_bb_add_intel_buf(struct intel_bb *ibb, struct intel_buf *buf,
 	if (!ibb->enforce_relocs)
 		obj->alignment = alignment;
 
+	if (igt_list_empty(&buf->link)) {
+		igt_list_add_tail(&buf->link, &ibb->intel_bufs);
+		buf->ibb = ibb;
+	} else {
+		igt_assert(buf->ibb == ibb);
+	}
+
 	return obj;
 }
 
@@ -1955,16 +1976,36 @@ intel_bb_add_intel_buf_with_alignment(struct intel_bb *ibb, struct intel_buf *bu
 
 bool intel_bb_remove_intel_buf(struct intel_bb *ibb, struct intel_buf *buf)
 {
-	bool removed = intel_bb_remove_object(ibb, buf->handle,
-					      buf->addr.offset,
-					      intel_buf_bo_size(buf));
+	bool removed;
+
+	igt_assert(ibb);
+	igt_assert(buf);
+	igt_assert(!buf->ibb || buf->ibb == ibb);
 
-	if (removed)
+	removed = intel_bb_remove_object(ibb, buf->handle,
+					 buf->addr.offset,
+					 intel_buf_bo_size(buf));
+
+	if (removed && !igt_list_empty(&buf->link)) {
 		buf->addr.offset = INTEL_BUF_INVALID_ADDRESS;
+		buf->ibb = NULL;
+		igt_list_del_init(&buf->link);
+	}
 
 	return removed;
 }
 
+void intel_bb_intel_buf_list(struct intel_bb *ibb)
+{
+	struct intel_buf *entry;
+
+	igt_list_for_each_entry(entry, &ibb->intel_bufs, link) {
+		igt_info("handle: %u, ibb: %p, offset: %lx\n",
+			 entry->handle, entry->ibb,
+			 (long) entry->addr.offset);
+	}
+}
+
 struct drm_i915_gem_exec_object2 *
 intel_bb_find_object(struct intel_bb *ibb, uint32_t handle)
 {
@@ -2347,6 +2388,7 @@ static void update_offsets(struct intel_bb *ibb,
 			   struct drm_i915_gem_exec_object2 *objects)
 {
 	struct drm_i915_gem_exec_object2 *object;
+	struct intel_buf *entry;
 	uint32_t i;
 
 	for (i = 0; i < ibb->num_objects; i++) {
@@ -2358,11 +2400,23 @@ static void update_offsets(struct intel_bb *ibb,
 		if (i == 0)
 			ibb->batch_offset = object->offset;
 	}
+
+	igt_list_for_each_entry(entry, &ibb->intel_bufs, link) {
+		object = intel_bb_find_object(ibb, entry->handle);
+		igt_assert(object);
+
+		if (ibb->allocator_type == INTEL_ALLOCATOR_SIMPLE)
+			igt_assert(object->offset == entry->addr.offset);
+		else
+			entry->addr.offset = object->offset;
+
+		entry->addr.ctx = ibb->ctx;
+	}
 }
 
 #define LINELEN 76
 /*
- * @__intel_bb_exec:
+ * __intel_bb_exec:
  * @ibb: pointer to intel_bb
  * @end_offset: offset of the last instruction in the bb
  * @flags: flags passed directly to execbuf
@@ -2402,6 +2456,9 @@ static int __intel_bb_exec(struct intel_bb *ibb, uint32_t end_offset,
 	if (ibb->dump_base64)
 		intel_bb_dump_base64(ibb, LINELEN);
 
+	/* For debugging on CI, remove in final series */
+	intel_bb_dump_execbuf(ibb, &execbuf);
+
 	ret = __gem_execbuf_wr(ibb->i915, &execbuf);
 	if (ret) {
 		intel_bb_dump_execbuf(ibb, &execbuf);
@@ -2479,36 +2536,6 @@ uint64_t intel_bb_get_object_offset(struct intel_bb *ibb, uint32_t handle)
 	return (*found)->offset;
 }
 
-/**
- * intel_bb_object_offset_to_buf:
- * @ibb: pointer to intel_bb
- * @buf: buffer we want to store last exec offset and context id
- *
- * Copy object offset used in the batch to intel_buf to allow caller prepare
- * other batch likely without relocations.
- */
-bool intel_bb_object_offset_to_buf(struct intel_bb *ibb, struct intel_buf *buf)
-{
-	struct drm_i915_gem_exec_object2 object = { .handle = buf->handle };
-	struct drm_i915_gem_exec_object2 **found;
-
-	igt_assert(ibb);
-	igt_assert(buf);
-
-	found = tfind((void *)&object, &ibb->root, __compare_objects);
-	if (!found) {
-		buf->addr.offset = 0;
-		buf->addr.ctx = ibb->ctx;
-
-		return false;
-	}
-
-	buf->addr.offset = (*found)->offset & (ibb->gtt_size - 1);
-	buf->addr.ctx = ibb->ctx;
-
-	return true;
-}
-
 /*
  * intel_bb_emit_bbe:
  * @ibb: batchbuffer
diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
index 702052d22..f8a38967b 100644
--- a/lib/intel_batchbuffer.h
+++ b/lib/intel_batchbuffer.h
@@ -6,6 +6,7 @@
 #include <i915_drm.h>
 
 #include "igt_core.h"
+#include "igt_list.h"
 #include "intel_reg.h"
 #include "drmtest.h"
 #include "intel_allocator.h"
@@ -481,6 +482,9 @@ struct intel_bb {
 	uint32_t num_relocs;
 	uint32_t allocated_relocs;
 
+	/* Tracked intel_bufs */
+	struct igt_list_head intel_bufs;
+
 	/*
 	 * BO recreate in reset path only when refcount == 0
 	 * Currently we don't need to use atomics because intel_bb
@@ -598,6 +602,7 @@ struct drm_i915_gem_exec_object2 *
 intel_bb_add_intel_buf_with_alignment(struct intel_bb *ibb, struct intel_buf *buf,
 				      uint64_t alignment, bool write);
 bool intel_bb_remove_intel_buf(struct intel_bb *ibb, struct intel_buf *buf);
+void intel_bb_intel_buf_list(struct intel_bb *ibb);
 struct drm_i915_gem_exec_object2 *
 intel_bb_find_object(struct intel_bb *ibb, uint32_t handle);
 
diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
index d8eb64e3a..166a957f1 100644
--- a/lib/intel_bufops.c
+++ b/lib/intel_bufops.c
@@ -727,6 +727,7 @@ static void __intel_buf_init(struct buf_ops *bops,
 
 	buf->bops = bops;
 	buf->addr.offset = INTEL_BUF_INVALID_ADDRESS;
+	IGT_INIT_LIST_HEAD(&buf->link);
 
 	if (compression) {
 		int aux_width, aux_height;
@@ -822,13 +823,23 @@ void intel_buf_init(struct buf_ops *bops,
  *
  * 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.
+ * doesn't close it in close()/destroy() paths. When intel_buf was previously
+ * added to intel_bb (intel_bb_add_intel_buf() call) it is tracked there and
+ * must be removed from its internal structures.
  */
 void intel_buf_close(struct buf_ops *bops, struct intel_buf *buf)
 {
 	igt_assert(bops);
 	igt_assert(buf);
 
+	/* If buf is tracked by some intel_bb ensure it will be removed there */
+	if (buf->ibb) {
+		intel_bb_remove_intel_buf(buf->ibb, buf);
+		buf->addr.offset = INTEL_BUF_INVALID_ADDRESS;
+		buf->ibb = NULL;
+		IGT_INIT_LIST_HEAD(&buf->link);
+	}
+
 	if (buf->is_owner)
 		gem_close(bops->fd, buf->handle);
 }
diff --git a/lib/intel_bufops.h b/lib/intel_bufops.h
index 54480bff6..1a3d86925 100644
--- a/lib/intel_bufops.h
+++ b/lib/intel_bufops.h
@@ -2,6 +2,7 @@
 #define __INTEL_BUFOPS_H__
 
 #include <stdint.h>
+#include "igt_list.h"
 #include "igt_aux.h"
 #include "intel_batchbuffer.h"
 
@@ -13,6 +14,7 @@ struct buf_ops;
 
 struct intel_buf {
 	struct buf_ops *bops;
+
 	bool is_owner;
 	uint32_t handle;
 	uint64_t size;
@@ -40,6 +42,10 @@ struct intel_buf {
 		uint32_t ctx;
 	} addr;
 
+	/* Tracking */
+	struct intel_bb *ibb;
+	struct igt_list_head link;
+
 	/* CPU mapping */
 	uint32_t *ptr;
 	bool cpu_write;
diff --git a/lib/media_spin.c b/lib/media_spin.c
index 5da469a52..d2345d153 100644
--- a/lib/media_spin.c
+++ b/lib/media_spin.c
@@ -132,7 +132,6 @@ gen8_media_spinfunc(int i915, struct intel_buf *buf, uint32_t spins)
 	intel_bb_exec(ibb, intel_bb_offset(ibb),
 		      I915_EXEC_DEFAULT | I915_EXEC_NO_RELOC, false);
 
-	intel_bb_object_offset_to_buf(ibb, buf);
 	intel_bb_destroy(ibb);
 }
 
@@ -186,6 +185,5 @@ gen9_media_spinfunc(int i915, struct intel_buf *buf, uint32_t spins)
 	intel_bb_exec(ibb, intel_bb_offset(ibb),
 		      I915_EXEC_DEFAULT | I915_EXEC_NO_RELOC, false);
 
-	intel_bb_object_offset_to_buf(ibb, buf);
 	intel_bb_destroy(ibb);
 }
diff --git a/tests/i915/api_intel_bb.c b/tests/i915/api_intel_bb.c
index c6c943506..77dfb6854 100644
--- a/tests/i915/api_intel_bb.c
+++ b/tests/i915/api_intel_bb.c
@@ -828,9 +828,6 @@ static void offset_control(struct buf_ops *bops)
 		print_buf(dst2, "dst2");
 	}
 
-	igt_assert(intel_bb_object_offset_to_buf(ibb, src) == true);
-	igt_assert(intel_bb_object_offset_to_buf(ibb, dst1) == true);
-	igt_assert(intel_bb_object_offset_to_buf(ibb, dst2) == true);
 	poff_src = src->addr.offset;
 	poff_dst1 = dst1->addr.offset;
 	poff_dst2 = dst2->addr.offset;
@@ -853,10 +850,6 @@ static void offset_control(struct buf_ops *bops)
 		      I915_EXEC_DEFAULT | I915_EXEC_NO_RELOC, false);
 	intel_bb_sync(ibb);
 
-	igt_assert(intel_bb_object_offset_to_buf(ibb, src) == true);
-	igt_assert(intel_bb_object_offset_to_buf(ibb, dst1) == true);
-	igt_assert(intel_bb_object_offset_to_buf(ibb, dst2) == true);
-	igt_assert(intel_bb_object_offset_to_buf(ibb, dst3) == true);
 	igt_assert(poff_src == src->addr.offset);
 	igt_assert(poff_dst1 == dst1->addr.offset);
 	igt_assert(poff_dst2 == dst2->addr.offset);
-- 
2.26.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev