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

[Matthew Auld <matthew.auld@intel.com>]

On 28/10/2022 17:10, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: Auld, Matthew <matthew.auld@intel.com>
>> Sent: Friday, October 28, 2022 11:50 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>;
>> Ruhl, Michael J <michael.j.ruhl@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@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Michael J. Ruhl <michael.j.ruhl@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?

We are currently just sampling the first dword of each page (that could 
perhaps be random instead). We could go further and sample a selection 
of random pages instead, if speed is a concern. Although here the pages 
must be in system memory. dma-buf in upstream will currently force 
migrate to system memory.

> 
> 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@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
>