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 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.

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.

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.

Regards,

Tvrtko

> 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
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev