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

[Ramalingam C]

On 2019-06-20 at 17:14:38 +0100, Tvrtko Ursulin wrote:
> 
> 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 at intel.com>
> > Signed-off-by: Andi Shyti <andi.shyti at intel.com>
> > Cc: Ramalingam C <ramalingam.c at 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)
tvrtko and Andi,

instead of creating the i915_engine_class_instance on the go, can't we have the
intel_execution_engine2 * itself passed to this function? Anyway engine2
pointer will be available in all the time this function is called.

I am using this patch for
s/for_each_physical_engine/__for_each_physical_engine. Shall do the above change and submit?

-Ram
> > +{
> > +	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