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

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-03-18 14:38:53)
> 
> 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?

That is the concern, but it's not a huge concern for me :)

I was tempted just to put a busywait here, rather than couple up a
notification scheme. Although...

We do have a notification here for the context_retire we could listen to
instead of listening to the request idling. If we use

	i915_sw_fence_await_active(&engines->fence,
				   &ce->active,
				   I915_ACTIVE_AWAIT_ALL);

instead, then the fence will not fire until the final barrier has
executed.

Tada!
-Chris