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

[Andi Shyti <andi@etezian.org>]

On Mon, Jun 24, 2019 at 08:26:20AM +0100, Tvrtko Ursulin wrote:
> 
> On 24/06/2019 06:45, Ramalingam C wrote:
> > 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@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)
> > 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 think intel_execution_engine2 is not always available. For instance
> perf_pmu/frequency|interrupts it is not.

Yes, agree... besides is not nice to ask the caller to always
declare an intel_execution_engine2 structure

> Maybe change struct intel_execution_engine2 to embed struct
> i915_engine_class_instance instead of having separate class and instance
> fields? So in tests where you have it you can just pass e2->engine? Maybe it
> would require too much churn, not sure.

Yes, this would be nice... but it would require a huge changeset.
I'll try to do it.

> For the option with less churn we can have two or more flavours of the
> helper. One can take class and instance separately, one can take ci and one
> struct intel_execution_engine2 if that makes sense.

Yes, I can make some defines of this type in the next version:

int gem_context_get_eb_flags(int fd, uint32_t ctx_id, int class, int instance)

#define gem_context_get_eb_flags_ci(fd, ctx_id, ci) \
	gem_context_get_eb_flags(fd, ctx_id, ci->engine_class, ci->engine_instance)

#define gem_context_get_eb_flags_e(fd, ctx_id, e) \
	gem_context_get_eb_flags(fd, ctx_id, e->ci->engine_class, e->ci->engine_instance)

Does it add entropy?

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