| Message ID | 20200309183129.2296-6-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Per client engine busyness | expand |
Quoting Tvrtko Ursulin (2020-03-09 18:31:22) > From: Tvrtko Ursulin <tvrtko.ursulin@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@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 abc3a3e2fcf1..5f6861a36655 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]); > + } -> give this its own routine. > + > + i915_gem_context_put(ctx); > i915_sw_fence_fini(&engines->fence); > free_engines(engines); > } > @@ -540,7 +552,6 @@ static int engines_notify(struct i915_sw_fence *fence, > list_del(&engines->link); > spin_unlock_irqrestore(&ctx->stale.lock, flags); > } > - i915_gem_context_put(engines->ctx); Or accumulate here? Here we know the engines are idle and released, albeit there is the delay in accumulating after the swap. I'm not going to worry about that, live replacement of engines I don't expect anyone to notice the busy stats being off for a bit. Worst case is that they see a sudden jump; but typical practice will be to setup engines up before they being activity. We only have to worry about is if the transient misleading stats can be exploited. -Chris
On 10/03/2020 18:25, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-03-09 18:31:22) >> From: Tvrtko Ursulin <tvrtko.ursulin@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@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 abc3a3e2fcf1..5f6861a36655 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]); >> + } > > -> give this its own routine. Ack. >> + >> + i915_gem_context_put(ctx); >> i915_sw_fence_fini(&engines->fence); >> free_engines(engines); >> } >> @@ -540,7 +552,6 @@ static int engines_notify(struct i915_sw_fence *fence, >> list_del(&engines->link); >> spin_unlock_irqrestore(&ctx->stale.lock, flags); >> } >> - i915_gem_context_put(engines->ctx); > > Or accumulate here? Here we know the engines are idle and released, > albeit there is the delay in accumulating after the swap. I'm not going > to worry about that, live replacement of engines I don't expect anyone > to notice the busy stats being off for a bit. Worst case is that they > see a sudden jump; but typical practice will be to setup engines up > before they being activity. We only have to worry about is if the > transient misleading stats can be exploited. It was even here initially but then I started fearing it may not be the last unpin of intel_context, pending context save/complete so sounded safer to make it really really last. And But I guess you are right in saying that small error when replacing engines should not be large concern. If I move the accumulation back here I don't need the intel_context->closed patch any more so that's a plus. Unless it can be a large error if context ran for quite some time. Hm.. I think I still prefer to be safe and accumulate latest as possible. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index abc3a3e2fcf1..5f6861a36655 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]); + } + + i915_gem_context_put(ctx); i915_sw_fence_fini(&engines->fence); free_engines(engines); } @@ -540,7 +552,6 @@ static int engines_notify(struct i915_sw_fence *fence, list_del(&engines->link); spin_unlock_irqrestore(&ctx->stale.lock, flags); } - i915_gem_context_put(engines->ctx); break; case FENCE_FREE: 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 b0e03380c690..f0d7441aafc8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -177,6 +177,11 @@ struct i915_gem_context { spinlock_t lock; struct list_head engines; } stale; + + /** + * @past_runtime: Accumulation of freed intel_context pphwsp runtimes. + */ + atomic64_t past_runtime[MAX_ENGINE_CLASS + 1]; }; #endif /* __I915_GEM_CONTEXT_TYPES_H__ */