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

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

Hi Tvrtko,

> > > > +{
> > > > +	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>"?
> 
> Probably not. Plain "virtual" sounds ok.

OK!

> > > > +#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.
> 
> I am not following how we lose the context?

OK, you are talking about for_each_physical... sorry, sometimes I
might get lost.

> I was just discussing of our desired end game in therms of number and
> signature for for_each_.. iterators.
> 
> For me for_each_physical_engine doesn't need the context since it is about
> physical engines - not engine from the engine map. Maybe that one should
> even have some asserts then to make sure someone hasn't re-configured the
> default context.
> 
> And we have __for_each_physical_engine which uses the static table, for
> subtest enumeration.
> 
> Then for_each_context_engine is the fully featured one, which has the
> context id in parameters.
> 
> Can we solve all use cases with those three or we need mode?

Of course yes, We will have in this case these three:

__for_each_physical_engine(e)

for_each_physical_engine(fd, e) /* doesn't need context */

for_each_engine_class_instance(fd, ctx, e)


PS
What I meant before is that that we could have a

for_each_engine_class_instance(fd, e)

where the context is created inside the for_each, but in case of:

  for_each_engine_class_instance(fd, e) { /* <--- creates context in intel_init_engine_list() */
    if (<any_condition>)
      break; /* <---- we don't have time to call gem_context_destroy() */
  }

But you were referring to physical engines, and in that case
there are no issues.

> > > > @@ -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 :)
> 
> Just things like types and coding style. :) And some more details from
> Chris' review.

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