Re: [PATCH 20/27] drm/i915: Remove logical HW ID

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 25/09/2019 13:51, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-25 13:41:10)
>> [+ 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?
> 
> The above paragraph explains that as we give two distinct contexts the
> same tag then it will only perform (according to bspec) a lite-restore.

Right I see. But not with virtual engines per se, but even if someone 
created an engine map with two same engines in it. Virtual engines are 
then a super set of this problem.

>>> 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.
> 
> The guc is immaterial here, since they use their own mechanism. It just
> needs to be a value larger than 2 * EXECLIST_MAX_PORTS and less then
> BIT(10).

I don't know how GuC (will) work so hopefully Daniele can confirm or 
deny this won't be a problem there.

>   
>>> @@ -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?
> 
> Aye. OA being a pita.
>   
>> 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.
> 
> Because the values we supply do not matter (they are cookies), just the
> lifetime.

Okay so you are choosing to not use some of the available bits since we 
don't need all 20 (pre gen11), or even 10 (gen11+). This definitely 
deserves a comment. And another assert with NUM_CONTEXT_TAG relating to 
available bits.

> 
>>> +
>>>        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.
> 
> It is singular in the entire system. The mask is a guaranteed unique
> value.

What is singular? The OA context and it's id? I assumed what needs to be 
done is fix hw_id for _all_ contexts while OA is sampling. But on a more 
detailed look this assumption does not seem to hold in code. Okay, I 
declare to not know how OA works well enough.

> 
>>> 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.
> 
> Bspec says that for lrc_desc with matching tags (at least for gen8-gen11
> I believe), it performs a lite restore (regardless of lrca). Ergo it is
> quite a major fix.

Yes I see that now, my bad. This one will not lend itself well to 
backporting.. But I don't think there is a lighter-weight solution. 
Moving hw_id to intel_context would also end up large-ish I think. 
Because of the locking requirement etc.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx