Re: [igt-dev] [PATCH v13 6/9] lib/i915: add gem_engine_topology library

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

> > +       uint8_t buff[SIZEOF_CTX_PARAM] = { };
> > +       struct i915_context_param_engines *cengine =
> > +                               (struct i915_context_param_engines *) buff;
> 
> Oi, noet. And just a single tab indent.

Yes, I messed up a few things in this version and as I was writing
to Tvrtko, also the kernel I was running had some stuff that were
screwing up the ioctls values.

> > +       struct drm_i915_gem_context_param cparam = {
> > +               .param = I915_CONTEXT_PARAM_ENGINES,
> > +               .ctx_id = ctx_id,
> > +               .size = SIZEOF_CTX_PARAM,
> > +               .value = to_user_pointer(cengine),
> > +       };
> > +       int ret, i;
> > +
> > +       cparam.value = to_user_pointer(cengine);
> > +
> > +       ret = __gem_context_get_param(fd, &cparam);
> > +
> > +       if (ret) {
> > +               /* if kernel does not support engine/context mapping */
> > +               const struct intel_execution_engine2 *e2;
> 
> Hmm, how does this distinguish against too many engines (more than can
> fit into buf?). Both return -EINVAL iirc?

I haven't found in the driver where we return -EINVAL for having
too many engines. Have I missed it somewhere?

> No, that is cparam.size == 0.
...
> Pardon?

please, don't mind this bits, I was experimenting with the driver
and forgot some stuff inside :)

> > +                       dup_engine(&engine_data.engines[i], NULL,
> > +                                  cengine->class_instance[i].engine_class,
> > +                                  cengine->class_instance[i].engine_instance,
> > +                                  i + 1);
> 
> This seems very suspect. If class/instance doesn't map to a known
> engine, dup_engine() should be figuring it out, as the engine[] is
> entirely at the arbitrary whim of the user.

it does, right? we know the list of engines and we assign
"unk<class>:<instance>" if the engine is not recognised.

Am I missing something?

In any case, I'm still going to change it and compare all class
instances against the intel_execution_engines2 array.

Or do you mean that we shouldn't have the engine at all in the
list I am creating... at the end that's what comes from the
driver.

> > +struct intel_engine_data {
> > +       int fd;
> > +       uint32_t ctx;
> > +
> > +       uint32_t nengines;
> > +       uint32_t n;
> > +       struct intel_execution_engine2 engines[I915_EXEC_RING_MASK + 1];
> > +};
> 
> This is the _iter. Pull the for_each_foo() into this patch so we can see
> how it is put together.
> 
> At which point, do we need the (fd,ctx) here since they are parameters to
> the for_each() and so available later?

they are useful for my functions... well... little advantage, no
need indeed.

I didn't see this as an iter structure rather than a data
structure (just an 'n' that increments for helping the for_each),
that we could use in other occasions other than looping thorugh.

> Missing _iter_fini. Polish the for_each_foo() a bit more.

_iter_fini? You mean an iter_end to clean up things? Do we need
it? Is there anything to clean up?
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev