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

Andi Shyti
2019-03-29T12:05:51Z

Hi Tvrtko,

> > + nengines = param.size > sizeof(struct i915_context_param_engines) ?
> > + (param.size - sizeof(struct i915_context_param_engines)) /
> > + sizeof(engines.class_instance[0]) :
> > + 0;
> > +
> > + igt_assert_f(nengines < GEM_MAX_ENGINES, "unsupported engine count\n");
>
> Should this be <= ?

yes

> > +struct intel_engine_data {
> > + uint32_t nengines;
> > + uint32_t n;
> > + int error;
> > + struct intel_execution_engine2 *current_engine;
> > + struct intel_execution_engine2 *current_phys_engine;
>
> This field seem only ever assigned, never otherwise used. Do you need it for
> something later?

I took the comment from Chris that wanted to have two separate
lists, but indeed this doen't look fully right (or maybe I didn't
fully get what he meant exactly)

> > +#define for_each_context_engine(fd__, ctx__, e__) \
> > + for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> > + ((e__) = intel_get_current_engine(&i__)); \
> > + intel_next_engine(&i__))
> > +
>
> I would probably have the physical vs virtual logic in the "next" helper and
> just one intel_get_current_engine but it is not very relevant.

Sure! It's simpler.

> Looks mostly as expected. I trust you tested it works. :) With the assert
> fixed and unused struct member removed:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks a lot for your reviews!

Andi