Re: [igt-dev] [PATCH] lib/i915: gem_engine_topology: get eb flags from engine's coordiantes

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/06/2019 23:19, Andi Shyti wrote:
> The execution buffer flag value has now the engine index as it is
> mapped in the context. Retrieve its mapped index from the
> class/instance coordinate.

First sentence reads a bit unclear. It may s/have/need/ the engine index _if_ context has engine map. That would be more accurate I think.

Also s/coordiantes/coordinates/ in the subject, although we never used coordinates in this context so maybe just s/coordinates/class:instance/?
 
> Return -EINVAL if the engine is not mapped in the given context.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> ---
>   lib/i915/gem_engine_topology.c | 12 ++++++++++++
>   lib/i915/gem_engine_topology.h |  3 +++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b951672b..23208102021f 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -270,6 +270,18 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>   	return 0;
>   }
>   
> +int64_t gem_context_get_eb_flags(int fd, uint32_t ctx_id,
> +				 int class, int instance)
> +{

I'd just return int since I don't see 64-bit is giving any benefit. Plus I'd pass in struct i915_engine_class_instance as input.

> +	struct intel_execution_engine2 *e;
> +
> +	for_each_context_engine(fd, ctx_id, e)
> +		if (class == e->class && instance == e->instance)
> +			return e->flags;

And most important difference, I wouldn't configure the context from this helper. Instead I think all we need is:

int gem_context_get_eb_flags(int fd, uint32_t ctx, struct i915_engine_class_instance ci)
{
	DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
	int ret

	ret = gem_topology_get_param(fd, &param);
	if (ret) {
		const struct intel_execution_engine2 *e;

		/* Legacy kernels. */
		__for_each_static_engine(e) {
			if (e->class == ci.engine_class &&
			    e->instance == ci.engine_instance)
				return e->flags;
		}

		return -EINVAL;
	}

	/* Engine map with no engines. */
	if (!param.size)
		return -EINVAL;

	/* Engine map lookup. */
	for (unsigned int i = 0; i < param.size; i++) {
		if (engines.engines[i].engine_class == ci.engine_class &&
		    engines.engines[i].engine_instance == ci.engine_instance)
			return i;
	}

	return -EINVAL;
}

Regards,

Tvrtko

> +
> +	return -EINVAL;
> +}
> +
>   void gem_context_set_all_engines(int fd, uint32_t ctx)
>   {
>   	DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index 2415fd1e379b..f575b8968476 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -53,6 +53,9 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>   
>   void gem_context_set_all_engines(int fd, uint32_t ctx);
>   
> +int64_t gem_context_get_eb_flags(int fd, uint32_t ctx_id,
> +				  int class, int instance);
> +
>   #define __for_each_static_engine(e__) \
>   	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev