Re: [igt-dev] [PATCH v17 4/7] lib/i915: add gem_engine_topology library and for_each loop definition

[Andi Shyti <andi@etezian.org>]

Hi Tvrtko,

> > +	struct i915_context_param_engines *engines =
> > +			(struct i915_context_param_engines*) param->value;
> 
> Cosmetics only: Our coding style is "(type *)var".
> 
> > +	for (typeof(engines->class_instance[0]) *p =
> > +			&engines->class_instance[0];
> > +						i < ed->nengines; i++, p++)
> 
> More cosmetics: Indentation alignment looks wonky.

working at the edge of the 80 characters provides always
cosmetic challenges, I'll fix the above.

> > +struct intel_execution_engine2
> > +*intel_get_current_engine(struct intel_engine_data *ed)
> > +{
> > +	if (!ed->n)
> > +		ed->current_engine = &ed->engines[0];
> > +	else if (ed->n >= ed->nengines)
> > +		ed->current_engine = NULL;
> > +
> > +	return ed->current_engine;
> > +}
> > +
> 
> Why can this just return ed->current_engine? I am confused by the need to
> check against ed->nengines both in the getter and in the next helper. Just
> need to init ed->current_engine to the first one in init_engine_list I
> think.

In the previous patch there was an unrefined mistake:
ed->current_engine was initialized in the
'intel_init_engine_list()' function.

But because there everything is allocated in stack, the
'&ed->engines[0]' is also in stack that get freed when the
function returns, with the result that I lose reference.

For this, I would need the pointer to be assigned somewhere that
works with the addresses provided by the caller and this looks
like the best place for it (indeed I removed the assignement from
intel_init_engine_list()).

> > +struct intel_execution_engine2
> > +*intel_get_current_physical_engine(struct intel_engine_data *ed)
> > +{
> > +	struct intel_execution_engine2 *e;
> > +
> > +	if (ed->n >= ed->nengines)
> > +		return NULL;
> 
> Can this be hit? intel_next_engine looks to be avoiding incrementing past
> nengines - 1.

yes, you're right, I didn't want to hit the for loop for this
case that can be checked here, it's redundant, but it looks more
readable. I'll remove it.

> > +
> > +	if (igt_only_list_subtests())
> > +		intel_next_engine(ed);
> 
> In subtest listing mode there cannot be virtual engines in the list so I
> think this branch is not needed.

Yes, indeed, if we have the list of engines at this point, it's
only physical. I'll remove it.

> > +			__e2->name       = e2->name;
> > +			__e2->instance   = e2->instance;
> > +			__e2->class      = e2->class;
> > +			__e2->is_virtual = false;
> 
> You could use init_engine here but granted it would have to do a redundant
> name search so maybe not.

yes, this is how it is was done at first, I put it outside for
the reason you mentioned :)

Thanks,
Andi

> Regards,
> 
> Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev