Re: [PATCH i-g-t] gputop: Add memory only utilization type

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 21/11/2024 09:38, Christian Gmeiner wrote:
> From: Christian Gmeiner <cgmeiner@igalia.com>
> 
> Add special handling for drivers that only provide memory utilization
> fdinfo.

To check I am following correctly - special in this context means avoid 
displaying empty fields, or avoid a crash, or?

> Signed-off-by: Christian Gmeiner <cgmeiner@igalia.com>
> ---
>   tools/gputop.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 43b01f566..4135d84c1 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -35,6 +35,7 @@
>   enum utilization_type {
>   	UTILIZATION_TYPE_ENGINE_TIME,
>   	UTILIZATION_TYPE_TOTAL_CYCLES,
> +	UTILIZATION_TYPE_MEMORY_ONLY,
>   };
>   
>   static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> @@ -141,6 +142,9 @@ engines_identical(const struct igt_drm_client *c,
>   	    c->engines->max_engine_id != pc->engines->max_engine_id)
>   		return false;
>   
> +	if (!c->engines->num_engines && !pc->engines->num_engines)
> +		return true;

strcmp() below crashes?

I would move the check you added to be first in this function. I think 
that makes most sense from the flow wise - checking it before max_engine_id.

With this fixed what remains broken?

The loop in print_client() should ideally be skipped but even as is I 
think exits on the capacity check, no? At the moment it seems like this 
could be enough but maybe I am missing something.

Regards,

Tvrtko

> +
>   	for (i = 0; i <= c->engines->max_engine_id; i++)
>   		if (c->engines->capacity[i] != pc->engines->capacity[i] ||
>   		    !!c->engines->names[i] != !!pc->engines->names[i] ||
> @@ -193,6 +197,8 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>   		utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
>   	else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
>   		utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
> +	else if (c->regions->num_regions)
> +		utilization_type = UTILIZATION_TYPE_MEMORY_ONLY;
>   	else
>   		return 0;
>   
> @@ -209,6 +215,13 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>   	       if (!c->total_total_cycles)
>   		       return 0;
>   	       break;
> +	case UTILIZATION_TYPE_MEMORY_ONLY:
> +		for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
> +			sz += c->memory[i].total;
> +
> +		if (sz == 0)
> +			return 0;
> +		break;
>   	}
>   
>   	/* Print header when moving to a different DRM card. */
> @@ -234,6 +247,9 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>   
>   	lines++;
>   
> +	if (utilization_type == UTILIZATION_TYPE_MEMORY_ONLY)
> +		goto print_name;
> +
>   	for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
>   		double pct;
>   
> @@ -249,6 +265,9 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>   			pct = (double)c->utilization[i].delta_cycles / c->utilization[i].delta_total_cycles * 100 /
>   				c->engines->capacity[i];
>   			break;
> +		case UTILIZATION_TYPE_MEMORY_ONLY:
> +			/* Nothing to do. */
> +			break;
>   		}
>   
>   		/*
> @@ -262,6 +281,7 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>   		len += *engine_w;
>   	}
>   
> +print_name:
>   	printf(" %-*s\n", con_w - len - 1, c->print_name);
>   
>   	return lines;