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 Chris,

> > +static void query_engines(int fd,
> > +                         struct drm_i915_query_engine_info *query_engines)
> 
> Pass in the length of the query_engines block i.e. don't just assume
> item.length. And I'm still worrying about what happens when it is
> greater than 64 engines.

OK, looks the same to me, but sure, I can set item.length
outside query_engines().

> > +       if (__gem_context_get_param(fd, &param)) {
> > +               /* if kernel does not support engine/context mapping */
> 
> We also take this path if we have more than 64 engines.

But I haven't found anywhere in the get_param() and
i915_query_ioctl() in the driver where this case is considered.

Don't we receive -EINVAL only when "args->size < size"?

The set_engines would fail if we try to do that. I don't see this
a plausible thing to happen (unless, of course, I missed it,
which is very likely).

In any case, we don't have an error control in engine_data and we
would never understand whether there is somthing screwed up.

I was thinking that I can add in the structure an "error"
variable so that we quit the loop if something wrong happens,
e.g. we receive size > 3f.

> > +               __for_each_engine_class_instance(e2) {
> > +                       uint64_t flags;
> > +
> > +                       if (!gem_has_engine(fd, e2->class, e2->instance))
> > +                               continue;
> > +
> > +                       flags = gem_class_instance_to_eb_flags(fd, e2->class,
> > +                                                              e2->instance);
> 
> You added the field to the static struct, you might as well populate it
> as well, and just use e2->flags. And then you wouldn't even need to
> repeat the computation for gem_has_engine.

makes sense.

> > +struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id);
> > +
> > +#endif /* GEM_ENGINE_TOPOLOGY_H */
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 475c0b3c3cc6..84ea4af5392d 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> 
> And we need to break away from igt_gt.h, these will all be good inside
> gem_engine_topology.h I think.

I was thinking the same, but given that all the for_each are in
igt_gt.h I guess that's the right place for consistence, unless
we move all the for_each, until we will get rid of all the
legacy.

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