[igt-dev] [RFC PATCH v8 2/5] lib/i915: add gem_query library
Andi Shyti
andi.shyti at intel.com
Wed Feb 13 00:55:03 UTC 2019
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
More information about the igt-dev
mailing list