[igt-dev] [PATCH i-g-t 09/93] lib: Add an intel_ctx wrapper struct and helpers (v5)
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Thu Jun 10 03:42:28 UTC 2021
On Wed, Jun 09, 2021 at 12:35:52PM -0500, Jason Ekstrand wrote:
> We're trying to clean up some of our technical debt in the i915 API. In
> particular, context mutability and unnecessary getparam(). There's
> quite a bit of the introspection stuff that's not used by any userspace
> other than IGT. Most drivers don't care about fetching the set of
> engines, for instance, because they don't forget about what set of
> engines they asked for int the first place.
>
> Unfortunately, IGT relies heavily on context introspection for just
> about everything when it comes to multi-engine testing. It also likes
> to use ctx0 as temporary storage for whatever the current test config
> is. While effective at keeping IGC simple in some ways, this means
> we're making heavy use of context mutability. Also, passing data around
> with in tests isn't really what contexts are for.
>
> This patch adds a new intel_ctx_t struct which wraps a context and
> remembers the full context configuration. This will provide similar
> ease-of-use without having use ctx0 as temporary storage.
>
> v2 (Jason Ekstrand):
> - Make all intel_ctx_t's const
>
> v3 (Jason Ekstrand):
> - Fix up the docs so they build properly
>
> v4 (Jason Ekstrand):
> - Add an intel_ctx_create_for_engine helper
>
> v5 (Zbigniew Kempczyński):
> - Use SPDX license identifiers
> - Document default context semantics
>
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> ---
> .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 +
> lib/intel_ctx.c | 255 ++++++++++++++++++
> lib/intel_ctx.h | 57 ++++
> lib/meson.build | 1 +
> 4 files changed, 314 insertions(+)
> create mode 100644 lib/intel_ctx.c
> create mode 100644 lib/intel_ctx.h
>
> diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
> index 2e85f361..0f49b4a3 100644
> --- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
> +++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
> @@ -58,6 +58,7 @@
> <xi:include href="xml/gem_engine_topology.xml"/>
> <xi:include href="xml/gem_scheduler.xml"/>
> <xi:include href="xml/gem_submission.xml"/>
> + <xi:include href="xml/intel_ctx.xml"/>
> </chapter>
> <xi:include href="xml/igt_test_programs.xml"/>
>
> diff --git a/lib/intel_ctx.c b/lib/intel_ctx.c
> new file mode 100644
> index 00000000..6789ef9a
> --- /dev/null
> +++ b/lib/intel_ctx.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include <stddef.h>
> +
> +#include "intel_ctx.h"
> +#include "ioctl_wrappers.h"
> +#include "i915/gem_engine_topology.h"
> +
> +/**
> + * SECTION:intel_ctx
> + * @short_description: Wrapper structs for dealing with contexts
> + * @title: Intel Context Wrapper
> + *
> + * This helper library contains a couple of wrapper structs for easier
> + * dealing with GEM contexts. This includes a context configuration struct
> + * which represents important context construction parameters and a context
> + * struct which contains the context ID and its configuration. This makes
> + * it easier to pass around a context without loosing the context create
> + * information.
> + */
> +
> +static void
> +add_user_ext(uint64_t *root_ext_u64, struct i915_user_extension *ext)
> +{
> + ext->next_extension = *root_ext_u64;
> + *root_ext_u64 = to_user_pointer(ext);
> +}
> +
> +static size_t sizeof_param_engines(int count)
> +{
> + return offsetof(struct i915_context_param_engines, engines[count]);
> +}
> +
> +#define SIZEOF_QUERY offsetof(struct drm_i915_query_engine_info, \
> + engines[GEM_MAX_ENGINES])
> +
> +/**
> + * intel_ctx_cfg_all_physical:
> + * @fd: open i915 drm file descriptor
> + *
> + * Returns an intel_ctx_cfg_t containing all physical engines. On kernels
> + * without the engines API, a default context configuration will be
> + * returned.
> + */
> +intel_ctx_cfg_t intel_ctx_cfg_all_physical(int fd)
> +{
> + uint8_t buff[SIZEOF_QUERY] = { };
> + struct drm_i915_query_engine_info *qei = (void *) buff;
> + intel_ctx_cfg_t cfg = {};
> + int i;
> +
> + if (__gem_query_engines(fd, qei, SIZEOF_QUERY) == 0) {
> + cfg.num_engines = qei->num_engines;
> + for (i = 0; i < qei->num_engines; i++)
> + cfg.engines[i] = qei->engines[i].engine;
> + }
> +
> + return cfg;
> +}
> +
> +/**
> + * intel_ctx_cfg_for_engine:
> + * @class: engine class
> + * @inst: engine instance
> + *
> + * Returns an intel_ctx_cfg_t containing exactly one engine.
> + */
> +intel_ctx_cfg_t intel_ctx_cfg_for_engine(unsigned int class, unsigned int inst)
> +{
> + return (intel_ctx_cfg_t) {
> + .num_engines = 1,
> + .engines = {
> + { .engine_class = class, .engine_instance = inst },
> + },
> + };
> +}
> +
> +static int
> +__context_create_cfg(int fd, const intel_ctx_cfg_t *cfg, uint32_t *ctx_id)
> +{
> + uint64_t ext_root = 0;
> + I915_DEFINE_CONTEXT_PARAM_ENGINES(engines, GEM_MAX_ENGINES);
> + struct drm_i915_gem_context_create_ext_setparam engines_param, vm_param;
> + uint32_t i;
> +
> + if (cfg->vm) {
> + vm_param = (struct drm_i915_gem_context_create_ext_setparam) {
> + .base = {
> + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> + },
> + .param = {
> + .param = I915_CONTEXT_PARAM_VM,
> + .value = cfg->vm,
> + },
> + };
> + add_user_ext(&ext_root, &vm_param.base);
> + }
> +
> + if (cfg->num_engines) {
> + memset(&engines, 0, sizeof(engines));
> + for (i = 0; i < cfg->num_engines; i++)
> + engines.engines[i] = cfg->engines[i];
> +
> + engines_param = (struct drm_i915_gem_context_create_ext_setparam) {
> + .base = {
> + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> + },
> + .param = {
> + .param = I915_CONTEXT_PARAM_ENGINES,
> + .size = sizeof_param_engines(cfg->num_engines),
> + .value = to_user_pointer(&engines),
> + },
> + };
> + add_user_ext(&ext_root, &engines_param.base);
> + }
> +
> + return __gem_context_create_ext(fd, cfg->flags, ext_root, ctx_id);
> +}
> +
> +/**
> + * __intel_ctx_create:
> + * @fd: open i915 drm file descriptor
> + * @cfg: configuration for the created context
> + * @out_ctx: on success, the new intel_ctx_t pointer is written here
> + *
> + * Like intel_ctx_create but returns an error instead of asserting.
> + */
> +int __intel_ctx_create(int fd, const intel_ctx_cfg_t *cfg,
> + const intel_ctx_t **out_ctx)
> +{
> + uint32_t ctx_id;
> + intel_ctx_t *ctx;
> + int err;
> +
> + if (cfg)
> + err = __context_create_cfg(fd, cfg, &ctx_id);
> + else
> + err = __gem_context_create(fd, &ctx_id);
> + if (err)
> + return err;
> +
> + ctx = calloc(1, sizeof(*ctx));
> + igt_assert(ctx);
> +
> + ctx->id = ctx_id;
> + if (cfg)
> + ctx->cfg = *cfg;
> +
> + *out_ctx = ctx;
> + return 0;
> +}
> +
> +/**
> + * intel_ctx_create:
> + * @fd: open i915 drm file descriptor
> + * @cfg: configuration for the created context
> + *
> + * Creates a new intel_ctx_t with the given config. If @cfg is NULL, a
> + * default context is created.
> + */
> +const intel_ctx_t *intel_ctx_create(int fd, const intel_ctx_cfg_t *cfg)
> +{
> + const intel_ctx_t *ctx;
> + int err;
> +
> + err = __intel_ctx_create(fd, cfg, &ctx);
> + igt_assert_eq(err, 0);
> +
> + return ctx;
> +}
> +
> +static const intel_ctx_t __intel_ctx_0 = {};
> +
> +/**
> + * intel_ctx_0:
> + * @fd: open i915 drm file descriptor
> + *
> + * Returns an intel_ctx_t representing the default context.
> + */
> +const intel_ctx_t *intel_ctx_0(int fd)
> +{
> + (void)fd;
> + return &__intel_ctx_0;
> +}
> +
> +/**
> + * intel_ctx_create_for_engine:
> + * @fd: open i915 drm file descriptor
> + * @class: engine class
> + * @inst: engine instance
> + *
> + * Returns an intel_ctx_t containing the specified engine.
> + */
> +const intel_ctx_t *
> +intel_ctx_create_for_engine(int fd, unsigned int class, unsigned int inst)
> +{
> + intel_ctx_cfg_t cfg = intel_ctx_cfg_for_engine(class, inst);
> + return intel_ctx_create(fd, &cfg);
> +}
> +
> +/**
> + * intel_ctx_create_all_physical:
> + * @fd: open i915 drm file descriptor
> + *
> + * Creates an intel_ctx_t containing all physical engines. On kernels
> + * without the engines API, the created context will be the same as
> + * intel_ctx_0() except that it will be a new GEM context. On kernels or
> + * hardware which do not support contexts, it is the same as intel_ctx_0().
> + */
> +const intel_ctx_t *intel_ctx_create_all_physical(int fd)
> +{
> + intel_ctx_cfg_t cfg;
> +
> + if (!gem_has_contexts(fd))
> + return intel_ctx_0(fd);
> +
> + cfg = intel_ctx_cfg_all_physical(fd);
> + return intel_ctx_create(fd, &cfg);
> +}
> +
> +/**
> + * intel_ctx_destroy:
> + * @fd: open i915 drm file descriptor
> + * @ctx: context to destroy, or NULL
> + *
> + * Destroys an intel_ctx_t.
> + */
> +void intel_ctx_destroy(int fd, const intel_ctx_t *ctx)
> +{
> + if (!ctx || ctx->id == 0)
> + return;
> +
> + gem_context_destroy(fd, ctx->id);
> + free((void *)ctx);
> +}
> +
> +/**
> + * intel_ctx_engine_class:
> + * @ctx: an intel_ctx_t
> + * @engine: an engine specifier
> + *
> + * Returns the class for the given engine.
> + */
> +unsigned int intel_ctx_engine_class(const intel_ctx_t *ctx, unsigned int engine)
> +{
> + if (ctx->cfg.num_engines) {
> + igt_assert(engine < ctx->cfg.num_engines);
> + return ctx->cfg.engines[engine].engine_class;
> + } else {
> + return gem_execbuf_flags_to_engine_class(engine);
> + }
> +}
> diff --git a/lib/intel_ctx.h b/lib/intel_ctx.h
> new file mode 100644
> index 00000000..3e059478
> --- /dev/null
> +++ b/lib/intel_ctx.h
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#ifndef INTEL_CTX_H
> +#define INTEL_CTX_H
> +
> +#include "igt_core.h"
> +
> +#include "i915_drm.h"
> +
> +#define GEM_MAX_ENGINES I915_EXEC_RING_MASK + 1
> +
> +/**
> + * intel_ctx_cfg_t:
> + * @flags: Context create flags
> + * @vm: VM to inherit or 0 for using a per-context VM
> + * @num_engines: Number of client-specified engines or 0 for legacy mode
> + * @engines: Client-specified engines
> + *
> + * Represents the full configuration of an intel_ctx.
> + */
> +typedef struct intel_ctx_cfg {
> + uint32_t flags;
> + uint32_t vm;
> + unsigned int num_engines;
> + struct i915_engine_class_instance engines[GEM_MAX_ENGINES];
> +} intel_ctx_cfg_t;
> +
> +intel_ctx_cfg_t intel_ctx_cfg_for_engine(unsigned int class, unsigned int inst);
> +intel_ctx_cfg_t intel_ctx_cfg_all_physical(int fd);
> +
> +/**
> + * intel_ctx_t:
> + * @id: the context id/handle
> + * @cfg: the config used to create this context
> + *
> + * Represents the full configuration of an intel_ctx.
> + */
> +typedef struct intel_ctx {
> + uint32_t id;
> + intel_ctx_cfg_t cfg;
> +} intel_ctx_t;
> +
> +int __intel_ctx_create(int fd, const intel_ctx_cfg_t *cfg,
> + const intel_ctx_t **out_ctx);
> +const intel_ctx_t *intel_ctx_create(int i915, const intel_ctx_cfg_t *cfg);
> +const intel_ctx_t *intel_ctx_0(int fd);
> +const intel_ctx_t *intel_ctx_create_for_engine(int fd, unsigned int class,
> + unsigned int inst);
> +const intel_ctx_t *intel_ctx_create_all_physical(int fd);
> +void intel_ctx_destroy(int fd, const intel_ctx_t *ctx);
> +
> +unsigned int intel_ctx_engine_class(const intel_ctx_t *ctx, unsigned int engine);
> +
> +#endif
> diff --git a/lib/meson.build b/lib/meson.build
> index 078a357b..67d40512 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -43,6 +43,7 @@ lib_sources = [
> 'intel_batchbuffer.c',
> 'intel_bufops.c',
> 'intel_chipset.c',
> + 'intel_ctx.c',
> 'intel_device_info.c',
> 'intel_os.c',
> 'intel_mmio.c',
> --
> 2.31.1
Ok, I haven't spotted anything wrong here.
Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
--
Zbigniew
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list