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

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

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