[PATCH i-g-t v2 15/21] lib/igt_drm_clients: Record total cycles

[Tvrtko Ursulin]

On 24/04/2024 00:44, Lucas De Marchi wrote:
> If the fdinfo data indicates there's drm-total-cycles, use it to
> propagate the engine utilization. Also add an assert in intel_gpu_top
> since it only knows how to deal with with engine time busyness.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>   lib/igt_drm_clients.c | 47 +++++++++++++++++++++++++++++++++++--------
>   lib/igt_drm_clients.h |  7 +++++--
>   lib/igt_drm_fdinfo.c  |  2 ++
>   tools/intel_gpu_top.c |  1 +
>   4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index 71a938b18..8b1959ae1 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -106,16 +106,41 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>   	c->last_runtime = 0;
>   	c->total_runtime = 0;
>   
> +	/*
> +	 * Make sure utilization type doesn't change between any 2 samples,
> +	 * otherwise the units don't match
> +	 */
> +	assert(c->dcut | info->dcut);
> +
>   	for (i = 0; i <= c->engines->max_engine_id; i++) {
>   		assert(i < ARRAY_SIZE(info->busy));
> -
> -		if (info->busy[i] < c->u[i].last_busy)
> -			continue; /* It will catch up soon. */
> -
> -		c->total_runtime += info->busy[i];
> -		c->u[i].delta_busy = info->busy[i] - c->u[i].last_busy;
> -		c->last_runtime += c->u[i].delta_busy;
> -		c->u[i].last_busy = info->busy[i];
> +		assert(i < ARRAY_SIZE(info->cycles));
> +		assert(i < ARRAY_SIZE(info->total_cycles));
> +
> +		switch (c->dcut) {
> +		case DCUT_ENGINE_TIME:
> +			if (info->busy[i] < c->u[i].last_busy)
> +				continue; /* It will catch up soon. */
> +
> +			c->total_runtime += info->busy[i];
> +			c->u[i].delta_busy = info->busy[i] - c->u[i].last_busy;
> +			c->last_runtime += c->u[i].delta_busy;
> +			c->u[i].last_busy = info->busy[i];
> +			break;
> +		case DCUT_GPU_TOTAL_CYCLES:
> +			if (info->cycles[i] < c->u[i].last_busy)
> +				continue; /* It will catch up soon. */
> +
> +			c->total_runtime += info->cycles[i];
> +			c->u[i].delta_busy = info->cycles[i] - c->u[i].last_busy;
> +			c->last_runtime += c->u[i].delta_busy;
> +			c->u[i].last_busy = info->cycles[i];
> +
> +			c->u[i].delta_total_cycles = info->total_cycles[i]
> +							- c->u[i].last_total_cycles;
> +			c->u[i].last_total_cycles = info->total_cycles[i];
> +			break;
> +		}
>   	}
>   
>   	/* Memory regions */
> @@ -207,6 +232,12 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>   	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
>   	assert(c->memory);
>   
> +	/* Prefer using GPU_TOTAL_CYCLES if available */
> +	if (info->dcut & DCUT_GPU_TOTAL_CYCLES)
> +		c->dcut = DCUT_GPU_TOTAL_CYCLES;
> +	else
> +		c->dcut = DCUT_ENGINE_TIME;

Hmm I have a feeling igt_drm_client shouldn't be making this decision 
but just tracking all available keys.

As such I would suggest making these flags instead of types.

(And with "dcut" naming spreading now I really think it is too obscure 
to stay.)

Something like:

if (info->utilisation_type & DRM_FDINFO_UTILISATION_TOTAL_CYCLES)
	c->utilisation_type |= DRM_CLIENT_TOTAL_CYCLES;
if (info->utilisation_type & DRM_FDINFO_UTILISATION_TIME)
	c->utilisation_type |= DRM_CLIENT_ENGINE_TIME;

Intel_gpu_top and gputop then pick when they know to work with, with the 
priority the application chooses.

Regards,

Tvrtko

> +
>   	igt_drm_client_update(c, pid, name, info);
>   }
>   
> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
> index 5c6130ff1..5ab6f6e2e 100644
> --- a/lib/igt_drm_clients.h
> +++ b/lib/igt_drm_clients.h
> @@ -65,9 +65,12 @@ struct igt_drm_client {
>   	unsigned int samples; /* Count of times scanning updated this client. */
>   	unsigned long total_runtime; /* Aggregate busyness on all engines since client start. */
>   	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
> +	enum drm_client_utilization_type dcut; /* Select method to calculate engine utiliazation */
>   	struct igt_drm_client_utilization {
> -		unsigned long delta_busy; /* Engine busyness data, relative to previous scan. */
> -		uint64_t last_busy; /* Engine busyness data as parsed from fdinfo. */
> +		unsigned long delta_busy; /* Engine busyness (either drm-cycles or drm-engine data), relative to previous scan. */
> +		unsigned long delta_total_cycles; /* Total engine cycles, relative to previous scan. */
> +		uint64_t last_busy; /* Engine busyness (either drm-cycles or drm-engine data) as parsed from fdinfo. */
> +		uint64_t last_total_cycles; /* Total engin cycles as parsed from fdinfo */
>   	} *u; /* Array of engine utilization */
>   	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>   };
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index 7a1485c66..292c7e4d8 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -270,10 +270,12 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>   			idx = parse_engine(l + keylen, info,
>   					   name_map, map_entries, &val);
>   			UPDATE_ENGINE(idx, cycles, val);
> +			has_cycles = true;
>   		} else if (strstartswith(l, "drm-total-cycles-", &keylen)) {
>   			idx = parse_engine(l + keylen, info,
>   					   name_map, map_entries, &val);
>   			UPDATE_ENGINE(idx, total_cycles, val);
> +			has_total_cycles = true;
>   		} else if (strstartswith(l, "drm-total-", &keylen)) {
>   			idx = parse_region(l + keylen, info,
>   					   region_map, region_entries, &val);
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index a4aa8414b..abbc30006 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -880,6 +880,7 @@ static struct igt_drm_clients *display_clients(struct igt_drm_clients *clients)
>   			break;
>   
>   		assert(c->status == IGT_DRM_CLIENT_ALIVE);
> +		assert(c->dcut == DCUT_ENGINE_TIME);
>   
>   		if (!cp || c->pid != cp->pid) {
>   			ac = &aggregated->client[num++];