[Intel-gfx] [PATCH v2] drm/i915: Reduce context HW ID lifetime

[Tvrtko Ursulin]

On 05/09/2018 11:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-05 10:49:02)
>>
>> On 04/09/2018 16:31, Chris Wilson wrote:
>>> Future gen reduce the number of bits we will have available to
>>> differentiate between contexts, so reduce the lifetime of the ID
>>> assignment from that of the context to its current active cycle (i.e.
>>> only while it is pinned for use by the HW, will it have a constant ID).
>>> This means that instead of a max of 2k allocated contexts (worst case
>>> before fun with bit twiddling), we instead have a limit of 2k in flight
>>> contexts (minus a few that have been pinned by the kernel or by perf).
>>>
>>> To reduce the number of contexts id we require, we allocate a context id
>>> on first and mark it as pinned for as long as the GEM context itself is,
>>> that is we keep it pinned it while active on each engine. If we exhaust
>>> our context id space, then we try to reclaim an id from an idle context.
>>> In the extreme case where all context ids are pinned by active contexts,
>>> we force the system to idle in order to recover ids.
>>>
>>> We cannot reduce the scope of an HW-ID to an engine (allowing the same
>>> gem_context to have different ids on each engine) as in the future we
>>> will need to preassign an id before we know which engine the
>>> context is being executed on.
>>>
>>> v2: Improved commentary (Tvrtko) [I tried at least]
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Michel Thierry <michel.thierry at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_debugfs.c           |   5 +-
>>>    drivers/gpu/drm/i915/i915_drv.h               |   2 +
>>>    drivers/gpu/drm/i915/i915_gem_context.c       | 222 +++++++++++++-----
>>>    drivers/gpu/drm/i915/i915_gem_context.h       |  23 ++
>>>    drivers/gpu/drm/i915/intel_lrc.c              |   8 +
>>>    drivers/gpu/drm/i915/selftests/mock_context.c |  11 +-
>>>    6 files changed, 201 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 4ad0e2ed8610..1f7051e97afb 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
>>>                return ret;
>>>    
>>>        list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
>>> -             seq_printf(m, "HW context %u ", ctx->hw_id);
>>> +             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));
>>
>> Do you want to put some marker for the unallocated case here?
> 
> I was content with absence of marker as indicating it has never had an
> id, or it had been revoked.
> 
> Who reads this file anyway? There's not one igt where I've thought this
> would be useful debug info. Maybe I'm wrong...

True, the file as it is looks weak. It was only a question/suggestion 
anyway. Perhaps we can later improve the file to list them in a more 
modern/meaningful way.

Regards,

Tvrtko