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

Tvrtko Ursulin
2019-06-20T05:59:13Z

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__)++)