Re: [igt-dev] [PATCH i-g-t 1/3] tests/gem_ctx_shared: Adopt to use allocator

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 18 Aug 2021 21:49:39 -0700, Zbigniew Kempczyński wrote:
>
> @@ -518,12 +533,24 @@ static void store_dword(int i915, const intel_ctx_t *ctx, unsigned ring,
>
>	memset(obj, 0, sizeof(obj));
>	obj[0].handle = cork;
> -	obj[0].offset = cork << 20;
>	obj[1].handle = target;
> -	obj[1].offset = target << 20;
>	obj[2].handle = gem_create(i915, 4096);
> -	obj[2].offset = 256 << 10;
> -	obj[2].offset += (random() % 128) << 12;
> +	if (ahnd) {
> +		obj[0].offset = cork_offset;
> +		obj[0].flags |= EXEC_OBJECT_PINNED;
> +		obj[1].offset = target_offset;
> +		obj[1].flags |= EXEC_OBJECT_PINNED;
> +		if (write_domain)
> +			obj[1].flags |= EXEC_OBJECT_WRITE;
> +		obj[2].offset = get_offset(ahnd, obj[2].handle, 4096, 0x0);
> +		obj[2].flags |= EXEC_OBJECT_PINNED;
> +		execbuf.flags |= I915_EXEC_NO_RELOC;

I915_EXEC_NO_RELOC needed (since I think we weren't doing this earlier)?

> -static void unplug_show_queue(int i915, struct igt_cork *c,
> +static void unplug_show_queue(int i915, struct igt_cork *c, uint64_t ahnd,
>			      const intel_ctx_cfg_t *cfg, unsigned int engine)
>  {
>	igt_spin_t *spin[MAX_ELSP_QLEN];
>
>	for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>		const intel_ctx_t *ctx = create_highest_priority(i915, cfg);
> -		spin[n] = __igt_spin_new(i915, .ctx = ctx, .engine = engine);
> +
> +		/*
> +		 * When we're using same vm we should use allocator handle
> +		 * passed by the caller. This is the case where cfg->vm
> +		 * is not NULL.
> +		 *
> +		 * For cases where context use its own vm we need separate
> +		 * ahnd for it.
> +		 */
> +		if (!cfg->vm)
> +			ahnd = get_reloc_ahnd(i915, ctx->id);

Ok, good, even though in this code we always used shared vm this is good in
case something changes later.

> @@ -599,8 +643,10 @@ static uint32_t store_timestamp(int i915,
>	uint32_t handle = gem_create(i915, 4096);
>	struct drm_i915_gem_exec_object2 obj = {
>		.handle = handle,
> -		.relocation_count = 1,
> -		.offset = (32 << 20) + (handle << 16),
> +		.relocation_count = !ahnd ? 1 : 0,
> +		.offset = !ahnd ? (32 << 20) + (handle << 16) :
> +				  get_offset(ahnd, handle, 4096, 0),
> +		.flags = !ahnd ? 0 : EXEC_OBJECT_PINNED,

| EXEC_OBJECT_WRITE ?

> @@ -612,7 +658,7 @@ static uint32_t store_timestamp(int i915,
>	struct drm_i915_gem_execbuffer2 execbuf = {
>		.buffers_ptr = to_user_pointer(&obj),
>		.buffer_count = 1,
> -		.flags = ring | I915_EXEC_FENCE_IN,
> +		.flags = ring | I915_EXEC_FENCE_IN | (!!ahnd * I915_EXEC_NO_RELOC),

I915_EXEC_NO_RELOC needed? Also !! has gone out of fashion :)

> @@ -831,6 +903,8 @@ static void smoketest(int i915, const intel_ctx_cfg_t *cfg,
>	unsigned engine;
>	uint32_t scratch;
>	uint32_t *ptr;
> +	uint64_t ahnd = get_reloc_ahnd(i915, 0); /* same vm */
> +	uint64_t scratch_offset;
>
>	q_cfg = *cfg;
>	q_cfg.vm = gem_vm_create(i915);
> @@ -848,9 +922,11 @@ static void smoketest(int i915, const intel_ctx_cfg_t *cfg,
>	igt_require(nengine);
>
>	scratch = gem_create(i915, 4096);
> +	scratch_offset = get_offset(ahnd, scratch, 4096, 0);
>	igt_fork(child, ncpus) {
>		unsigned long count = 0;
>		const intel_ctx_t *ctx;
> +		ahnd = get_reloc_ahnd(i915, 0); /* ahnd to same vm */

Can't we use the parent's ahnd here, since we are using
intel_allocator_multiprocess_start/stop?

Please take a look at the nits above and if anything needs to be
done. Otherwise this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>