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

[Andi Shyti]

On Wed, Mar 20, 2019 at 10:59:05AM +0000, Chris Wilson wrote:
> Quoting Andi Shyti (2019-03-20 10:49:13)
> > > > +       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?
> 
> If we cannot fit the ctx->engines[] into the cparam.size we also report
> -EINVAL. I'm wondering if we should establish a different errno
> convention for that.
> 
> > > > +                       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?
> 
> I want to handle virtual engines somehow :)
> 
> > 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.
> 
> Here I was just saying '+1' is obsolete.
> 
> > > > +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?
> 
> Did I not see asprintf? Anyway Tvrtko suggested that they can all be
> static names, so no, there shouldn't be much to clean up, but that is
> one huge struct to be passing around the stack!!!

in any case, I thought about the clean up, but it wouldn't fix
anything anyway, because if the for_each is interrupted, we would
never ever clean up and leak anything inside.

I somehow ignored asprintf because I always thought that pointing
to static names is not future proof enough (I considered that one
day we will get rid of intel_execution_engines2 array, while
this way we are binding to it even more. I don't know what is
worse :) ).

A solution is to expand the struct intel_execution_engine2, by
removing the "const" in front of the name so that we can have
more flexibility at assigning names.

Another solution would be having the engine_data structure as a
global structure that contains all informations and it's
allocated outside the for_each. We can have functions inside it
like init, get_next, and so on, instead of using it just as a
mere leaking iterator.

> -Chris