[igt-dev] [RFC PATCH v9 2/5] lib/i915: add gem_query library
Chris Wilson
chris at chris-wilson.co.uk
Thu Feb 14 09:42:50 UTC 2019
Quoting Andi Shyti (2019-02-14 00:44:42)
> 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 -ENODEV.
> 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 | 184 +++++++++++++++++++++++++++++++++++++++++++
> lib/i915/gem_query.h | 38 +++++++++
> lib/igt_gt.c | 2 +-
> lib/igt_gt.h | 2 +-
> lib/meson.build | 1 +
> 6 files changed, 227 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..e55f6fd94543
> --- /dev/null
> +++ b/lib/i915/gem_query.c
> @@ -0,0 +1,184 @@
> +/*
> + * 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"
> +
> +/*
> + * HACK: the '2 * sizeof(__u16)' is the size required for the flexible array
> + * inside 'struct i915_context_param_engines' that doesn't have a name and
> + * therefore cannot be referenced. It needs a name in the uapi.
> + */
It's not going to get one until you raise the point in review of the
uAPI. It doesn't though, you can use sizeof on the unnamed member.
> +#define SIZEOF_CTX_PARAM sizeof(struct i915_context_param_engines) + \
> + I915_EXEC_RING_MASK * \
> + 2 * sizeof(__u16)
For example:
SIZEOF_CTX_PARAM offsetofend(struct i915_context_param_engines,
class_instance[I915_EXEC_RING_MASK + 1])
To make it generic though, it needs __attribute__((packed))
(If the maximum index is I915_EXEC_RING_MASK, we need
I915_EXEC_RING_MASK + 1 storage.)
> +#define SIZEOF_QUERY sizeof(struct drm_i915_query_engine_info) + \
> + I915_EXEC_RING_MASK * \
> + sizeof(struct drm_i915_engine_info)
> +
> +struct intel_execution_engine2 *intel_active_engines2;
> +
> +static int __gem_query(int fd, struct drm_i915_query *q)
> +{
> + int err = 0;
> +
> + if(igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
^ kernel coding style applies (missed space)
> + err = -errno;
> + errno = 0;
> + return err;
> +}
> +
> +void gem_query(int fd, struct drm_i915_query *q)
> +{
> + igt_assert_eq(__gem_query(fd, q), 0);
> +}
> +
> +bool gem_can_query(void)
Still nothing to do with query per-se...
gem_has_engine_topology() ?
> +{
> + return intel_active_engines2 != intel_execution_engines2;
> +}
> +
> +static void query_engines(int fd,
> + struct drm_i915_query_engine_info *query_engines)
> +{
> + struct drm_i915_query query = { };
> + struct drm_i915_query_item item = { };
> +
> + item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> + query.items_ptr = to_user_pointer(&item);
> + query.num_items = 1;
> + item.length = SIZEOF_QUERY;
> +
> + item.data_ptr = to_user_pointer(query_engines);
> +
> + gem_query(fd, &query);
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
We've drifted out of the domain of i915/gem_query.c into
i915/gem_engine.c You could even argue we want to keep gem_engine.c for
lower level interactions, and say gem_engine_topology.c for the details of
the layout. Hmm, I like?
> +{
> + int i;
Kernel coding style is to order variable lines by length, longest first;
unless there is a clear reason why not to.
> + __u8 ctx_param_buffer[SIZEOF_CTX_PARAM] = { };
s/__u8/uint8_t/
It just a boring stack buf, so call it buf.
> + const struct intel_execution_engine2 *e2;
> + struct drm_i915_gem_context_param ctx_param;
> + struct i915_context_param_engines *ctx_engine;
> +
> + if (!gem_can_query())
> + return;
> +
> + ctx_engine = (struct i915_context_param_engines *) ctx_param_buffer;
> +
> + ctx_param.ctx_id = ctx_id;
> + ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> + ctx_param.size = SIZEOF_CTX_PARAM;
Not quite; size is used to determine the number of engines, this makes a
large array with upto 5 real engines followed by rcs0 repeated ~60
times. Interesting ;)
> + ctx_engine->extensions = 0;
> + for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {
You should at least have an assert you don't overflow your assumptions.
> + 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);
ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i]);
* quickly adds the packed attribute
> +
> + gem_context_set_param(fd, &ctx_param);
> +}
> +
> +/*
> + * Initializes the list of engines.
> + *
> + * Returns:
> + *
> + * - 0 in case of success and the get/set_param ioctls are implemented
> + * - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
> + * implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
> + * - -errno in case of failure
> + */
> +static int gem_init_engine_list(int fd)
> +{
> + int i, ret;
> + unsigned char query_buffer[SIZEOF_QUERY] = { };
> + struct drm_i915_query_engine_info *query_engine;
> + const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
> + struct drm_i915_gem_context_param ctx_param = {
> + .param = I915_CONTEXT_PARAM_ENGINES,
> + };
> +
> + /* the list is already initialized */
> + if (intel_active_engines2)
> + return gem_can_query() ? 0 : -ENODEV;
> +
> + /*
> + * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
> + * supported by the running kernel. If not, __gem_context_get_param()
> + * will return -EINVAL which, at this point, is not necessarily a
> + * failure but it means that we need to fall beck to polling the engines
> + * directly from intel_execution_engines2[].
> + *
> + * We will return -ENODEV with the meaning of missing interface
> + */
> + ret = __gem_context_get_param(fd, &ctx_param);
> + if (ret) {
> + if (ret == -EINVAL) {
Just
if (__gem_context_get_param()) {
intel_active_engines2 = intel_execution_engines2;
return -ENODEV;
}
This be library code, so you define the interface; as opposed to test
code where we demand certain responses from the kernel.
> + query_engine = (struct drm_i915_query_engine_info *) query_buffer;
> + query_engines(fd, query_engine);
> +
> + intel_active_engines2 = malloc((query_engine->num_engines + 1) *
> + sizeof(*intel_active_engines2));
> + igt_assert(intel_active_engines2);
> +
> + 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(class_names) && class >= 0);
> + igt_assert(class_names[class]);
Be future proof;
if ((unsigned)class >= ARRAY_SIZE() || !class_names[class])
tmpl = "xxx"; // or "unk" I think I've seen Tvrtko use
else
tmpl = class_names[class];
Bonus points to include class-id in tmpl in case there's more than one!
> + intel_active_engines2[i].class = class;
> + intel_active_engines2[i].instance = instance;
> +
> + igt_assert(asprintf(&name, "%s%d",
> + class_names[class], instance) > 0);
> + intel_active_engines2[i].name = name;
> + }
> +
> + /* '0' value sentinel */
Which bit?
class_instance = (0, 0) is valid (rcs0)
/* Use .name=NULL as a sentinel */
> + intel_active_engines2[i].name = NULL;
> + intel_active_engines2[i].class = 0;
> + intel_active_engines2[i].instance = 0;
> +
> + return 0;
> +}
> +
> +void igt_require_gem_engine_list(int fd)
> +{
> + 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..263c7bd5c6e2
> --- /dev/null
> +++ b/lib/i915/gem_query.h
> @@ -0,0 +1,38 @@
> +/*
> + * 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"
For intel_execution_engine2?
Just a forward decl will do.
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id);
One day you'll find a more fitting name. Hint, this isn't part of the
"set" infrastructure.
> +bool gem_can_query(void);
> +void gem_query(int fd, struct drm_i915_query *q);
> +
> +void igt_require_gem_engine_list(int fd);
> +
> +extern struct intel_execution_engine2 *intel_active_engines2;
But I'm voting this extern should be in gem_engine_topology.[ch]
-Chris
More information about the igt-dev
mailing list