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

[Andi Shyti via igt-dev <igt-dev@lists.freedesktop.org>]

Hi Chris,

> > +static int __gem_query(int fd, struct drm_i915_query *q)
> > +{
> > +       return igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q) ? -errno : 0;
> > +}
> > +
> > +void gem_query(int fd, struct drm_i915_query *q)
> > +{
> > +       igt_assert(!__gem_query(fd, q));
> 
> For extra tidy asserts:
> 
> static int __gem_query(int fd, struct drm_i915_query *q)
> {
> 	int err;
> 
> 	err = 0;
> 	if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
> 		err = -errno;
> 
> 	errno = 0;
> 	return er;
> }

Yes, I've seen this around, although it looks a bit redundant to
me, I'll keep the style.

> > +static int __gem_get_set_param(int fd, unsigned long request,
> > +                              struct drm_i915_gem_context_param *p)
> > +{
> > +       return igt_ioctl(fd, request, p) ?  -errno : 0;
> > +}
> > +
> > +void gem_get_set_param(int fd, unsigned long request,
> > +                      struct drm_i915_gem_context_param *p)
> 
> gem_context_set_param! It exists!

Oh! I didn't know! That's a great discovery :)

> > +{
> > +       igt_assert(!__gem_get_set_param(fd, request, p));
> > +}
> > +
> > +bool gem_has_get_set_param(void)
> 
> Has what?

has get/setparam. I couldn't come out with a better name. I'll
think harder.

> > +       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> > +       query.items_ptr = to_user_pointer(&item);
> > +       query.num_items = 1;
> > +       item.length = sizeof(*query_engines) +
> > +                     64 * sizeof(struct drm_i915_engine_info);
> 
> You are betting we are not going to exceed 64 engines? A common bet for
> sure...

We've been discussing about this in v4 and we agreed that 64 is
big enough[*]. Am I missing anything?
Besides, I thought that we won't have more engines than
I915_EXEC_RING_MASK.

[*] https://lists.freedesktop.org/archives/igt-dev/2019-January/008034.html

> > +static int gem_init_engine_list(int fd)
> > +{
> > +       int i, ret;
> > +       struct drm_i915_query_engine_info *query_engine = query_engines(fd);
> > +       const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
> 
> class, not engine, names. And deserves its own mapping table with api.

I can make a new API in a next patch and remove it from here, as
it is a bit out of the scope if the series.

> > +       struct drm_i915_gem_context_param ctx_param = {
> > +               .param = I915_CONTEXT_PARAM_ENGINES,
> > +       };
> > +
> > +       /* the list is already initialized */
> > +       if (intel_active_engines2)
> > +               return gem_has_get_set_param() ? 0 : -EINVAL;
> 
> We would use -ENODEV? Leaks query_engine, probably should reorder.

I wanted here to be consistent with the failure value...  (continues)

> > +       /*
> > +        * we check first whether the new engine discovery uapi
> > +        * is in the current kernel, if not, the
> > +        * DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM will fail with
> > +        * errno = EINVAL. In this case, we fall back to using
> > +        * the previous engine discovery way
> > +        */
> > +       ret = __gem_get_set_param(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM,
> > +                                 &ctx_param);
> > +       if (ret) {
> > +               if (ret == -EINVAL)
> > +                       intel_active_engines2 = intel_execution_engines2;

... here we return -EINVAL to indicate that the get/setparam we
need is not implemented. If I return -ENODEV before, I should
return -ENODEV here as well (but that's not what the ioctl
returns).

> Leaks

Right!

> > +       igt_assert((intel_active_engines2 =
> > +                   calloc(query_engine->num_engines + 1,
> > +                          sizeof(*intel_active_engines2))));
> 
> Don't be afraid of using two lines for different effects.

You mean 2 instead of 3? I just wanted to keep it under 80.

> > +       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(engine_names) && class >= 0);
> > +               igt_assert(engine_names[class]);
> > +
> > +               intel_active_engines2[i].class = class;
> > +               intel_active_engines2[i].instance = instance;
> > +
> > +               igt_assert(asprintf(&name, "%s%d",
> > +                                   engine_names[class], instance) > 0);
> > +               intel_active_engines2[i].name = name;
> > +       }
> 
> +1 because you want to terminate with a sentinel?
> 
> Use malloc and make it explicit.

OK.

> > +void igt_require_gem_engine_list(int fd)
> > +{
> > +       igt_require_intel(fd);
> 
> Redundant.

OK.

Thanks again, Chris!

Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev