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

[Lucas De Marchi]

On Fri, Apr 26, 2024 at 12:11:33PM GMT, Tvrtko Ursulin wrote:
>
>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)

I guess this will start a war on british/american spelling :)

more seriously, see below

>	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. */

I was trying to spare yet another counter here since application
has to use one or the other.... but I see your point. Maybe we have too
many layers of abstractions here for what it's doing?
drm_fdinfo/drm_clients could be just one layer rather than 2.

Anyway, agreed with your points. I will change on next version.

thanks
Lucas De Marchi