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

[Andi Shyti <andi.shyti@intel.com>]

Hi Tvrtko,

> > +static void init_engine(struct intel_execution_engine2 *e2, const char *name,
> > +			uint16_t class, uint16_t instance, uint64_t flags)
> 
> You are keeping name for the future? (It is unused at the moment.)

I simply forgot to remove it :)

> > +{
> > +	static const char *unknown_name = "unknown",
> > +			  *virtual_name = "virtual";
> 
> Unusual style but it is actually readable so I think I like it.

like the function below, it doesn't happen everyday to declare
variables/functions where the longest part of the line is the
type.

Need to come out with something :)

> > +
> > +	e2->class    = class;
> > +	e2->instance = instance;
> > +	e2->flags    = flags;
> > +
> > +	if (class < 0 && instance < 0) {
> > +		e2->name = virtual_name;
> > +	} else {
> > +		const struct intel_execution_engine2 *__e2;
> > +
> > +		__for_each_static_engine(__e2)
> > +			if (__e2->class == class && __e2->instance == instance)
> > +				break;
> > +
> > +		e2->name = __e2->name ? __e2->name : unknown_name;
> 
> I've now started to worry about how will CI/buglog handle us forgetting to
> expand the static list. (More than one subtest of a same name for
> "test-$engine_name" ones?) Do we want and igt_warn on unknown engines to
> make it more visible? Or even just crash?

Right! I guess just a warning would be nice, we can gather more
information from the logs about unkown engines... I guess.

> > +	if (nengines > I915_EXEC_RING_MASK + 1) {
> > +		engine_data.error = ret ? ret : -EINVAL;
> > +		return engine_data;
> > +	}
> 
> If we one day allow more engines in the map than the current limit?

for now this is a driver limitation and that's what igt checks. I
guess the right approach would be to update igt according to the
driver, right?

> It looks this would make the iterator not work. Was that the intention? What
> is the point of continuing then rather than just asserting?

yes, the iterator wouldn't loop and would provide a 0 size list
of engines. Asserting makes more sense.

> > +struct intel_execution_engine2
> > +	*intel_get_current_engine(struct intel_engine_data *ed)
> 
> Unusual coding style, we use:
> 
> type
> func(params)
> 
> or:
> 
> type func(params)

yes, I've seen it around, I personally don't like it, but I will
do it as the style is.

> > +	return (ed->n < ed->nengines) && !ed->error ?
> > +		&ed->engines[ed->n] :
> > +		NULL;
> 
> So could store the pointer to current engine in the iterator?

I still don't see the use of it, but of course, I can add it. It
might make more sense now that I have a "_current" and a "_next"
function.

> > @@ -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_context_engine(gem_fd, 0, e_) {
> 
> Make perf_pmu use for_each_physical_engine, apart from the test enumeration.

the reason I didn't replace it right away is that still to many
functions are using the current implementation of
for_each_physical_engine and I wanted to avoid, in this patchset,
touching too many files. I'm already changing more than I
wished.

that's why I called it:

   #define __for_each_physical_engine__(fd__, e__)

at the beginning I wanted to call it

   #define __DO_NOT_USE_for_each_physical_engine_YET__(fd__, e__) \

Besides, none of all those functions is using the new e2
structure.

If it's OK with you, I would swap everything to use either
__for_each_static_engine or the new for_each_physical_engine
right after this patch goes in.

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