[igt-dev] [RFC PATCH v9 2/5] lib/i915: add gem_query library

[Chris Wilson]

Quoting Andi Shyti (2019-02-14 13:02:58)
> Hi Chris,
> 
> > > +/*
> > > + * HACK: the '2 * sizeof(__u16)' is the size required for the flexible array
> > > + * inside 'struct i915_context_param_engines' that doesn't have a name and
> > > + * therefore cannot be referenced. It needs a name in the uapi.
> > > + */
> > 
> > It's not going to get one until you raise the point in review of the
> > uAPI. It doesn't though, you can use sizeof on the unnamed member.
> > 
> > > +#define SIZEOF_CTX_PARAM       sizeof(struct i915_context_param_engines) + \
> > > +                                       I915_EXEC_RING_MASK * \
> > > +                                       2 * sizeof(__u16)
> > 
> > For example:
> > 
> > SIZEOF_CTX_PARAM offsetofend(struct i915_context_param_engines,
> >                            class_instance[I915_EXEC_RING_MASK + 1])
> > 
> > To make it generic though, it needs __attribute__((packed))
> 
> yeah! makes sense.
> 
> > (If the maximum index is I915_EXEC_RING_MASK, we need
> > I915_EXEC_RING_MASK + 1 storage.)
> 
> Yes, I didn't add the '+ 1' because in the driver you do:
> 
>         if (set.nengine == 0 || set.nengine > I915_EXEC_RING_MASK)
>                 return -EINVAL;

Indeed that needs to be +1 since we aren't reserving slot 0 anymore.
> 
> > > +       ret = __gem_context_get_param(fd, &ctx_param);
> > > +       if (ret) {
> > > +               if (ret == -EINVAL) {
> > 
> > Just
> > 
> > if (__gem_context_get_param()) {
> >       intel_active_engines2 = intel_execution_engines2;
> >       return -ENODEV;
> > }
> > 
> > This be library code, so you define the interface; as opposed to test
> > code where we demand certain responses from the kernel.
> 
> we don't need other -errnos? OK!

I haven't checked if we need that error either ;)

I think I have a slightly different idea for the for_each_engine loop
anyway.
-Chris