[Intel-gfx] [PATCH v2 2/4] drm/i915/selftests: exercise GPU access from the importer

[Ruhl, Michael J]

>-----Original Message-----
>From: Auld, Matthew <matthew.auld at intel.com>
>Sent: Friday, October 28, 2022 11:50 AM
>To: intel-gfx at lists.freedesktop.org
>Cc: Landwerlin, Lionel G <lionel.g.landwerlin at intel.com>; Tvrtko Ursulin
><tvrtko.ursulin at linux.intel.com>; Ville Syrjälä <ville.syrjala at linux.intel.com>;
>Ruhl, Michael J <michael.j.ruhl at intel.com>
>Subject: [PATCH v2 2/4] drm/i915/selftests: exercise GPU access from the
>importer
>
>Using PAGE_SIZE here potentially hides issues so bump that to something
>larger. This should also make it possible for iommu to coalesce entries
>for us. With that in place verify we can write from the GPU using the
>importers sg_table, followed by checking that our writes match when read
>from the CPU side.
>
>v2: Switch over to igt_gpu_fill_dw(), which looks to be more widely
>supported than the migrate stuff (at least OOTB).
>
>References: https://gitlab.freedesktop.org/drm/intel/-/issues/7306
>Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>Cc: Michael J. Ruhl <michael.j.ruhl at intel.com>
>---
> .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 79
>++++++++++++++++++-
> 1 file changed, 78 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>index f2f3cfad807b..e57f9390076c 100644
>--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>@@ -6,8 +6,12 @@
>
> #include "i915_drv.h"
> #include "i915_selftest.h"
>+#include "gem/i915_gem_context.h"
>
>+#include "mock_context.h"
> #include "mock_dmabuf.h"
>+#include "igt_gem_utils.h"
>+#include "selftests/mock_drm.h"
> #include "selftests/mock_gem_device.h"
>
> static int igt_dmabuf_export(void *arg)
>@@ -140,6 +144,75 @@ static int
>igt_dmabuf_import_same_driver_lmem(void *arg)
> 	return err;
> }
>
>+static int verify_access(struct drm_i915_private *i915,
>+			 struct drm_i915_gem_object *native_obj,
>+			 struct drm_i915_gem_object *import_obj)
>+{
>+	struct i915_gem_engines_iter it;
>+	struct i915_gem_context *ctx;
>+	struct intel_context *ce;
>+	struct i915_vma *vma;
>+	struct file *file;
>+	u32 *vaddr;
>+	int err = 0, i;
>+
>+	file = mock_file(i915);
>+	if (IS_ERR(file))
>+		return PTR_ERR(file);
>+
>+	ctx = live_context(i915, file);
>+	if (IS_ERR(ctx)) {
>+		err = PTR_ERR(ctx);
>+		goto out_file;
>+	}
>+
>+	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>+		if (intel_engine_can_store_dword(ce->engine))
>+			break;
>+	}
>+	i915_gem_context_unlock_engines(ctx);
>+	if (!ce)
>+		goto out_file;
>+
>+	vma = i915_vma_instance(import_obj, ce->vm, NULL);
>+	if (IS_ERR(vma)) {
>+		err = PTR_ERR(vma);
>+		goto out_file;
>+	}
>+
>+	err = i915_vma_pin(vma, 0, 0, PIN_USER);
>+	if (err)
>+		goto out_file;
>+
>+	err = igt_gpu_fill_dw(ce, vma, 0,
>+			      vma->size >> PAGE_SHIFT, 0xdeadbeaf);
>+	i915_vma_unpin(vma);
>+	if (err)
>+		goto out_file;
>+
>+	err = i915_gem_object_wait(import_obj, 0,
>MAX_SCHEDULE_TIMEOUT);
>+	if (err)
>+		goto out_file;
>+
>+	vaddr = i915_gem_object_pin_map_unlocked(native_obj,
>I915_MAP_WB);
>+	if (IS_ERR(vaddr)) {
>+		err = PTR_ERR(vaddr);
>+		goto out_file;
>+	}
>+
>+	for (i = 0; i < native_obj->base.size / sizeof(u32); i += PAGE_SIZE /
>sizeof(u32)) {
>+		if (vaddr[i] != 0xdeadbeaf) {
>+			pr_err("Data mismatch [%d]=%u\n", i, vaddr[i]);
>+			err = -EINVAL;
>+			goto out_file;
>+		}

Not sure what timing issues are related to this test, but this loop could have
some impact (takes a long time, assuming the object is LMEM).

Would checking the beginning, middle and end of each page be any less
beneficial than the current check?

Not a suggested change, but just a thought to ponder if the timing becomes
an issue...

This test looks reasonable to me.

Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>

M

>+	}
>+
>+out_file:
>+	fput(file);
>+	return err;
>+}
>+
> static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
> 					 struct intel_memory_region
>**regions,
> 					 unsigned int num_regions)
>@@ -154,7 +227,7 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
>
> 	force_different_devices = true;
>
>-	obj = __i915_gem_object_create_user(i915, PAGE_SIZE,
>+	obj = __i915_gem_object_create_user(i915, SZ_8M,
> 					    regions, num_regions);
> 	if (IS_ERR(obj)) {
> 		pr_err("__i915_gem_object_create_user failed with
>err=%ld\n",
>@@ -206,6 +279,10 @@ static int igt_dmabuf_import_same_driver(struct
>drm_i915_private *i915,
>
> 	i915_gem_object_unlock(import_obj);
>
>+	err = verify_access(i915, obj, import_obj);
>+	if (err)
>+		goto out_import;
>+
> 	/* Now try a fake an importer */
> 	import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev);
> 	if (IS_ERR(import_attach)) {
>--
>2.37.3