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

Andi Shyti via igt-dev
2019-02-14T13:03:01Z

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