[Intel-gfx] [PATCH 20/27] drm/i915: Remove logical HW ID
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Sep 25 17:59:32 UTC 2019
+ Matt
On 9/25/19 5:41 AM, Tvrtko Ursulin wrote:
> [+ Daniele, I think he might want to have a look at this.]
>
> On 25/09/2019 11:01, Chris Wilson wrote:
>> With the introduction of ctx->engines[] we allow multiple logical
>> contexts to be used on the same engine (e.g. with virtual engines). Each
>> logical context requires a unique tag in order for context-switching to
>> occur correctly between them.
>>
>> We only need to keep a unique tag for the active lifetime of the
>> context, and for as long as we need to identify that context. The HW
>> uses the tag to determine if it should use a lite-restore (why not the
>> LRCA?) and passes the tag back for various status identifies. The only
>> status we need to track is for OA, so when using perf, we assign the
>> specific context a unique tag.
>>
>> Fixes: 976b55f0e1db ("drm/i915: Allow a context to define its set of
>> engines")
>
> Fixes? Why?
>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 147 ------------------
>> drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 --
>> .../gpu/drm/i915/gem/i915_gem_context_types.h | 18 ---
>> .../drm/i915/gem/selftests/i915_gem_context.c | 13 +-
>> .../gpu/drm/i915/gem/selftests/mock_context.c | 8 -
>> drivers/gpu/drm/i915/gt/intel_context_types.h | 1 +
>> drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 +-
>> drivers/gpu/drm/i915/gt/intel_lrc.c | 29 ++--
>> drivers/gpu/drm/i915/gvt/kvmgt.c | 17 --
>> drivers/gpu/drm/i915/i915_debugfs.c | 3 -
>> drivers/gpu/drm/i915/i915_gpu_error.c | 7 +-
>> drivers/gpu/drm/i915/i915_gpu_error.h | 1 -
>> drivers/gpu/drm/i915/i915_perf.c | 25 ++-
>> drivers/gpu/drm/i915/i915_trace.h | 38 ++---
>> .../gpu/drm/i915/selftests/i915_gem_evict.c | 4 +-
>> drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +-
>> 16 files changed, 48 insertions(+), 284 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> index e832238a72e5..4e59b809d901 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> @@ -168,97 +168,6 @@ lookup_user_engine(struct i915_gem_context *ctx,
>> return i915_gem_context_get_engine(ctx, idx);
>> }
>> -static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
>> -{
>> - unsigned int max;
>> -
>> - lockdep_assert_held(&i915->contexts.mutex);
>> -
>> - if (INTEL_GEN(i915) >= 12)
>> - max = GEN12_MAX_CONTEXT_HW_ID;
>> - else if (INTEL_GEN(i915) >= 11)
>> - max = GEN11_MAX_CONTEXT_HW_ID;
>> - else if (USES_GUC_SUBMISSION(i915))
>> - /*
>> - * When using GuC in proxy submission, GuC consumes the
>> - * highest bit in the context id to indicate proxy submission.
>> - */
>> - max = MAX_GUC_CONTEXT_HW_ID;
>> - else
>> - max = MAX_CONTEXT_HW_ID;
>> -
>> - return ida_simple_get(&i915->contexts.hw_ida, 0, max, gfp);
>> -}
>> -
>> -static int steal_hw_id(struct drm_i915_private *i915)
>> -{
>> - struct i915_gem_context *ctx, *cn;
>> - LIST_HEAD(pinned);
>> - int id = -ENOSPC;
>> -
>> - lockdep_assert_held(&i915->contexts.mutex);
>> -
>> - list_for_each_entry_safe(ctx, cn,
>> - &i915->contexts.hw_id_list, hw_id_link) {
>> - if (atomic_read(&ctx->hw_id_pin_count)) {
>> - list_move_tail(&ctx->hw_id_link, &pinned);
>> - continue;
>> - }
>> -
>> - GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
>> - list_del_init(&ctx->hw_id_link);
>> - id = ctx->hw_id;
>> - break;
>> - }
>> -
>> - /*
>> - * Remember how far we got up on the last repossesion scan, so the
>> - * list is kept in a "least recently scanned" order.
>> - */
>> - list_splice_tail(&pinned, &i915->contexts.hw_id_list);
>> - return id;
>> -}
>> -
>> -static int assign_hw_id(struct drm_i915_private *i915, unsigned int
>> *out)
>> -{
>> - int ret;
>> -
>> - lockdep_assert_held(&i915->contexts.mutex);
>> -
>> - /*
>> - * We prefer to steal/stall ourselves and our users over that of the
>> - * entire system. That may be a little unfair to our users, and
>> - * even hurt high priority clients. The choice is whether to oomkill
>> - * something else, or steal a context id.
>> - */
>> - ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL |
>> __GFP_NOWARN);
>> - if (unlikely(ret < 0)) {
>> - ret = steal_hw_id(i915);
>> - if (ret < 0) /* once again for the correct errno code */
>> - ret = new_hw_id(i915, GFP_KERNEL);
>> - if (ret < 0)
>> - return ret;
>> - }
>> -
>> - *out = ret;
>> - return 0;
>> -}
>> -
>> -static void release_hw_id(struct i915_gem_context *ctx)
>> -{
>> - struct drm_i915_private *i915 = ctx->i915;
>> -
>> - if (list_empty(&ctx->hw_id_link))
>> - return;
>> -
>> - mutex_lock(&i915->contexts.mutex);
>> - if (!list_empty(&ctx->hw_id_link)) {
>> - ida_simple_remove(&i915->contexts.hw_ida, ctx->hw_id);
>> - list_del_init(&ctx->hw_id_link);
>> - }
>> - mutex_unlock(&i915->contexts.mutex);
>> -}
>> -
>> static void __free_engines(struct i915_gem_engines *e, unsigned int
>> count)
>> {
>> while (count--) {
>> @@ -313,8 +222,6 @@ static void i915_gem_context_free(struct
>> i915_gem_context *ctx)
>> lockdep_assert_held(&ctx->i915->drm.struct_mutex);
>> GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>> - release_hw_id(ctx);
>> -
>> free_engines(rcu_access_pointer(ctx->engines));
>> mutex_destroy(&ctx->engines_mutex);
>> @@ -459,12 +366,6 @@ static void context_close(struct i915_gem_context
>> *ctx)
>> ctx->file_priv = ERR_PTR(-EBADF);
>> - /*
>> - * This context will never again be assinged to HW, so we can
>> - * reuse its ID for the next context.
>> - */
>> - release_hw_id(ctx);
>> -
>> /*
>> * The LUT uses the VMA as a backpointer to unref the object,
>> * so we need to clear the LUT before we close all the VMA (inside
>> @@ -507,7 +408,6 @@ __create_context(struct drm_i915_private *i915)
>> RCU_INIT_POINTER(ctx->engines, e);
>> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
>> - INIT_LIST_HEAD(&ctx->hw_id_link);
>> /* NB: Mark all slices as needing a remap so that when the
>> context first
>> * loads it will restore whatever remap state already exists. If
>> there
>> @@ -676,18 +576,11 @@ struct i915_gem_context *
>> i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
>> {
>> struct i915_gem_context *ctx;
>> - int err;
>> ctx = i915_gem_create_context(i915, 0);
>> if (IS_ERR(ctx))
>> return ctx;
>> - err = i915_gem_context_pin_hw_id(ctx);
>> - if (err) {
>> - destroy_kernel_context(&ctx);
>> - return ERR_PTR(err);
>> - }
>> -
>> i915_gem_context_clear_bannable(ctx);
>> i915_gem_context_set_persistence(ctx);
>> ctx->sched.priority = I915_USER_PRIORITY(prio);
>> @@ -727,15 +620,6 @@ int i915_gem_contexts_init(struct
>> drm_i915_private *dev_priv)
>> DRM_ERROR("Failed to create default global context\n");
>> return PTR_ERR(ctx);
>> }
>> - /*
>> - * For easy recognisablity, we want the kernel context to be 0
>> and then
>> - * all user contexts will have non-zero hw_id. Kernel contexts are
>> - * permanently pinned, so that we never suffer a stall and can
>> - * use them from any allocation context (e.g. for evicting other
>> - * contexts and from inside the shrinker).
>> - */
>> - GEM_BUG_ON(ctx->hw_id);
>> - GEM_BUG_ON(!atomic_read(&ctx->hw_id_pin_count));
>> dev_priv->kernel_context = ctx;
>> DRM_DEBUG_DRIVER("%s context support initialized\n",
>> @@ -749,10 +633,6 @@ void i915_gem_contexts_fini(struct
>> drm_i915_private *i915)
>> lockdep_assert_held(&i915->drm.struct_mutex);
>> destroy_kernel_context(&i915->kernel_context);
>> -
>> - /* Must free all deferred contexts (via flush_workqueue) first */
>> - GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
>> - ida_destroy(&i915->contexts.hw_ida);
>> }
>> static int context_idr_cleanup(int id, void *p, void *data)
>> @@ -2438,33 +2318,6 @@ int i915_gem_context_reset_stats_ioctl(struct
>> drm_device *dev,
>> return ret;
>> }
>> -int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
>> -{
>> - struct drm_i915_private *i915 = ctx->i915;
>> - int err = 0;
>> -
>> - mutex_lock(&i915->contexts.mutex);
>> -
>> - GEM_BUG_ON(i915_gem_context_is_closed(ctx));
>> -
>> - if (list_empty(&ctx->hw_id_link)) {
>> - GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count));
>> -
>> - err = assign_hw_id(i915, &ctx->hw_id);
>> - if (err)
>> - goto out_unlock;
>> -
>> - list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
>> - }
>> -
>> - GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == ~0u);
>> - atomic_inc(&ctx->hw_id_pin_count);
>> -
>> -out_unlock:
>> - mutex_unlock(&i915->contexts.mutex);
>> - return err;
>> -}
>> -
>> /* GEM context-engines iterator: for_each_gem_engine() */
>> struct intel_context *
>> i915_gem_engines_iter_next(struct i915_gem_engines_iter *it)
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> b/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> index e0f5b6c6a331..1b0df53436cf 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> @@ -127,21 +127,6 @@ i915_gem_context_clear_user_engines(struct
>> i915_gem_context *ctx)
>> clear_bit(CONTEXT_USER_ENGINES, &ctx->flags);
>> }
>> -int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
>> -static inline int i915_gem_context_pin_hw_id(struct i915_gem_context
>> *ctx)
>> -{
>> - if (atomic_inc_not_zero(&ctx->hw_id_pin_count))
>> - return 0;
>> -
>> - return __i915_gem_context_pin_hw_id(ctx);
>> -}
>> -
>> -static inline void i915_gem_context_unpin_hw_id(struct
>> i915_gem_context *ctx)
>> -{
>> - GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == 0u);
>> - atomic_dec(&ctx->hw_id_pin_count);
>> -}
>> -
>> static inline bool i915_gem_context_is_kernel(struct
>> i915_gem_context *ctx)
>> {
>> return !ctx->file_priv;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> index daf1ea5075a6..6419da7c9f90 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> @@ -148,24 +148,6 @@ struct i915_gem_context {
>> #define CONTEXT_FORCE_SINGLE_SUBMISSION 2
>> #define CONTEXT_USER_ENGINES 3
>> - /**
>> - * @hw_id: - unique identifier for the context
>> - *
>> - * The hardware needs to uniquely identify the context for a few
>> - * functions like fault reporting, PASID, scheduling. The
>> - * &drm_i915_private.context_hw_ida is used to assign a unqiue
>> - * id for the lifetime of the context.
>> - *
>> - * @hw_id_pin_count: - number of times this context had been pinned
>> - * for use (should be, at most, once per engine).
>> - *
>> - * @hw_id_link: - all contexts with an assigned id are tracked
>> - * for possible repossession.
>> - */
>> - unsigned int hw_id;
>> - atomic_t hw_id_pin_count;
>> - struct list_head hw_id_link;
>> -
>> struct mutex mutex;
>> struct i915_sched_attr sched;
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> index b39224ed3817..a8a9293f4ac6 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
>> @@ -446,9 +446,9 @@ static int igt_ctx_exec(void *arg)
>> err = gpu_fill(ce, obj, dw);
>> if (err) {
>> - pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) in ctx %u [full-ppgtt? %s], err=%d\n",
>> + pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) [full-ppgtt? %s], err=%d\n",
>> ndwords, dw, max_dwords(obj),
>> - engine->name, ctx->hw_id,
>> + engine->name,
>> yesno(!!ctx->vm), err);
>> intel_context_put(ce);
>> kernel_context_close(ctx);
>> @@ -584,9 +584,9 @@ static int igt_shared_ctx_exec(void *arg)
>> err = gpu_fill(ce, obj, dw);
>> if (err) {
>> - pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) in ctx %u [full-ppgtt? %s], err=%d\n",
>> + pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) [full-ppgtt? %s], err=%d\n",
>> ndwords, dw, max_dwords(obj),
>> - engine->name, ctx->hw_id,
>> + engine->name,
>> yesno(!!ctx->vm), err);
>> intel_context_put(ce);
>> kernel_context_close(ctx);
>> @@ -1168,10 +1168,9 @@ static int igt_ctx_readonly(void *arg)
>> err = gpu_fill(ce, obj, dw);
>> if (err) {
>> - pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) in ctx %u [full-ppgtt? %s], err=%d\n",
>> + pr_err("Failed to fill dword %lu [%lu/%lu] with gpu
>> (%s) [full-ppgtt? %s], err=%d\n",
>> ndwords, dw, max_dwords(obj),
>> - ce->engine->name, ctx->hw_id,
>> - yesno(!!ctx->vm), err);
>> + ce->engine->name, yesno(!!ctx->vm), err);
>> i915_gem_context_unlock_engines(ctx);
>> goto out_unlock;
>> }
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
>> b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
>> index 0d1a275ec316..ebc46f098561 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
>> @@ -13,7 +13,6 @@ mock_context(struct drm_i915_private *i915,
>> {
>> struct i915_gem_context *ctx;
>> struct i915_gem_engines *e;
>> - int ret;
>> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> if (!ctx)
>> @@ -32,13 +31,8 @@ mock_context(struct drm_i915_private *i915,
>> RCU_INIT_POINTER(ctx->engines, e);
>> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
>> - INIT_LIST_HEAD(&ctx->hw_id_link);
>> mutex_init(&ctx->mutex);
>> - ret = i915_gem_context_pin_hw_id(ctx);
>> - if (ret < 0)
>> - goto err_engines;
>> -
>> if (name) {
>> struct i915_ppgtt *ppgtt;
>> @@ -56,8 +50,6 @@ mock_context(struct drm_i915_private *i915,
>> return ctx;
>> -err_engines:
>> - free_engines(rcu_access_pointer(ctx->engines));
>> err_free:
>> kfree(ctx);
>> return NULL;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
>> b/drivers/gpu/drm/i915/gt/intel_context_types.h
>> index bf9cedfccbf0..6959b05ae5f8 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>> @@ -58,6 +58,7 @@ struct intel_context {
>> u32 *lrc_reg_state;
>> u64 lrc_desc;
>> + u32 tag; /* cookie passed to HW to track this context on
>> submission */
>> unsigned int active_count; /* protected by timeline->mutex */
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> index da4b45aa84b1..7bce176e696c 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> @@ -296,10 +296,12 @@ struct intel_engine_cs {
>> u8 uabi_class;
>> u8 uabi_instance;
>> + u32 uabi_capabilities;
>> u32 context_size;
>> u32 mmio_base;
>> - u32 uabi_capabilities;
>> + unsigned int context_tag;
>> +#define NUM_CONTEXT_TAG (256)
>
> AFAICT this define now defines how deep ELSP we can have before we start
> getting hard to debug problems. GuC angle needs considering here.
>
> Can we add some asserts somewhere to express the relationship?
>
> And a comment here explaining this, or with the assert.
>
>> struct rb_node uabi_node;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> index 0ea2f4d7dc43..af79b8d70800 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> @@ -433,12 +433,8 @@ assert_priority_queue(const struct i915_request
>> *prev,
>> static u64
>> lrc_descriptor(struct intel_context *ce, struct intel_engine_cs
>> *engine)
>> {
>> - struct i915_gem_context *ctx = ce->gem_context;
>> u64 desc;
>> - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (BIT(GEN8_CTX_ID_WIDTH)));
>> - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID >
>> (BIT(GEN11_SW_CTX_ID_WIDTH)));
>> -
>> desc = INTEL_LEGACY_32B_CONTEXT;
>> if (i915_vm_is_4lvl(ce->vm))
>> desc = INTEL_LEGACY_64B_CONTEXT;
>> @@ -456,20 +452,11 @@ lrc_descriptor(struct intel_context *ce, struct
>> intel_engine_cs *engine)
>> * anything below.
>> */
>> if (INTEL_GEN(engine->i915) >= 11) {
>> - GEM_BUG_ON(ctx->hw_id >= BIT(GEN11_SW_CTX_ID_WIDTH));
>> - desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
>> - /* bits 37-47 */
>> -
>> desc |= (u64)engine->instance << GEN11_ENGINE_INSTANCE_SHIFT;
>> /* bits 48-53 */
>> - /* TODO: decide what to do with SW counter (bits 55-60) */
>> -
>> desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
>> /* bits 61-63 */
>> - } else {
>> - GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH));
>> - desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits
>> 32-52 */
>> }
>> return desc;
>> @@ -978,6 +965,15 @@ __execlists_schedule_in(struct i915_request *rq)
>> intel_context_get(ce);
>> + if (ce->tag) {
>> + ce->lrc_desc |= (u64)ce->tag << 32;
>> + } else {
>> + ce->lrc_desc &= ~GENMASK_ULL(47, 37);
>> + ce->lrc_desc |=
>> + (u64)(engine->context_tag++ % NUM_CONTEXT_TAG) <<
>> + GEN11_SW_CTX_ID_SHIFT;
>> + }
>
> So hw id is valid only while context is in ELSP and it changes with
> every submission except in the OA case?
>
> Shifts and masks look dodgy. For >= gen11 the current code has the hw_id
> in [37, 42], otherwise [32, 52]. This patch does not seem to handle the
> differences between gens.
>
>> +
>> intel_gt_pm_get(engine->gt);
>> execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
>> intel_engine_context_in(engine);
>> @@ -2224,7 +2220,6 @@ static void execlists_context_unpin(struct
>> intel_context *ce)
>> check_redzone((void *)ce->lrc_reg_state - LRC_STATE_PN * PAGE_SIZE,
>> ce->engine);
>> - i915_gem_context_unpin_hw_id(ce->gem_context);
>> i915_gem_object_unpin_map(ce->state->obj);
>> intel_ring_reset(ce->ring, ce->ring->tail);
>> }
>> @@ -2274,18 +2269,12 @@ __execlists_context_pin(struct intel_context *ce,
>> goto unpin_active;
>> }
>> - ret = i915_gem_context_pin_hw_id(ce->gem_context);
>> - if (ret)
>> - goto unpin_map;
>> -
>> ce->lrc_desc = lrc_descriptor(ce, engine);
>> ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>> __execlists_update_reg_state(ce, engine);
>> return 0;
>> -unpin_map:
>> - i915_gem_object_unpin_map(ce->state->obj);
>> unpin_active:
>> intel_context_active_release(ce);
>> err:
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 343d79c1cb7e..04a5a0d90823 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -1564,27 +1564,10 @@ vgpu_id_show(struct device *dev, struct
>> device_attribute *attr,
>> return sprintf(buf, "\n");
>> }
>> -static ssize_t
>> -hw_id_show(struct device *dev, struct device_attribute *attr,
>> - char *buf)
>> -{
>> - struct mdev_device *mdev = mdev_from_dev(dev);
>> -
>> - if (mdev) {
>> - struct intel_vgpu *vgpu = (struct intel_vgpu *)
>> - mdev_get_drvdata(mdev);
>> - return sprintf(buf, "%u\n",
>> - vgpu->submission.shadow[0]->gem_context->hw_id);
>> - }
>> - return sprintf(buf, "\n");
>> -}
>> -
>> static DEVICE_ATTR_RO(vgpu_id);
>> -static DEVICE_ATTR_RO(hw_id);
>> static struct attribute *intel_vgpu_attrs[] = {
>> &dev_attr_vgpu_id.attr,
>> - &dev_attr_hw_id.attr,
>> NULL
>> };
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index fd41505d52ec..8caaa446490f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1504,9 +1504,6 @@ static int i915_context_status(struct seq_file
>> *m, void *unused)
>> struct intel_context *ce;
>> seq_puts(m, "HW context ");
>> - if (!list_empty(&ctx->hw_id_link))
>> - seq_printf(m, "%x [pin %u]", ctx->hw_id,
>> - atomic_read(&ctx->hw_id_pin_count));
>> if (ctx->pid) {
>> struct task_struct *task;
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
>> b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index a86d28bda6dd..47239df653f2 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -471,9 +471,9 @@ static void error_print_context(struct
>> drm_i915_error_state_buf *m,
>> const char *header,
>> const struct drm_i915_error_context *ctx)
>> {
>> - err_printf(m, "%s%s[%d] hw_id %d, prio %d, guilty %d active %d\n",
>> - header, ctx->comm, ctx->pid, ctx->hw_id,
>> - ctx->sched_attr.priority, ctx->guilty, ctx->active);
>> + err_printf(m, "%s%s[%d] prio %d, guilty %d active %d\n",
>> + header, ctx->comm, ctx->pid, ctx->sched_attr.priority,
>> + ctx->guilty, ctx->active);
>> }
>> static void error_print_engine(struct drm_i915_error_state_buf *m,
>> @@ -1262,7 +1262,6 @@ static bool record_context(struct
>> drm_i915_error_context *e,
>> rcu_read_unlock();
>> }
>> - e->hw_id = ctx->hw_id;
>> e->sched_attr = ctx->sched;
>> e->guilty = atomic_read(&ctx->guilty_count);
>> e->active = atomic_read(&ctx->active_count);
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h
>> b/drivers/gpu/drm/i915/i915_gpu_error.h
>> index caaed5093d95..4dc36d6ee3a2 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.h
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.h
>> @@ -117,7 +117,6 @@ struct i915_gpu_state {
>> struct drm_i915_error_context {
>> char comm[TASK_COMM_LEN];
>> pid_t pid;
>> - u32 hw_id;
>> int active;
>> int guilty;
>> struct i915_sched_attr sched_attr;
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c
>> b/drivers/gpu/drm/i915/i915_perf.c
>> index 80055501eccb..d36ba248d438 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1283,22 +1283,15 @@ static int oa_get_render_ctx_id(struct
>> i915_perf_stream *stream)
>> } else {
>> stream->specific_ctx_id_mask =
>> (1U << GEN8_CTX_ID_WIDTH) - 1;
>> - stream->specific_ctx_id =
>> - upper_32_bits(ce->lrc_desc);
>> - stream->specific_ctx_id &=
>> - stream->specific_ctx_id_mask;
>> + stream->specific_ctx_id = stream->specific_ctx_id_mask;
>> }
>> break;
>> case 11:
>> case 12: {
>> stream->specific_ctx_id_mask =
>> - ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) <<
>> (GEN11_SW_CTX_ID_SHIFT - 32) |
>> - ((1U << GEN11_ENGINE_INSTANCE_WIDTH) - 1) <<
>> (GEN11_ENGINE_INSTANCE_SHIFT - 32) |
>> - ((1 << GEN11_ENGINE_CLASS_WIDTH) - 1) <<
>> (GEN11_ENGINE_CLASS_SHIFT - 32);
>> - stream->specific_ctx_id = upper_32_bits(ce->lrc_desc);
>> - stream->specific_ctx_id &=
>> - stream->specific_ctx_id_mask;
>> + ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) <<
>> (GEN11_SW_CTX_ID_SHIFT - 32);
>> + stream->specific_ctx_id = stream->specific_ctx_id_mask;
>> break;
>> }
>> @@ -1306,6 +1299,8 @@ static int oa_get_render_ctx_id(struct
>> i915_perf_stream *stream)
>> MISSING_CASE(INTEL_GEN(i915));
>> }
>> + ce->tag = stream->specific_ctx_id_mask;
>> +
>> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n",
>> stream->specific_ctx_id,
>> stream->specific_ctx_id_mask);
>> @@ -1324,12 +1319,14 @@ static void oa_put_render_ctx_id(struct
>> i915_perf_stream *stream)
>> {
>> struct intel_context *ce;
>> - stream->specific_ctx_id = INVALID_CTX_ID;
>> - stream->specific_ctx_id_mask = 0;
>> -
>> ce = fetch_and_zero(&stream->pinned_ctx);
>> - if (ce)
>> + if (ce) {
>> + ce->tag = 0;
>> intel_context_unpin(ce);
>> + }
>> +
>> + stream->specific_ctx_id = INVALID_CTX_ID;
>> + stream->specific_ctx_id_mask = 0;
>
> OA hunks looks dodgy as well. ce->tag is set to
> stream->specific_ctx_id_mask while I think it should be id. Furthermore
> id is assigned from mask which is fixed and not unique for different
> contexts.
>
>> }
>> static void
>> diff --git a/drivers/gpu/drm/i915/i915_trace.h
>> b/drivers/gpu/drm/i915/i915_trace.h
>> index 24f2944da09d..1f2cf6cfafb5 100644
>> --- a/drivers/gpu/drm/i915/i915_trace.h
>> +++ b/drivers/gpu/drm/i915/i915_trace.h
>> @@ -665,7 +665,6 @@ TRACE_EVENT(i915_request_queue,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> - __field(u32, hw_id)
>> __field(u64, ctx)
>> __field(u16, class)
>> __field(u16, instance)
>> @@ -675,7 +674,6 @@ TRACE_EVENT(i915_request_queue,
>> TP_fast_assign(
>> __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->uabi_instance;
>> __entry->ctx = rq->fence.context;
>> @@ -683,10 +681,9 @@ TRACE_EVENT(i915_request_queue,
>> __entry->flags = flags;
>> ),
>> - TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu,
>> seqno=%u, flags=0x%x",
>> + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u,
>> flags=0x%x",
>> __entry->dev, __entry->class, __entry->instance,
>> - __entry->hw_id, __entry->ctx, __entry->seqno,
>> - __entry->flags)
>> + __entry->ctx, __entry->seqno, __entry->flags)
>> );
>> DECLARE_EVENT_CLASS(i915_request,
>> @@ -695,7 +692,6 @@ DECLARE_EVENT_CLASS(i915_request,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> - __field(u32, hw_id)
>> __field(u64, ctx)
>> __field(u16, class)
>> __field(u16, instance)
>> @@ -704,16 +700,15 @@ DECLARE_EVENT_CLASS(i915_request,
>> TP_fast_assign(
>> __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->uabi_instance;
>> __entry->ctx = rq->fence.context;
>> __entry->seqno = rq->fence.seqno;
>> ),
>> - TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu, seqno=%u",
>> + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u",
>> __entry->dev, __entry->class, __entry->instance,
>> - __entry->hw_id, __entry->ctx, __entry->seqno)
>> + __entry->ctx, __entry->seqno)
>> );
>> DEFINE_EVENT(i915_request, i915_request_add,
>> @@ -738,7 +733,6 @@ TRACE_EVENT(i915_request_in,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> - __field(u32, hw_id)
>> __field(u64, ctx)
>> __field(u16, class)
>> __field(u16, instance)
>> @@ -749,7 +743,6 @@ TRACE_EVENT(i915_request_in,
>> TP_fast_assign(
>> __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->uabi_instance;
>> __entry->ctx = rq->fence.context;
>> @@ -758,9 +751,9 @@ TRACE_EVENT(i915_request_in,
>> __entry->port = port;
>> ),
>> - TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu,
>> seqno=%u, prio=%u, port=%u",
>> + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u, prio=%u,
>> port=%u",
>> __entry->dev, __entry->class, __entry->instance,
>> - __entry->hw_id, __entry->ctx, __entry->seqno,
>> + __entry->ctx, __entry->seqno,
>> __entry->prio, __entry->port)
>> );
>> @@ -770,7 +763,6 @@ TRACE_EVENT(i915_request_out,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> - __field(u32, hw_id)
>> __field(u64, ctx)
>> __field(u16, class)
>> __field(u16, instance)
>> @@ -780,7 +772,6 @@ TRACE_EVENT(i915_request_out,
>> TP_fast_assign(
>> __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->uabi_instance;
>> __entry->ctx = rq->fence.context;
>> @@ -788,10 +779,9 @@ TRACE_EVENT(i915_request_out,
>> __entry->completed = i915_request_completed(rq);
>> ),
>> - TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu,
>> seqno=%u, completed?=%u",
>> + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u,
>> completed?=%u",
>> __entry->dev, __entry->class, __entry->instance,
>> - __entry->hw_id, __entry->ctx, __entry->seqno,
>> - __entry->completed)
>> + __entry->ctx, __entry->seqno, __entry->completed)
>> );
>> #else
>> @@ -829,7 +819,6 @@ TRACE_EVENT(i915_request_wait_begin,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> - __field(u32, hw_id)
>> __field(u64, ctx)
>> __field(u16, class)
>> __field(u16, instance)
>> @@ -845,7 +834,6 @@ TRACE_EVENT(i915_request_wait_begin,
>> */
>> TP_fast_assign(
>> __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->uabi_instance;
>> __entry->ctx = rq->fence.context;
>> @@ -853,9 +841,9 @@ TRACE_EVENT(i915_request_wait_begin,
>> __entry->flags = flags;
>> ),
>> - TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu,
>> seqno=%u, flags=0x%x",
>> + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u,
>> flags=0x%x",
>> __entry->dev, __entry->class, __entry->instance,
>> - __entry->hw_id, __entry->ctx, __entry->seqno,
>> + __entry->ctx, __entry->seqno,
>> __entry->flags)
>> );
>> @@ -958,19 +946,17 @@ DECLARE_EVENT_CLASS(i915_context,
>> TP_STRUCT__entry(
>> __field(u32, dev)
>> __field(struct i915_gem_context *, ctx)
>> - __field(u32, hw_id)
>> __field(struct i915_address_space *, vm)
>> ),
>> TP_fast_assign(
>> __entry->dev = ctx->i915->drm.primary->index;
>> __entry->ctx = ctx;
>> - __entry->hw_id = ctx->hw_id;
>> __entry->vm = ctx->vm;
>> ),
>> - TP_printk("dev=%u, ctx=%p, ctx_vm=%p, hw_id=%u",
>> - __entry->dev, __entry->ctx, __entry->vm, __entry->hw_id)
>> + TP_printk("dev=%u, ctx=%p, ctx_vm=%p",
>> + __entry->dev, __entry->ctx, __entry->vm)
>> )
>> DEFINE_EVENT(i915_context, i915_context_create,
>> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
>> b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
>> index 52d2df843148..f39f0282e78c 100644
>> --- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
>> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
>> @@ -491,8 +491,8 @@ static int igt_evict_contexts(void *arg)
>> if (IS_ERR(rq)) {
>> /* When full, fail_if_busy will trigger EBUSY */
>> if (PTR_ERR(rq) != -EBUSY) {
>> - pr_err("Unexpected error from request alloc (ctx
>> hw id %u, on %s): %d\n",
>> - ctx->hw_id, engine->name,
>> + pr_err("Unexpected error from request alloc (on
>> %s): %d\n",
>> + engine->name,
>> (int)PTR_ERR(rq));
>> err = PTR_ERR(rq);
>> }
>> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c
>> b/drivers/gpu/drm/i915/selftests/i915_vma.c
>> index 1c9db08f7c28..ac1ff558eb90 100644
>> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
>> @@ -170,7 +170,7 @@ static int igt_vma_create(void *arg)
>> }
>> nc = 0;
>> - for_each_prime_number(num_ctx, MAX_CONTEXT_HW_ID) {
>> + for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
>> for (; nc < num_ctx; nc++) {
>> ctx = mock_context(i915, "mock");
>> if (!ctx)
>>
>
> Unless I am missing something I see this patch as simplification and not
> a fix. If we can get away with it in the GuC world then it's quite a big
> simplification so it's fine by me.
The GuC will require a fixed ID per intel_context for the whole time the
context is in use/pinned. The GUC IDs are a global list (i.e. all
engines share it) with 64k entries, so the logic to assign them is going
to be a bit different and most likely confined to the guc
context_pin/unpin callbacks, but we might bring back some of the
low-level ID management logic that has been removed in this patch.
An ID at the gem_context level definitely doesn't help on the guc side, so:
Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
Daniele
>
> Regards,
>
> Tvrtko
More information about the Intel-gfx
mailing list