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

Andi Shyti
2019-03-21T12:18:05Z

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