[igt-dev] [RFC PATCH v8 2/5] lib/i915: add gem_query library
Chris Wilson
chris at chris-wilson.co.uk
Wed Feb 13 00:13:15 UTC 2019
Quoting Andi Shyti (2019-02-12 23:54:33)
> The gem_query library is a set of functions that interface with
> the query and getparam/setparam ioctls.
>
> The entry point of the library is the
> igt_require_gem_engine_list() which checks whether the ioctls are
> implemented in the running kernel. If not the function skips the
> test with -EINVAL.
> On the other hand, if the interfaces are implemented, then, at
> the first call it initializes an array of active engines (either
> physical or virtual).
>
> The global '*intel_active_engines2' points to the active engines
> array or to the existing 'intel_execution_engines2'. The latter
> is in preparation to the next patch.
>
> The value of the 'intel_active_engines2' will be used in further
> calls to check whether the above mentioned ioctls are implemented
> without the need to call getparam().
>
> Signed-off-by: Andi Shyti <andi.shyti at intel.com>
> ---
> lib/Makefile.sources | 2 +
> lib/i915/gem_query.c | 175 +++++++++++++++++++++++++++++++++++++++++++
> lib/i915/gem_query.h | 42 +++++++++++
> lib/igt_gt.c | 2 +-
> lib/igt_gt.h | 2 +-
> lib/meson.build | 1 +
> 6 files changed, 222 insertions(+), 2 deletions(-)
> create mode 100644 lib/i915/gem_query.c
> create mode 100644 lib/i915/gem_query.h
>
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 808b9617eca2..9dd5e9242ae4 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -11,6 +11,8 @@ lib_source_list = \
> i915/gem_submission.h \
> i915/gem_ring.h \
> i915/gem_ring.c \
> + i915/gem_query.c \
> + i915/gem_query.h \
> i915_3d.h \
> i915_reg.h \
> i915_pciids.h \
> diff --git a/lib/i915/gem_query.c b/lib/i915/gem_query.c
> new file mode 100644
> index 000000000000..2b7bb713c70a
> --- /dev/null
> +++ b/lib/i915/gem_query.c
> @@ -0,0 +1,175 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "ioctl_wrappers.h"
> +#include "drmtest.h"
> +
> +#include "i915/gem_query.h"
> +
> +struct intel_execution_engine2 *intel_active_engines2;
> +
> +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;
}
void gem_query(int fd, struct drm_i915_query *q)
{
igt_assert_eq(__gem_query(fd, q), 0);
}
> +
> +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!
> +{
> + igt_assert(!__gem_get_set_param(fd, request, p));
> +}
> +
> +bool gem_has_get_set_param(void)
Has what?
> +{
> + return intel_active_engines2 != intel_execution_engines2;
> +}
> +
> +static struct drm_i915_query_engine_info *query_engines(int fd)
> +{
> + struct drm_i915_query query = { };
> + struct drm_i915_query_item item = { };
> + struct drm_i915_query_engine_info *query_engines;
> +
> + 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...
> +
> + igt_assert((query_engines = calloc(1, item.length)));
> + item.data_ptr = to_user_pointer(query_engines);
> +
> + gem_query(fd, &query);
If we did have move than 64 engines, this fails with -EINVAL. I just
feel it's a failure mode we can handle. I tend to only make the
assumption for stack allocations, but for heap use an accurate double
pass.
> + return query_engines;
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> +{
> + int i;
> + const struct intel_execution_engine2 *e2;
> + struct drm_i915_gem_context_param ctx_param;
> + struct i915_context_param_engines *ctx_engine;
> +
> + if (!gem_has_get_set_param())
> + return;
> +
> + ctx_param.ctx_id = ctx_id;
> + ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> + ctx_param.size = sizeof(*ctx_engine) +
> + (I915_EXEC_RING_MASK - 1) *
> + sizeof(*ctx_engine->class_instance);
> +
> + igt_assert((ctx_engine = calloc(1, ctx_param.size)));
Here you know the size better than this estimate and can put it on the
stack.
> +
> + ctx_engine->extensions = 0;
> + for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {
> + 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);
> +
> + gem_setparam(fd, &ctx_param);
> +
> + free(ctx_engine);
> +}
> +
> +/*
> + * Initializes the list of engines.
> + *
> + * Returns:
> + *
> + * - 0 in case of success and the get/setparam ioctls are implemented
> + * - -EINVAL in case of success but get/setparam ioctls are not implemented
> + * - errno in case of failure
> + */
> +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.
> + 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.
> + /*
> + * 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;
Leaks
> + return ret;
> + }
> +
> + 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.
> + 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.
> +
> + free(query_engine);
> +
> + return 0;
> +}
> +
> +void igt_require_gem_engine_list(int fd)
> +{
> + igt_require_intel(fd);
Redundant.
> + igt_require(!gem_init_engine_list(fd));
> +}
> diff --git a/lib/i915/gem_query.h b/lib/i915/gem_query.h
> new file mode 100644
> index 000000000000..d927269c8aa9
> --- /dev/null
> +++ b/lib/i915/gem_query.h
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef GEM_QUERY_H
> +#define GEM_QUERY_H
> +
> +#include "igt_gt.h"
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id);
> +
> +bool gem_has_get_set_param(void);
> +void gem_query(int fd, struct drm_i915_query *q);
> +
> +void gem_get_set_param(int fd, unsigned long request, struct drm_i915_gem_context_param *p);
> +#define gem_setparam(f, p) gem_get_set_param(f, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, p)
> +#define gem_getparam(f, p) gem_get_set_param(f, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, p)
> +
> +void igt_require_gem_engine_list(int fd);
> +
> +extern struct intel_execution_engine2 *intel_active_engines2;
> +
> +#endif /* GEM_QUEY_H */
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 646696727ee4..2bfd6417b5e5 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,7 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> return true;
> }
>
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> +struct intel_execution_engine2 intel_execution_engines2[] = {
> { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da98084..f4bd6c22a81a 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -91,7 +91,7 @@ bool gem_ring_has_physical_engine(int fd, unsigned int ring);
>
> bool gem_can_store_dword(int fd, unsigned int engine);
>
> -extern const struct intel_execution_engine2 {
> +extern struct intel_execution_engine2 {
> const char *name;
> int class;
> int instance;
> diff --git a/lib/meson.build b/lib/meson.build
> index dd36f818033c..a703da2fb0d2 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -4,6 +4,7 @@ lib_sources = [
> 'i915/gem_scheduler.c',
> 'i915/gem_submission.c',
> 'i915/gem_ring.c',
> + 'i915/gem_query.c',
> 'igt_color_encoding.c',
> 'igt_debugfs.c',
> 'igt_device.c',
> --
> 2.20.1
>
More information about the igt-dev
mailing list