[Intel-gfx] [PATCH 07/23] drm/i915/gt: Move the [class][inst] lookup for engines onto the GT
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Jul 26 09:22:08 UTC 2019
On 23/07/2019 19:38, Chris Wilson wrote:
> To maintain a fast lookup from a GT centric irq handler, we want the
> engine lookup tables on the intel_gt. To avoid having multiple copies of
> the same multi-dimension lookup table, move the generic user engine
> lookup into an rbtree (for fast and flexible indexing).
>
> v2: Split uabi_instance cf uabi_class
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 3 +-
> drivers/gpu/drm/i915/gt/intel_engine.h | 3 -
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 53 +++++-----------
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 9 ++-
> drivers/gpu/drm/i915/gt/intel_engine_user.c | 66 ++++++++++++++++++++
> drivers/gpu/drm/i915/gt/intel_engine_user.h | 20 ++++++
> drivers/gpu/drm/i915/gt/intel_gt_types.h | 4 ++
> drivers/gpu/drm/i915/gt/selftest_lrc.c | 15 +++--
> drivers/gpu/drm/i915/i915_drv.h | 7 ++-
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/i915_pmu.c | 3 +-
> drivers/gpu/drm/i915/i915_query.c | 2 +-
> drivers/gpu/drm/i915/i915_trace.h | 10 +--
> 14 files changed, 138 insertions(+), 60 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_user.c
> create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_user.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 524516251a40..fafc3763dc2d 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -74,6 +74,7 @@ gt-y += \
> gt/intel_context.o \
> gt/intel_engine_cs.o \
> gt/intel_engine_pm.o \
> + gt/intel_engine_user.o \
> gt/intel_gt.o \
> gt/intel_gt_pm.o \
> gt/intel_hangcheck.o \
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 18b226bc5e3a..e31431fa141e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -70,6 +70,7 @@
> #include <drm/i915_drm.h>
>
> #include "gt/intel_lrc_reg.h"
> +#include "gt/intel_engine_user.h"
>
> #include "i915_gem_context.h"
> #include "i915_globals.h"
> @@ -1740,7 +1741,7 @@ get_engines(struct i915_gem_context *ctx,
>
> if (e->engines[n]) {
> ci.engine_class = e->engines[n]->engine->uabi_class;
> - ci.engine_instance = e->engines[n]->engine->instance;
> + ci.engine_instance = e->engines[n]->engine->uabi_instance;
> }
>
> if (copy_to_user(&user->engines[n], &ci, sizeof(ci))) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index db5c73ce86ee..30856383e4c5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -432,9 +432,6 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> struct drm_printer *m,
> const char *header, ...);
>
> -struct intel_engine_cs *
> -intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
> -
> static inline void intel_engine_context_in(struct intel_engine_cs *engine)
> {
> unsigned long flags;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 65cbf1d9118d..ed5c4e161e6e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -32,6 +32,7 @@
>
> #include "intel_engine.h"
> #include "intel_engine_pm.h"
> +#include "intel_engine_user.h"
> #include "intel_context.h"
> #include "intel_lrc.h"
> #include "intel_reset.h"
> @@ -285,9 +286,7 @@ static void intel_engine_sanitize_mmio(struct intel_engine_cs *engine)
> intel_engine_set_hwsp_writemask(engine, ~0u);
> }
>
> -static int
> -intel_engine_setup(struct drm_i915_private *dev_priv,
> - enum intel_engine_id id)
> +static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> {
> const struct engine_info *info = &intel_engines[id];
> struct intel_engine_cs *engine;
> @@ -303,10 +302,9 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
> return -EINVAL;
>
> - if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance]))
> + if (GEM_DEBUG_WARN_ON(gt->engine_class[info->class][info->instance]))
> return -EINVAL;
>
> - GEM_BUG_ON(dev_priv->engine[id]);
> engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> if (!engine)
> return -ENOMEM;
> @@ -315,12 +313,12 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>
> engine->id = id;
> engine->mask = BIT(id);
> - engine->i915 = dev_priv;
> - engine->gt = &dev_priv->gt;
> - engine->uncore = &dev_priv->uncore;
> + engine->i915 = gt->i915;
> + engine->gt = gt;
> + engine->uncore = gt->uncore;
> __sprint_engine_name(engine->name, info);
> engine->hw_id = engine->guc_id = info->hw_id;
> - engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases);
> + engine->mmio_base = __engine_mmio_base(gt->i915, info->mmio_bases);
> engine->class = info->class;
> engine->instance = info->instance;
>
> @@ -331,13 +329,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> engine->destroy = (typeof(engine->destroy))kfree;
>
> engine->uabi_class = intel_engine_classes[info->class].uabi_class;
> + engine->uabi_instance = info->instance;
>
> - engine->context_size = intel_engine_context_size(dev_priv,
> + engine->context_size = intel_engine_context_size(gt->i915,
> engine->class);
> if (WARN_ON(engine->context_size > BIT(20)))
> engine->context_size = 0;
> if (engine->context_size)
> - DRIVER_CAPS(dev_priv)->has_logical_contexts = true;
> + DRIVER_CAPS(gt->i915)->has_logical_contexts = true;
>
> /* Nothing to do here, execute in order of dependencies */
> engine->schedule = NULL;
> @@ -349,8 +348,11 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> /* Scrub mmio state on takeover */
> intel_engine_sanitize_mmio(engine);
>
> - dev_priv->engine_class[info->class][info->instance] = engine;
> - dev_priv->engine[id] = engine;
> + engine->gt->engine_class[info->class][info->instance] = engine;
No need to go through engine to gt here.
> +
> + intel_engine_add_user(engine);
> + gt->i915->engine[id] = engine;
> +
> return 0;
> }
>
> @@ -433,7 +435,7 @@ int intel_engines_init_mmio(struct drm_i915_private *i915)
> if (!HAS_ENGINE(i915, i))
> continue;
>
> - err = intel_engine_setup(i915, i);
> + err = intel_engine_setup(&i915->gt, i);
> if (err)
> goto cleanup;
>
> @@ -1501,29 +1503,6 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> intel_engine_print_breadcrumbs(engine, m);
> }
>
> -static u8 user_class_map[] = {
> - [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
> - [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
> - [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
> - [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
> -};
> -
> -struct intel_engine_cs *
> -intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
> -{
> - if (class >= ARRAY_SIZE(user_class_map))
> - return NULL;
> -
> - class = user_class_map[class];
> -
> - GEM_BUG_ON(class > MAX_ENGINE_CLASS);
> -
> - if (instance > MAX_ENGINE_INSTANCE)
> - return NULL;
> -
> - return i915->engine_class[class][instance];
> -}
> -
> /**
> * intel_enable_engine_stats() - Enable engine busy tracking on engine
> * @engine: engine to enable stats collection
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 8be63019d707..9c927fa408aa 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -12,6 +12,7 @@
> #include <linux/kref.h>
> #include <linux/list.h>
> #include <linux/llist.h>
> +#include <linux/rbtree.h>
> #include <linux/timer.h>
> #include <linux/types.h>
>
> @@ -267,15 +268,19 @@ struct intel_engine_cs {
> unsigned int guc_id;
> intel_engine_mask_t mask;
>
> - u8 uabi_class;
> -
> u8 class;
> u8 instance;
> +
> + u8 uabi_class;
> + u8 uabi_instance;
> +
> u32 context_size;
> u32 mmio_base;
>
> u32 uabi_capabilities;
>
> + struct rb_node uabi_node;
> +
> struct intel_sseu sseu;
>
> struct intel_ring *buffer;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> new file mode 100644
> index 000000000000..f74fb4d2fa0d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -0,0 +1,66 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_engine.h"
> +#include "intel_engine_user.h"
> +
> +struct intel_engine_cs *
> +intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
> +{
> + struct rb_node *p = i915->uabi_engines.rb_node;
> +
> + while (p) {
> + struct intel_engine_cs *it =
> + rb_entry(p, typeof(*it), uabi_node);
> +
> + if (class < it->uabi_class)
> + p = p->rb_left;
> + else if (class > it->uabi_class ||
> + instance > it->uabi_instance)
> + p = p->rb_right;
> + else if (instance < it->uabi_instance)
> + p = p->rb_left;
> + else
> + return it;
> + }
> +
> + return NULL;
> +}
> +
> +void intel_engine_add_user(struct intel_engine_cs *engine)
> +{
> + struct rb_root *root = &engine->i915->uabi_engines;
> + struct rb_node **p, *parent;
> +
> + parent = NULL;
> + p = &root->rb_node;
> + while (*p) {
> + struct intel_engine_cs *it;
> +
> + parent = *p;
> + it = rb_entry(parent, typeof(*it), uabi_node);
> +
> + /* All user class:instance identifiers must be unique */
> + GEM_BUG_ON(it->uabi_class == engine->uabi_class &&
> + it->uabi_instance == engine->uabi_instance);
> +
> + if (engine->uabi_class < it->uabi_class)
> + p = &parent->rb_left;
> + else if (engine->uabi_class > it->uabi_class ||
> + engine->uabi_instance > it->uabi_instance)
> + p = &parent->rb_right;
> + else
> + p = &parent->rb_left;
> + }
> +
> + rb_link_node(&engine->uabi_node, parent, p);
> + rb_insert_color(&engine->uabi_node, root);
> +
> + GEM_BUG_ON(intel_engine_lookup_user(engine->i915,
> + engine->uabi_class,
> + engine->uabi_instance) != engine);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.h b/drivers/gpu/drm/i915/gt/intel_engine_user.h
> new file mode 100644
> index 000000000000..091dc8a4a39f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.h
> @@ -0,0 +1,20 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef INTEL_ENGINE_USER_H
> +#define INTEL_ENGINE_USER_H
> +
> +#include <linux/types.h>
> +
> +struct drm_i915_private;
> +struct intel_engine_cs;
> +
> +struct intel_engine_cs *
> +intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
> +
> +void intel_engine_add_user(struct intel_engine_cs *engine);
> +
> +#endif /* INTEL_ENGINE_USER_H */
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 34d4a868e4f1..5fd11e361d03 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -21,6 +21,7 @@
>
> struct drm_i915_private;
> struct i915_ggtt;
> +struct intel_engine_cs;
> struct intel_uncore;
>
> struct intel_hangcheck {
> @@ -76,6 +77,9 @@ struct intel_gt {
> u32 pm_ier;
>
> u32 pm_guc_events;
> +
> + struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1]
> + [MAX_ENGINE_INSTANCE + 1];
> };
>
> enum intel_gt_scratch_field {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 60f27e52d267..eb40a58665be 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -1773,6 +1773,7 @@ static int live_virtual_engine(void *arg)
> struct drm_i915_private *i915 = arg;
> struct intel_engine_cs *siblings[MAX_ENGINE_INSTANCE + 1];
> struct intel_engine_cs *engine;
> + struct intel_gt *gt = &i915->gt;
> enum intel_engine_id id;
> unsigned int class, inst;
> int err = -ENODEV;
> @@ -1796,10 +1797,10 @@ static int live_virtual_engine(void *arg)
>
> nsibling = 0;
> for (inst = 0; inst <= MAX_ENGINE_INSTANCE; inst++) {
> - if (!i915->engine_class[class][inst])
> + if (!gt->engine_class[class][inst])
> continue;
>
> - siblings[nsibling++] = i915->engine_class[class][inst];
> + siblings[nsibling++] = gt->engine_class[class][inst];
> }
> if (nsibling < 2)
> continue;
> @@ -1920,6 +1921,7 @@ static int live_virtual_mask(void *arg)
> {
> struct drm_i915_private *i915 = arg;
> struct intel_engine_cs *siblings[MAX_ENGINE_INSTANCE + 1];
> + struct intel_gt *gt = &i915->gt;
> unsigned int class, inst;
> int err = 0;
>
> @@ -1933,10 +1935,10 @@ static int live_virtual_mask(void *arg)
>
> nsibling = 0;
> for (inst = 0; inst <= MAX_ENGINE_INSTANCE; inst++) {
> - if (!i915->engine_class[class][inst])
> + if (!gt->engine_class[class][inst])
> break;
>
> - siblings[nsibling++] = i915->engine_class[class][inst];
> + siblings[nsibling++] = gt->engine_class[class][inst];
> }
> if (nsibling < 2)
> continue;
> @@ -2097,6 +2099,7 @@ static int live_virtual_bond(void *arg)
> };
> struct drm_i915_private *i915 = arg;
> struct intel_engine_cs *siblings[MAX_ENGINE_INSTANCE + 1];
> + struct intel_gt *gt = &i915->gt;
> unsigned int class, inst;
> int err = 0;
>
> @@ -2111,11 +2114,11 @@ static int live_virtual_bond(void *arg)
>
> nsibling = 0;
> for (inst = 0; inst <= MAX_ENGINE_INSTANCE; inst++) {
> - if (!i915->engine_class[class][inst])
> + if (!gt->engine_class[class][inst])
> break;
>
> GEM_BUG_ON(nsibling == ARRAY_SIZE(siblings));
> - siblings[nsibling++] = i915->engine_class[class][inst];
> + siblings[nsibling++] = gt->engine_class[class][inst];
> }
> if (nsibling < 2)
> continue;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 269a1b32b48b..12a7fdabc2f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1371,11 +1371,12 @@ struct drm_i915_private {
> wait_queue_head_t gmbus_wait_queue;
>
> struct pci_dev *bridge_dev;
> - struct intel_engine_cs *engine[I915_NUM_ENGINES];
> +
> /* Context used internally to idle the GPU and setup initial state */
> struct i915_gem_context *kernel_context;
> - struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1]
> - [MAX_ENGINE_INSTANCE + 1];
> +
> + struct intel_engine_cs *engine[I915_NUM_ENGINES];
> + struct rb_root uabi_engines;
>
> struct resource mch_res;
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 11c73af92597..4d98e8597637 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3109,7 +3109,7 @@ gen11_engine_irq_handler(struct intel_gt *gt, const u8 class,
> struct intel_engine_cs *engine;
>
> if (instance <= MAX_ENGINE_INSTANCE)
> - engine = gt->i915->engine_class[class][instance];
> + engine = gt->engine_class[class][instance];
> else
> engine = NULL;
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index eff86483bec0..bdf7963a043b 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -8,6 +8,7 @@
> #include <linux/pm_runtime.h>
>
> #include "gt/intel_engine.h"
> +#include "gt/intel_engine_user.h"
>
> #include "i915_drv.h"
> #include "i915_pmu.h"
> @@ -926,7 +927,7 @@ create_event_attributes(struct drm_i915_private *i915)
> i915_iter =
> add_i915_attr(i915_iter, str,
> __I915_PMU_ENGINE(engine->uabi_class,
> - engine->instance,
> + engine->uabi_instance,
> engine_events[i].sample));
>
> str = kasprintf(GFP_KERNEL, "%s-%s.unit",
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 7b7016171057..70b1ad38e615 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -127,7 +127,7 @@ query_engine_info(struct drm_i915_private *i915,
>
> for_each_engine(engine, i915, id) {
> info.engine.engine_class = engine->uabi_class;
> - info.engine.engine_instance = engine->instance;
> + info.engine.engine_instance = engine->uabi_instance;
> info.capabilities = engine->uabi_capabilities;
>
> if (__copy_to_user(info_ptr, &info, sizeof(info)))
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index da18b8d6b80c..1d11245c4c87 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -677,7 +677,7 @@ TRACE_EVENT(i915_request_queue,
> __entry->dev = rq->i915->drm.primary->index;
> __entry->hw_id = rq->gem_context->hw_id;
> __entry->class = rq->engine->uabi_class;
> - __entry->instance = rq->engine->instance;
> + __entry->instance = rq->engine->uabi_instance;
> __entry->ctx = rq->fence.context;
> __entry->seqno = rq->fence.seqno;
> __entry->flags = flags;
> @@ -706,7 +706,7 @@ DECLARE_EVENT_CLASS(i915_request,
> __entry->dev = rq->i915->drm.primary->index;
> __entry->hw_id = rq->gem_context->hw_id;
> __entry->class = rq->engine->uabi_class;
> - __entry->instance = rq->engine->instance;
> + __entry->instance = rq->engine->uabi_instance;
> __entry->ctx = rq->fence.context;
> __entry->seqno = rq->fence.seqno;
> ),
> @@ -751,7 +751,7 @@ TRACE_EVENT(i915_request_in,
> __entry->dev = rq->i915->drm.primary->index;
> __entry->hw_id = rq->gem_context->hw_id;
> __entry->class = rq->engine->uabi_class;
> - __entry->instance = rq->engine->instance;
> + __entry->instance = rq->engine->uabi_instance;
> __entry->ctx = rq->fence.context;
> __entry->seqno = rq->fence.seqno;
> __entry->prio = rq->sched.attr.priority;
> @@ -782,7 +782,7 @@ TRACE_EVENT(i915_request_out,
> __entry->dev = rq->i915->drm.primary->index;
> __entry->hw_id = rq->gem_context->hw_id;
> __entry->class = rq->engine->uabi_class;
> - __entry->instance = rq->engine->instance;
> + __entry->instance = rq->engine->uabi_instance;
> __entry->ctx = rq->fence.context;
> __entry->seqno = rq->fence.seqno;
> __entry->completed = i915_request_completed(rq);
> @@ -847,7 +847,7 @@ TRACE_EVENT(i915_request_wait_begin,
> __entry->dev = rq->i915->drm.primary->index;
> __entry->hw_id = rq->gem_context->hw_id;
> __entry->class = rq->engine->uabi_class;
> - __entry->instance = rq->engine->instance;
> + __entry->instance = rq->engine->uabi_instance;
> __entry->ctx = rq->fence.context;
> __entry->seqno = rq->fence.seqno;
> __entry->flags = flags;
>
I read it, relatively rushed, since pressure keeps getting applied! :/
There are some good parts and implementation looks okay, but I am not
sure we need a tree. Nodes are bigger than pointers, management code is
bigger, lookup is slower.. is it a win all things considered?
Regards,
Tvrtko
More information about the Intel-gfx
mailing list