[Intel-gfx] [PATCH v3] drm/i915/pmu: Reconstruct active state on starting busy-stats

[Tvrtko Ursulin]

On 11/01/2018 07:30, Chris Wilson wrote:
> We have a hole in our busy-stat accounting if the pmu is enabled during
> a long running batch, the pmu will not start accumulating busy-time
> until the next context switch. This then fails tests that are only
> sampling a single batch.
> 
> v2: Count each active port just once (context in/out events are only on
> the first and last assigment to a port).
> v3: Avoid hardcoding knowlege of 2 submission ports
> 
> Fixes: 30e17b7847f5 ("drm/i915: Engine busy time tracking")
> Testcase: igt/perf_pmu/busy-start
> Testcase: igt/perf_pmu/busy-double-start
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 6bb51a502b8b..d790bdc227ff 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1951,8 +1951,22 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>   	spin_lock_irqsave(&engine->stats.lock, flags);
>   	if (engine->stats.enabled == ~0)
>   		goto busy;
> -	if (engine->stats.enabled++ == 0)
> +	if (engine->stats.enabled++ == 0) {
> +		struct intel_engine_execlists *execlists = &engine->execlists;
> +		const struct execlist_port *port = execlists->port;
> +		unsigned int num_ports = execlists_num_ports(execlists);
> +
>   		engine->stats.enabled_at = ktime_get();
> +
> +		/* XXX submission method oblivious? */
> +		while (num_ports-- && port_isset(port)) {
> +			engine->stats.active++;
> +			port++;
> +		}

Argh, engine->timeline->lock is required to safely to this. But it needs 
to be outside the engine->stats.lock. I can't think of any problems 
doing it at the moment. What do you think?

Regards,

Tvrtko

> +
> +		if (engine->stats.active)
> +			engine->stats.start = engine->stats.enabled_at;
> +	}
>   	spin_unlock_irqrestore(&engine->stats.lock, flags);
>   
>   	return 0;
>