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

Chris Wilson
2019-02-14T13:08:28Z

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