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

[Andi Shyti]

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;

> > +#define SIZEOF_QUERY           sizeof(struct drm_i915_query_engine_info) + \
> > +                                       I915_EXEC_RING_MASK * \
> > +                                       sizeof(struct drm_i915_engine_info)
> > +
> > +struct intel_execution_engine2 *intel_active_engines2;
> > +
> > +static int __gem_query(int fd, struct drm_i915_query *q)
> > +{
> > +       int err = 0;
> > +
> > +       if(igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
>             ^ kernel coding style applies (missed space)

yes :) I didn't notice, it should be some late night coding
leftover, we should add checkpatch.

> > +bool gem_can_query(void)
> 
> Still nothing to do with query per-se...
> 
> gem_has_engine_topology() ?

OK, sure.

> > +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> 
> We've drifted out of the domain of i915/gem_query.c into
> i915/gem_engine.c You could even argue we want to keep gem_engine.c for
> lower level interactions, and say gem_engine_topology.c for the details of
> the layout. Hmm, I like?

OK, I can add a new API

> > +{
> > +       int i;
> 
> Kernel coding style is to order variable lines by length, longest first;
> unless there is a clear reason why not to.

It's a community based preference, each maintainer has his own
opinion on variable ordering. I just place it in the order that
they come to my mind, until the maintainer dictates the rule.

> > +       __u8 ctx_param_buffer[SIZEOF_CTX_PARAM] = { };
> 
> s/__u8/uint8_t/
> 
> It just a boring stack buf, so call it buf.

OK.

> > +       ctx_param.ctx_id = ctx_id;
> > +       ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> > +       ctx_param.size = SIZEOF_CTX_PARAM;
> 
> Not quite; size is used to determine the number of engines, this makes a
> large array with upto 5 real engines followed by rcs0 repeated ~60
> times. Interesting ;)

yes, need to save the engine count somewhere. In a previous
series I was 'for(...);' but then removed it...

> > +       ctx_engine->extensions = 0;
> > +       for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {
> 
> You should at least have an assert you don't overflow your assumptions.

yes, this happened quite many times last night while I was fixing
the code and i promised myself to add the assert, but eventually
I forgot.

> > +               ctx_engine->class_instance[i].engine_class = e2->class;
> > +               ctx_engine->class_instance[i].engine_instance = e2->instance;
> > +       }
> 
> > +       ctx_param.value = to_user_pointer(ctx_engine);

> ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i]);
> 
> * quickly adds the packed attribute

... neat!

> > +       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!

> > +       for (i = 0; i < query_engine->num_engines; i++) {
> > +               char *name;
> > +               int class = query_engine->engines[i].class;
> > +               int instance = query_engine->engines[i].instance;
> > +
> > +               igt_assert(class < ARRAY_SIZE(class_names) && class >= 0);
> > +               igt_assert(class_names[class]);
> 
> Be future proof;
> 
> if ((unsigned)class >= ARRAY_SIZE() || !class_names[class])
> 	tmpl = "xxx"; // or "unk" I think I've seen Tvrtko use
> else
> 	tmpl = class_names[class];
> 
> Bonus points to include class-id in tmpl in case there's more than one!

OK.

> > +               intel_active_engines2[i].class = class;
> > +               intel_active_engines2[i].instance = instance;
> > +
> > +               igt_assert(asprintf(&name, "%s%d",
> > +                                   class_names[class], instance) > 0);
> > +               intel_active_engines2[i].name = name;
> > +       }
> > +
> > +       /* '0' value sentinel */
> Which bit?
> class_instance = (0, 0) is valid (rcs0)
> 
> /* Use .name=NULL as a sentinel */

Sure.

> > +#ifndef GEM_QUERY_H
> > +#define GEM_QUERY_H
> > +
> > +#include "igt_gt.h"
> 
> For intel_execution_engine2?
> 
> Just a forward decl will do.
> 
> > +void __set_ctx_engine_map(int fd, uint32_t ctx_id);
> 
> One day you'll find a more fitting name. Hint, this isn't part of the
> "set" infrastructure.
> 
> > +bool gem_can_query(void);
> > +void gem_query(int fd, struct drm_i915_query *q);
> > +
> > +void igt_require_gem_engine_list(int fd);
> > +
> > +extern struct intel_execution_engine2 *intel_active_engines2;
> 
> But I'm voting this extern should be in gem_engine_topology.[ch]

OK, I will add the gem_engine_topology.[ch]

Thanks,
Andi