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.
> 
> Wait until you read the igt_assert output.

OK, didn't think about it, I'm definitely missing some history
here.

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

I can do that in case of 'drm_i915_query_engine_info' because the
flexible structure has a name (struct drm_i915_engine_info engines[];).

But here the choices are less pleasant. Because I either do in
the kernel something like:

+	struct whatever_name {
-	struct {
 		__u16 engine_class; /* see enum drm_i915_gem_engine_class */
 		__u16 engine_instance;
 	} class_instance[0];

Or I define some ugly unflexible array in __set_ctx_engine_map():

	struct {
		__u16 ngine_class;
		__u16 engine_instance;
	} class_instance[I915_EXEC_RING_MASK];

which looks to me quite strict (if we are considering that we
might subvert everyting).

Or, even worse, some "__u32 engine_stuff[I915_EXEC_RING_MASK]"
and cast it.

Allocating it everywhere it looked cleaner.

Whatever you like :)

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

Yes, makes sense, while I was writing this I also thought that
-EINVAL is not correct, but I kept it for consistency with the
ioctl. I will return in both cases -ENODEV, then.

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

It's a cultural thing :)

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