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

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

On 20/06/2019 14:14, Andi Shyti wrote:
> The execution buffer flag value has now the engine index as it is
> mapped in the context. Retrieve the mapped index by interrogating
> the driver starting from the class/instance tuple.
> 
> A "gem_context_get_eb_flags_ci" helper allows to avoid declaring
> a "struct i915_engine_class_instance" for the purpose.
> 
> Return -EINVAL if the engine is not mapped in the given context.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> ---
> V1 --> V2 changelog:
> --------------------
> - refactor the code to avoid initializing the context just for
>    the purpose of getting the execution buffer flag (thanks
>    Tvrtko)
> 
>   lib/i915/gem_engine_topology.c | 31 +++++++++++++++++++++++++++++++
>   lib/i915/gem_engine_topology.h |  6 ++++++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b951672b..fd5be3491b89 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -270,6 +270,37 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>   	return 0;
>   }
>   
> +int gem_context_get_eb_flags(int fd, uint32_t ctx_id,
> +			     struct i915_engine_class_instance *ci)
> +{
> +	DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx_id, GEM_MAX_ENGINES);
> +
> +	/* legacy kernels */
> +	if (gem_topology_get_param(fd, &param)) {
> +		const struct intel_execution_engine2 *e;
> +
> +		__for_each_static_engine(e)
> +			if (e->class == ci->engine_class &&
> +			    e->instance == ci->engine_instance)
> +				return e->flags;
> +
> +		return -EINVAL;
> +	}
> +
> +	/* context has no engine mapped */
> +	if (!param.size)
> +		return -EINVAL;
> +
> +	/* engine map lookup */
> +	for (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;
> +
> +	/* engine is not mapped in the given context */
> +	return -EINVAL;
> +}

Looks good to me.. ;)

> +
>   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..57b5473bbd5a 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -53,6 +53,12 @@ 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);
>   
> +int gem_context_get_eb_flags(int fd, uint32_t ctx_id,
> +			     struct i915_engine_class_instance *ci);
> +
> +#define gem_context_get_eb_flags_ci(f, c, ...) \
> +	gem_context_get_eb_flags(f, c, &((struct i915_engine_class_instance){__VA_ARGS__}))
> +

Hah this is some trick. I assume this allows:

eb.flags = gem_context_get_eb_flags(fd, ctx, ..._RENDER, 0);

?

What if too few or too many parameters are given? I'm in two minds but 
can't argue it is very to be able to do this in IGT.

>   #define __for_each_static_engine(e__) \
>   	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>   
> 

Can you extend the series with a patch which converts the problematic 
subtests in perf_pmu to use this helper? Or even merge into this patch, 
I don't mind. Would have some moral grounds to r-b it then. ;)

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev