[Intel-gfx] [PATCH 05/10] drm/i915: Track runtime spent in unreachable intel_contexts

[Tvrtko Ursulin]

On 18/03/2020 14:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-03-18 14:13:14)
>>
>> On 18/03/2020 13:55, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-03-18 12:11:34)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> As contexts are abandoned we want to remember how much GPU time they used
>>>> (per class) so later we can used it for smarter purposes.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/i915_gem_context.c       | 13 ++++++++++++-
>>>>    drivers/gpu/drm/i915/gem/i915_gem_context_types.h |  5 +++++
>>>>    2 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> index 7c119a3a2cbd..5edf79ed6247 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> @@ -257,7 +257,19 @@ static void free_engines_rcu(struct rcu_head *rcu)
>>>>    {
>>>>           struct i915_gem_engines *engines =
>>>>                   container_of(rcu, struct i915_gem_engines, rcu);
>>>> +       struct i915_gem_context *ctx = engines->ctx;
>>>> +       struct i915_gem_engines_iter it;
>>>> +       struct intel_context *ce;
>>>> +
>>>> +       /* Transfer accumulated runtime to the parent GEM context. */
>>>> +       for_each_gem_engine(ce, engines, it) {
>>>> +               unsigned int class = ce->engine->uabi_class;
>>>>    
>>>> +               GEM_BUG_ON(class >= ARRAY_SIZE(ctx->past_runtime));
>>>> +               atomic64_add(ce->runtime.total, &ctx->past_runtime[class]);
>>>
>>> Hmm, there's an odd situation where the free_engines_rcu could fire
>>> before we do the final schedule_out of the context.
>>>
>>> GEM_BUG_ON(intel_context_inflight(ce)) to see if that's being too
>>> paranoid.
>>
>> Deja vu.. have I forgotten to move this into intel_context_free while
>> the purpose of keeping ce->gem_context valid was exactly to enable that.
> 
> I would rather not have it in gt/ if possible. The delay should be
> nominal at worst.

But I thought your concern was this can miss the accumulation of the 
last active period due active tracker triggering the rcu cleanup before 
last context save. What do you then recommend?

Regards,

Tvrtko