Re: [igt-dev] [PATCH v19 5/6] lib: igt_dummyload: use for_each_context_engine()

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Andi Shyti <andi.shyti@intel.com>,
	IGT dev <igt-dev@lists.freedesktop.org>
Cc: Andi Shyti <andi@etezian.org>
Subject: Re: [igt-dev] [PATCH v19 5/6] lib: igt_dummyload: use for_each_context_engine()
Date: Mon, 8 Apr 2019 17:28:23 +0100	[thread overview]
Message-ID: <5bec9ae6-aaf4-4c65-b300-c51985b889b0@linux.intel.com> (raw)
In-Reply-To: <20190408161533.2421-6-andi.shyti@intel.com>

On 08/04/2019 17:15, Andi Shyti wrote:
> With the new getparam/setparam api, engines are mapped to
> context. Use for_each_context_engine() to loop through existing
> engines.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>   lib/igt_dummyload.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> index 47f6b92b424b..1d922fb698e0 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -39,6 +39,7 @@
>   #include "ioctl_wrappers.h"
>   #include "sw_sync.h"
>   #include "igt_vgem.h"
> +#include "i915/gem_engine_topology.h"
>   #include "i915/gem_mman.h"
>   
>   /**
> @@ -86,7 +87,7 @@ emit_recursive_batch(igt_spin_t *spin,
>   	struct drm_i915_gem_relocation_entry relocs[2], *r;
>   	struct drm_i915_gem_execbuffer2 *execbuf;
>   	struct drm_i915_gem_exec_object2 *obj;
> -	unsigned int engines[16];
> +	unsigned int flags[GEM_MAX_ENGINES];
>   	unsigned int nengine;
>   	int fence_fd = -1;
>   	uint32_t *batch, *batch_start;
> @@ -94,17 +95,30 @@ emit_recursive_batch(igt_spin_t *spin,
>   
>   	nengine = 0;
>   	if (opts->engine == ALL_ENGINES) {
> -		unsigned int engine;
> +		struct intel_execution_engine2 *engine;
>   
> -		for_each_physical_engine(fd, engine) {
> +		for_each_context_engine(fd, opts->ctx, engine) {
>   			if (opts->flags & IGT_SPIN_POLL_RUN &&
> -			    !gem_can_store_dword(fd, engine))
> +			    !gem_class_can_store_dword(fd, engine->class))
>   				continue;
>   
> -			engines[nengine++] = engine;
> +			flags[nengine++] = engine->flags;
>   		}
>   	} else {
> -		engines[nengine++] = opts->engine;
> +		struct intel_execution_engine2 *e;
> +		int class;
> +
> +		if (!gem_ctx_get_engine(fd, opts->engine, opts->ctx, e)) {
> +			class = e->class;
> +		} else {
> +			gem_require_ring(fd, opts->engine);
> +			class = gem_eb_to_class(opts->engine);
> +		}
> +
> +		if (opts->flags & IGT_SPIN_POLL_RUN)
> +			igt_require(gem_class_can_store_dword(fd, class));
> +
> +		flags[nengine++] = opts->engine;
>   	}

Patch looks good but I'm afraid I made an oversight earlier in review 
when I asked you to move this else block in this function. I forgot that 
the differentiation between igt_spin_batch_new and __igt_spin_batch_new 
was that the latter should not do any execbuf calls apart from the 
spinner itself. Since gem_require_ring does one it could be a problem.

Would you mind moving just this else block back into the main constructor?

With that you can have:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>   	igt_require(nengine);
>   
> @@ -234,7 +248,7 @@ emit_recursive_batch(igt_spin_t *spin,
>   
>   	for (i = 0; i < nengine; i++) {
>   		execbuf->flags &= ~ENGINE_MASK;
> -		execbuf->flags |= engines[i];
> +		execbuf->flags |= flags[i];
>   
>   		gem_execbuf_wr(fd, execbuf);
>   
> @@ -308,12 +322,6 @@ igt_spin_batch_factory(int fd, const struct igt_spin_factory *opts)
>   
>   	igt_require_gem(fd);
>   
> -	if (opts->engine != ALL_ENGINES) {
> -		gem_require_ring(fd, opts->engine);
> -		if (opts->flags & IGT_SPIN_POLL_RUN)
> -			igt_require(gem_can_store_dword(fd, opts->engine));
> -	}
> -
>   	spin = spin_batch_create(fd, opts);
>   
>   	igt_assert(gem_bo_busy(fd, spin->handle));
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev