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

[Chris Wilson]

Quoting Andi Shyti (2019-02-13 00:55:03)
> 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.

Wait until you read the igt_assert output.

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

Do you think that execbuf2 is our final form? Besides the argument about
heap vs stack is more appropriate here; using the stack is more
appropriate later.

> 
> [*] 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).

-ENODEV is generally for unsupported the platforms, which the former
tests. An unrecognised param is -EINVAL. ~o~
> 
> > 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.

engines = calloc(...).
igt_assert(engines);

I sometimes wish we distinguished between igt_assert() and just plain
old assert, so that we know that igt_assert() actually is significant
for testing.
-Chris