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

[Chris Wilson <chris@chris-wilson.co.uk>]

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!!!
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev