[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:47:43 UTC 2021
On Thu, Jun 3, 2021 at 4:23 AM Zbigniew Kempczyński
<zbigniew.kempczynski at intel.com> wrote:
>
> On Wed, Jun 02, 2021 at 10:15:52AM -0500, Jason Ekstrand wrote:
> > On Wed, Jun 2, 2021 at 7:16 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.
> > > > + */
> > > > +
> > > > +#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
> > > > + */
> > > > +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;
> > > > +}
> > >
> > > What would you say about:
> > >
> > > const intel_ctx_t *intel_ctx_0(int fd)
> > > {
> > > intel_ctx_t *ctx;
> > >
> > > if (!gem_has_contexts(fd))
> > > return &__intel_ctx_0;
> > >
> > > ctx = calloc(1, sizeof(*ctx));
> > > igt_assert(ctx);
> > >
> > > ctx->cfg = intel_ctx_cfg_all_physical(fd);
> >
> > The problem is that this isn't actually the right config for ctx0. :-(
> > More on that in a bit.
> >
> > > return ctx;
> > > }
> > >
> > > and
> > >
> > > void intel_ctx_destroy(int fd, const intel_ctx_t *ctx)
> > > {
> > > igt_assert(ctx);
> > >
> > > if (ctx->id)
> > > gem_context_destroy(fd, ctx->id);
> > >
> > > if (ctx != &__intel_ctx_0)
> > > free((void *)ctx);
> > > }
> > >
> > > We could avoid a lot of changes in tests which currently
> > > use default context, for example in gem_exec_nop.c instead:
> > >
> > > ctx = intel_ctx_create_all_physical(device);
> > >
> > > just
> > > ctx = intel_ctx_0(device);
> > >
> > > in opening igt_fixture. We still would have engines field filled
> > > from engine query if kernel supports it.
> >
> > Unfortunately, as I alluded to above, this doesn't actually work.
> > This is where the current test iterators are dangerously subtle and it
> > took me a while to figure out what they were doing. By default, a
> > context doesn't have the all_physical engine set. The all_physical
> > engine set is achieved by querying the kernel for the set of engines
> > and setting up a userspace-managed engine set on the context via
> > CONTEXT_PARAM_ENGINES. By default, however, contexts have the default
> > set of engines and use the legacy ring flag API.
> >
> > How does this work in all those tests, then? Am I making serious
> > functional changes and iterating over engines differently? No, I'm
> > avoiding serious functional changes. Those tests work by first
> > swapping out the engine set on ctx0 via SET_CONTEXT_PARAM with
> > CONTEXT_PARAM_ENGINES and then iterating over that engine set. So,
> > yes, they technically use ctx0 but it's a whole new context by the
> > time you get to the first execbuf.
> >
> > > This way we can avoid unnecessary changes (like in nop_on_ring())
> > > passing context and using it within execbuf. Especially intel_ctx
> > > you've proposed enforces using ctx->id in all execbufs making them
> > > a little bit prone to bug if someone would just forget setting
> > > rsvd1 field.
> >
> > Yeah, I know all those changes are annoying. I typed them! But one
> > of the things I tried very hard to do is to ensure that every time we
> > have an intel_execution_engine2* or a flags parameter that was really
> > an engine specifier, it's accompanied by the intel_ctx_t* or the
> > intel_ctx_cfg_t* it's relative to. No intel_execution_engine2* or
> > flags parameter is useful by itself, it's always relative to the
> > configuration of the context because the meaning of those flags
> > changes based on the configured engine set.
> >
> > I hope that makes the over-all change make more sense.
> >
> > --Jason
>
> As we're not able to set_engines() with your kernel patch anymore what
> is blocking us from returning all physical engines in default_engines()
> in the kernel? This would give us ctx0 armed with all engines and we
> could leave a lot of igt code intact.
default_engines() returns a set of engines which corresponds to the
I915_EXEC_RENDER, I915_EXEC_BLT, and friends. If we have, say,
hardware with 4 video engines, it can't be expressed through that
interface. Sadly, it's fixed to the 5 engines we have explicit flags
for, if they exist.
--Jason
> --
> Zbigniew
>
>
> >
> > > --
> > > Zbigniew
> > >
> > >
> > > > +
> > > > +/**
> > > > + * 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..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.
> > > > + */
> > > > +
> > > > +#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