Re: [PATCH 03/23] drm/i915: Remove lrc default desc from GEM context

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

On 01/08/2019 09:41, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-08-01 09:37:38)
>>
>> On 24/07/2019 10:20, Tvrtko Ursulin wrote:
>>>
>>> On 23/07/2019 19:38, Chris Wilson wrote:
>>>> We only compute the lrc_descriptor() on pinning the context, i.e.
>>>> infrequently, so we do not benefit from storing the template as the
>>>> addressing mode is also fixed for the lifetime of the intel_context.
>>>>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/i915_gem_context.c   | 28 ++-----------------
>>>>    .../gpu/drm/i915/gem/i915_gem_context_types.h |  2 --
>>>>    drivers/gpu/drm/i915/gt/intel_lrc.c           | 12 +++++---
>>>>    drivers/gpu/drm/i915/gvt/scheduler.c          |  3 --
>>>>    4 files changed, 10 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> index b28c7ca681a8..1b3dc7258ef2 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> @@ -397,30 +397,6 @@ static void context_close(struct i915_gem_context
>>>> *ctx)
>>>>        i915_gem_context_put(ctx);
>>>>    }
>>>> -static u32 default_desc_template(const struct drm_i915_private *i915,
>>>> -                 const struct i915_address_space *vm)
>>>> -{
>>>> -    u32 address_mode;
>>>> -    u32 desc;
>>>> -
>>>> -    desc = GEN8_CTX_VALID | GEN8_CTX_PRIVILEGE;
>>>> -
>>>> -    address_mode = INTEL_LEGACY_32B_CONTEXT;
>>>> -    if (vm && i915_vm_is_4lvl(vm))
>>>> -        address_mode = INTEL_LEGACY_64B_CONTEXT;
>>>> -    desc |= address_mode << GEN8_CTX_ADDRESSING_MODE_SHIFT;
>>>> -
>>>> -    if (IS_GEN(i915, 8))
>>>> -        desc |= GEN8_CTX_L3LLC_COHERENT;
>>>> -
>>>> -    /* TODO: WaDisableLiteRestore when we start using semaphore
>>>> -     * signalling between Command Streamers
>>>> -     * ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE;
>>>> -     */
>>>> -
>>>> -    return desc;
>>>> -}
>>>> -
>>>>    static struct i915_gem_context *
>>>>    __create_context(struct drm_i915_private *i915)
>>>>    {
>>>> @@ -459,7 +435,6 @@ __create_context(struct drm_i915_private *i915)
>>>>        i915_gem_context_set_recoverable(ctx);
>>>>        ctx->ring_size = 4 * PAGE_SIZE;
>>>> -    ctx->desc_template = default_desc_template(i915, NULL);
>>>>        for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
>>>>            ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>>>> @@ -478,8 +453,9 @@ __set_ppgtt(struct i915_gem_context *ctx, struct
>>>> i915_address_space *vm)
>>>>        struct i915_gem_engines_iter it;
>>>>        struct intel_context *ce;
>>>> +    GEM_BUG_ON(old && i915_vm_is_4lvl(vm) != i915_vm_is_4lvl(old));
>>>> +
>>>>        ctx->vm = i915_vm_get(vm);
>>>> -    ctx->desc_template = default_desc_template(ctx->i915, vm);
>>>>        for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>>>>            i915_vm_put(ce->vm);
>>>> 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 0ee61482ef94..a02d98494078 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>>>> @@ -171,8 +171,6 @@ struct i915_gem_context {
>>>>        /** ring_size: size for allocating the per-engine ring buffer */
>>>>        u32 ring_size;
>>>> -    /** desc_template: invariant fields for the HW context descriptor */
>>>> -    u32 desc_template;
>>>>        /** guilty_count: How many times this context has caused a GPU
>>>> hang. */
>>>>        atomic_t guilty_count;
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>> b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>> index 632344c163a8..5fdac40015cf 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>> @@ -418,13 +418,17 @@ lrc_descriptor(struct intel_context *ce, struct
>>>> intel_engine_cs *engine)
>>>>        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 = ctx->desc_template;                /* bits  0-11 */
>>>> -    GEM_BUG_ON(desc & GENMASK_ULL(63, 12));
>>>> +    desc = INTEL_LEGACY_32B_CONTEXT;
>>>> +    if (i915_vm_is_4lvl(ce->vm))
>>>> +        desc = INTEL_LEGACY_64B_CONTEXT;
>>>
>>> if-else now that the vm null check is gone.
>>>
>>>> +    desc <<= GEN8_CTX_ADDRESSING_MODE_SHIFT;
>>>> +
>>>> +    desc |= GEN8_CTX_VALID | GEN8_CTX_PRIVILEGE;
>>>> +    if (IS_GEN(engine->i915, 8))
>>>> +        desc |= GEN8_CTX_L3LLC_COHERENT;
>>>
>>> Don't know.. it's nicer to keep it stored it both for Gen and context
>>> state. What's the problem with it?
>>
>> Ping.
> 
> There's no gem_context.

We could store it in ce then. We already have well defined control 
points for when vm changes when all are updated.

If done like this then it looks like assigning ctx->hw_id could also do 
the default_desc update, so that we can avoid even more work done at pin 
time.

Regards,

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