[igt-dev] [PATCH v14 4/5] lib/i915: add gem_engine_topology library and for_each loop definition

[Andi Shyti]

> > +static void init_engine(struct intel_execution_engine2 *e2, const char *name,
> > +			uint16_t class, uint16_t instance, uint8_t flags)
> 
> I'd probably use u64 for flags to match the structure.

yes, flags it's u64, I used u8 because the flags we use is never
supposed to be higher than than 3f. But sure, can make it u64.

> > +{
> > +	static const char *unk_name = "unk";
> > +
> > +	e2->class    = class;
> > +	e2->instance = instance;
> > +	e2->flags    = flags;
> > +
> > +	if (name) {
> > +		e2->name = name;
> 
> This path is used only for the legacy fall back mode so I am contemplating
> whether is is even worth having the name passed in.

yes, just wanted to be consistent. At the biginning the
dup_engine had a bigger role, but then I demoted it to just doing
this.

> The if you find a virtual engine in the list (
> I915_ENGINE_CLASS_INVALID/I915_ENGINE_CLASS_INVALID_VIRTUAL) you could set
> the name to "virtual" or something.

do we really need a name of the type "virtual-<engine>"?

> Now listen to this.. how about we export the engine names via the query API?
> Primarily I was thinking to distinguish difference instance of virtual, but
> then it would also lessen the reliance on the static map. Thoughts?

Do you mean that the name would be provided by the driver?

Other than improving the debug information, is the name
formatting giving any advantage if we can distinguish by
class/instance/flags?

We can't use it anyway for test creation.

[...]

> > +		uint8_t nengines = (param.size -
> > +				sizeof(struct i915_context_param_engines)) /
> > +				sizeof(engines->class_instance[0]);
> 
> I'd probably just use unsigned int.

Oh... I have set u32 in the intel_engine_data, I didn't reliase,
I assume that nengines would never be higher than 64 (if that
happens we can't handle it here).

But Chris is considering the case we will have more tha 64
engines, I can set it to u32, of course.

[...]

> > +#define for_each_engine_class_instance(fd__, ctx__, e__) \
> > +	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> > +		((e__) = (i__.n < i__.nengines) ? &i__.engines[i__.n] : NULL); \
> > +			i__.n++)
> 
> Do we want a context parameter in this helper, or even this helper at all? I
> thought we can end up with only two, for_each_physical_engine and
> for_each_context_engine - but I guess it is open for discussion.

I don't know of possible use cases that do or don't need ctx
outside the for_each...().

If you don't see any use of the context index outside the
for_each I can create the context inside the init_list function.

But, I have a little concern about the destraction of that
context. If the for_each... gets interrupted in the middle of the
loop, we lose the context.

[...]

> > @@ -434,7 +434,7 @@ busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
> >   	i = 0;
> >   	fd[0] = -1;
> > -	for_each_engine_class_instance(gem_fd, e_) {
> > +	for_each_engine_class_instance(gem_fd, 0, e_) {
> >   		if (e == e_)
> >   			busy_idx = i;
> > @@ -497,7 +497,7 @@ most_busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
> >   	unsigned int idle_idx, i;
> >   	i = 0;
> > -	for_each_engine_class_instance(gem_fd, e_) {
> > +	for_each_engine_class_instance(gem_fd, 0, e_) {
> >   		if (e == e_)
> >   			idle_idx = i;
> >   		else if (spin)
> > @@ -554,7 +554,7 @@ all_busy_check_all(int gem_fd, const unsigned int num_engines,
> >   	unsigned int i;
> >   	i = 0;
> > -	for_each_engine_class_instance(gem_fd, e) {
> > +	for_each_engine_class_instance(gem_fd, 0, e) {
> >   		if (spin)
> >   			__submit_spin_batch(gem_fd, spin, e, 64);
> >   		else
> > @@ -1683,7 +1683,7 @@ igt_main
> >   		igt_require_gem(fd);
> >   		igt_require(i915_type_id() > 0);
> > -		for_each_engine_class_instance(fd, e)
> > +		for_each_engine_class_instance(fd, 0, e)
> >   			num_engines++;
> >   	}
> > 
> 
> Looks like this would work. Just the question of virtual engine, set of
> chosen iterators, and maybe some nits.

Yes, as we discussed, right after this patchset I will do the
for_each_physical.

What are the nits? I love nits :)

Andi