[igt-dev] [PATCH i-g-t 09/89] lib: Add an intel_ctx wrapper struct and helpers (v4)
Jason Ekstrand
jason at jlekstrand.net
Wed Jun 9 14:50:54 UTC 2021
On Mon, May 17, 2021 at 3:48 AM Zbigniew Kempczyński
<zbigniew.kempczynski at intel.com> wrote:
>
> On Fri, Apr 23, 2021 at 04:47:33PM -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
> >
> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > ---
> > .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 +
> > lib/intel_ctx.c | 272 ++++++++++++++++++
> > lib/intel_ctx.h | 75 +++++
> > lib/meson.build | 1 +
> > 4 files changed, 349 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..271bb812
> > --- /dev/null
> > +++ b/lib/intel_ctx.c
> > @@ -0,0 +1,272 @@
> > +/*
> > + * Copyright © 2021 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.
> > + */
>
> We likely should use SPDX here.
>
> > +
> > +#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;
>
> So we loose flags and capabilities here. I've walked through the code
> and caps are related to HEVC decoding and SFC. I don't think we will
> have some embedded test h265 stream soon in IGT so this is likely
> not a problem. Some doubts comes with SFC because we already have
> copyfunc which is using VEBOX engine. I'm not sure how much effort
> do we need to scale / convert using SFC in this pipeline and do we
> really need to test this in IGT. If you think we need only engine
> class/instance I got no objections here.
>
> > + }
> > +
> > + 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;
>
> This is public and wouldn't assume cfg won't be NULL here. I would just
> add igt_assert() to avoid surprises.
Not sure what you mean. The intention is that intel_ctx_create(NULL)
gives you a default context; the same as if you zero-initialized the
config. There are a bunch of cases where this short-hand is really
useful. Maybe I should document that.
> > +
> > + 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
> > + */
> > +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;
>
> Shouldn't we assert here instead of silence quit on !ctx?
We can. There are some cases where it's nice to have the free()
semantics of noop-on-NULL but I could go either way. I can change it
if you'd like; it'll mean changing a few tests, though.
--Jason
> > +
> > + 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..79bc1154
> > --- /dev/null
> > +++ b/lib/intel_ctx.h
> > @@ -0,0 +1,75 @@
> > +/*
> > + * Copyright © 2021 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.
> > + */
>
> Use SPDX.
>
> --
> Zbigniew
>
> > +
> > +#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 9929520e..d66748ad 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -42,6 +42,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
> >
> > _______________________________________________
> > 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