[Intel-gfx] [PATCH 3/3] drm/i915: Store a pointer to intel_context in i915_request

[Tvrtko Ursulin]

On 19/04/2018 12:10, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-04-19 11:40:06)
>>
>> On 19/04/2018 08:17, Chris Wilson wrote:
>>> To ease the frequent and ugly pointer dance of
>>> &request->gem_context->engine[request->engine->id] during request
>>> submission, store that pointer as request->hw_context. One major
>>> advantage that we will exploit later is that this decouples the logical
>>> context state from the engine itself.
>>
>> I can see merit in making all the places which work on engines or
>> requests more logically and elegantly grab the engine specific context.
>>
>> Authors of current unmerged work will probably be not so thrilled though.
> 
>>> @@ -353,60 +346,56 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>>>        struct intel_vgpu_submission *s = &vgpu->submission;
>>>        struct i915_gem_context *shadow_ctx = s->shadow_ctx;
>>>        struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>>> -     int ring_id = workload->ring_id;
>>> -     struct intel_engine_cs *engine = dev_priv->engine[ring_id];
>>> -     struct intel_ring *ring;
>>> +     struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
>>> +     struct intel_context *ce;
>>>        int ret;
>>>    
>>>        lockdep_assert_held(&dev_priv->drm.struct_mutex);
>>>    
>>> -     if (workload->shadowed)
>>> +     if (workload->req)
>>>                return 0;
>>
>> GVT team will need to look at this hunk/function.
> ...
>> At which point I skip over all GVT and continue further down. :)
> 
> That code doesn't merit your attention (or ours really until it is
> improved, it really is staging/ quality).

Still, best to ask GVT team to r-b their parts.

[snip]

>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 0777226e65a6..e6725690b5b6 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -164,7 +164,8 @@
>>>    #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
>>>    
>>>    static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>>> -                                         struct intel_engine_cs *engine);
>>> +                                         struct intel_engine_cs *engine,
>>> +                                         struct intel_context *ce);
>>
>> Feels a bit redundant to pass in everything but OK.
> 
> You do remember which patch this was split out of ;)
> 
>>>    {
>>>        return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
>>> -             i915_gem_context_force_single_submission(ctx));
>>> +             i915_gem_context_force_single_submission(ctx->gem_context));
>>>    }
>>
>> But the whole function change is again not needed I think.
>>
>>>    
>>> -static bool can_merge_ctx(const struct i915_gem_context *prev,
>>> -                       const struct i915_gem_context *next)
>>> +static bool can_merge_ctx(const struct intel_context *prev,
>>> +                       const struct intel_context *next)
>>
>> This one neither.
> 
> Consider what it means. The rule is that we are not allowed to submit
> the same lrc descriptor to subsequent ports; that corresponds to
> the hw context, not gem_context. The payoff, aside from the better
> semantics now, is in subsequent patches.

Yes all fine, just saying it did not have to be in _this_ series but 
could have came later.

>>> +static struct intel_context *
>>> +execlists_context_pin(struct intel_engine_cs *engine,
>>> +                   struct i915_gem_context *ctx)
>>>    {
>>> -     struct intel_context *ce = &ctx->engine[engine->id];
>>> +     struct intel_context *ce = to_intel_context(ctx, engine);
>>>    
>>>        lockdep_assert_held(&ctx->i915->drm.struct_mutex);
>>> -     GEM_BUG_ON(ce->pin_count == 0);
>>> -
>>> -     if (--ce->pin_count)
>>> -             return;
>>>    
>>> -     intel_ring_unpin(ce->ring);
>>> +     if (likely(ce->pin_count++))
>>> +             return ce;
>>> +     GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
>>>    
>>> -     ce->state->obj->pin_global--;
>>> -     i915_gem_object_unpin_map(ce->state->obj);
>>> -     i915_vma_unpin(ce->state);
>>> +     ce->ops = &execlists_context_ops;
>>
>> If ops are set on pin it would be good to unset them on destroy.
> 
> No point on destroy, since it's being destroyed.
> 
>> Hm, or even set them not on pin but on creation.
> 
> engine->context_pin() is our intel_context factory, pin is the creation
> function.
> 
> Definitely a bit asymmetrical, the key bit was having ce->unpin() so that
> we didn't try to release the intel_context via a stale engine reference.
> 
> What I have been considering is passing intel_context to
> i915_request_alloc() instead and trying to avoid that asymmetry by
> managing the creation in the caller and separating it from pin().
> 
>> Not a huge deal but it is a
>> bit unbalanced, and also if ce->ops is also a guard to distinguish
>> possible from impossible engines then it is a bit more strange.
> 
> Not impossible engines in this regard, unused contexts.

Both then, since impossible engines will have unused contexts. :)

>>> @@ -1323,10 +1345,6 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
>>>    
>>>        intel_engine_setup_common(engine);
>>>    
>>> -     err = intel_engine_init_common(engine);
>>> -     if (err)
>>> -             goto err;
>>> -
>>>        ring = intel_engine_create_ring(engine, 32 * PAGE_SIZE);
>>>        if (IS_ERR(ring)) {
>>>                err = PTR_ERR(ring);
>>> @@ -1341,8 +1359,14 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
>>>        GEM_BUG_ON(engine->buffer);
>>>        engine->buffer = ring;
>>>    
>>> +     err = intel_engine_init_common(engine);
>>> +     if (err)
>>> +             goto err_unpin;
>>> +
>>
>> This ordering change doesn't look like it has to be done in this patch.
> 
> It does. Do you want to take another look ;)
> 
> The problem here is the legacy ringbuffer submission uses a common
> engine->buffer, and the kernel contexts are pinned in init_common; so we
> have to create that buffer first.

So it looks like an pre-existing bug. intel_ring_context_pin returns 
engine->buffer which hasn't been created yet. Oh well.. can you inject a 
patch which solves only that before this one?

Regards,

Tvrtko