[igt-dev] [PATCH i-g-t 34/79] tests/i915/gem_watchdog: Convert to intel_ctx_t (v2)

[Dixit, Ashutosh]

On Thu, 17 Jun 2021 12:12:41 -0700, Jason Ekstrand wrote:
>
> v2 (Jason Ekstrand):
>  - Rebase on Tvrtko's changes
>  - Fix an issue in default-virtual with the set-once rule for engines
>
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>

A few comments below. Please fix as needed, after fixing this is:

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

> -static void physical(int i915)
> +static void physical(int i915, const intel_ctx_t *ctx)
>  {
>	const unsigned int wait_us = default_timeout_wait_s * USEC_PER_SEC;
> -	unsigned int num_engines = __engines__->num_engines, i, count;
> +	unsigned int num_engines, i, count;
>	const struct intel_execution_engine2 *e;
> -	unsigned int expect = num_engines;
> -	igt_spin_t *spin[num_engines];
> +	igt_spin_t *spin[GEM_MAX_ENGINES];
>
>	i = 0;
> -	__for_each_physical_engine(i915, e) {
> -		spin[i] = igt_spin_new(i915,
> +	for_each_ctx_engine(i915, ctx, e) {
> +		spin[i] = igt_spin_new(i915, .ctx = ctx,
>				       .engine = e->flags,
>				       .flags = spin_flags());
>		i++;
>	}
> +	num_engines = i;

Don't even need this, num_engines = ctx->cfg->num_engines.

> -static void virtual(int i915)
> +static void virtual(int i915, const intel_ctx_cfg_t *base_cfg)
>  {
>	const unsigned int wait_us = default_timeout_wait_s * USEC_PER_SEC;
> -	unsigned int num_engines = __engines__->num_engines, i, count;
> +	unsigned int num_engines = base_cfg->num_engines, i, count;
>	igt_spin_t *spin[num_engines];
>	unsigned int expect = num_engines;
> -	uint32_t ctx[num_engines];
> -	uint32_t vm;
> +	intel_ctx_cfg_t cfg = {};
> +	const intel_ctx_t *ctx[num_engines];
>
>	igt_require(gem_has_execlists(i915));
>
>	igt_debug("%u virtual engines\n", num_engines);
>	igt_require(num_engines);
>
> +	cfg.vm = gem_vm_create(i915);
> +
>	i = 0;
>	for (int class = 0; class < 32; class++) {
>		struct i915_engine_class_instance *ci;
>
> -		ci = list_engines(class, &count);
> +		ci = list_engines(base_cfg, class, &count);
>		if (!ci)
>			continue;
>
> @@ -296,17 +238,12 @@ static void virtual(int i915)
>
>			igt_assert(i < num_engines);
>
> -			ctx[i] = gem_context_create(i915);
> +			ctx[i] = intel_ctx_create(i915, &cfg);

I would have expected this to be base_cfg, since cfg only has legacy
engines. But even the original code has only legacy engines for the load
balancer so this "somehow works" in a manner unknown to me :-/

>
> -			if (!i)
> -				vm = ctx_get_vm(i915, ctx[i]);
> -			else
> -				ctx_set_vm(i915, ctx[i], vm);
> -
> -			set_load_balancer(i915, ctx[i], ci, count, NULL);
> +			set_load_balancer(i915, ctx[i]->id, ci, count, NULL);
>
>			spin[i] = igt_spin_new(i915,
> -					       .ctx_id = ctx[i],
> +					       .ctx = ctx[i],
>					       .flags = spin_flags());
>			i++;
>		}
> @@ -317,8 +254,8 @@ static void virtual(int i915)
>	count = wait_timeout(i915, spin, num_engines, wait_us, expect);
>
>	for (i = 0; i < num_engines && spin[i]; i++) {
> -		gem_context_destroy(i915, ctx[i]);
>		igt_spin_free(i915, spin[i]);
> +		intel_ctx_destroy(i915, ctx[i]);
>	}
>
>	igt_assert_eq(count, expect);
> @@ -499,17 +436,6 @@ delay_create(int i915, uint32_t ctx,
>	return obj;
>  }
>
> -static uint32_t vm_clone(int i915)
> -{
> -	uint32_t ctx = 0;
> -	__gem_context_clone(i915, 0,
> -			    I915_CONTEXT_CLONE_VM |
> -			    I915_CONTEXT_CLONE_ENGINES,
> -			    I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE,
> -			    &ctx);
> -	return ctx;
> -}
> -
>  static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
>  {
>	int err;
> @@ -526,6 +452,7 @@ static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
>
>  static uint32_t
>  far_delay(int i915, unsigned long delay, unsigned int target,
> +	  const intel_ctx_t *ctx,
>	  const struct intel_execution_engine2 *e, int *fence)
>  {
>	struct drm_i915_gem_exec_object2 obj = delay_create(i915, 0, e, delay);
> @@ -540,6 +467,7 @@ far_delay(int i915, unsigned long delay, unsigned int target,
>		.buffer_count = 2,
>		.flags = e->flags,
>	};
> +	intel_ctx_cfg_t cfg = ctx->cfg;
>	uint32_t handle = gem_create(i915, 4096);
>	unsigned long count, submit;
>
> @@ -552,23 +480,27 @@ far_delay(int i915, unsigned long delay, unsigned int target,
>	submit *= NSEC_PER_SEC;
>	submit /= 2 * delay;
>
> +	if (gem_has_vm(i915))
> +		cfg.vm = gem_vm_create(i915);

Might as well add I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE too if needed?

> +
>	/*
>	 * Submit a few long chains of individually short pieces of work
>	 * against a shared object.
>	 */
>	for (count = 0; count < submit;) {
> -		execbuf.rsvd1 = vm_clone(i915);
> +		const intel_ctx_t *tmp_ctx = intel_ctx_create(i915, &cfg);
> +		execbuf.rsvd1 = tmp_ctx->id;
>		if (!execbuf.rsvd1)

This check is always false now (intel_ctx_create will assert on error).

> @@ -652,32 +585,22 @@ igt_main
>		}
>
>		i915 = gem_reopen_driver(i915); /* Apply modparam. */
> -
> -		__engines__ = malloc(4096);
> -		igt_assert(__engines__);
> -		memset(__engines__, 0, 4096);
> -		memset(&item, 0, sizeof(item));
> -		item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> -		item.data_ptr = to_user_pointer(__engines__);
> -		item.length = 4096;
> -		i915_query_items(i915, &item, 1);
> -		igt_assert(item.length >= 0);
> -		igt_assert(item.length <= 4096);
> -		igt_assert(__engines__->num_engines > 0);
> +		ctx = intel_ctx_create_all_physical(i915);
>	}
>
>	igt_subtest_group {
>		igt_subtest("default-physical")
> -			physical(i915);
> +			physical(i915, ctx);
>
>		igt_subtest("default-virtual")
> -			virtual(i915);
> +			virtual(i915, &ctx->cfg);
>	}
>
>	igt_subtest_with_dynamic("far-fence") {
> -		__for_each_physical_engine(i915, e) {
> +		for_each_ctx_engine(i915, ctx, e) {
>			igt_dynamic_f("%s", e->name)
> -				far_fence(i915, default_timeout_wait_s * 3, e);
> +				far_fence(i915, default_timeout_wait_s * 3,
> +					  ctx, e);
>		}
>	}

Missing intel_ctx_destroy.