[PATCH i-g-t] tools/gputop: Fix engine columns with amdgpu

[Kamil Konieczny]

Hi Tvrtko,
On 2024-04-03 at 16:00:57 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> 
> Amdgpu kernel driver skips output of fdinfo keys for unused engines. That
> is completely legal but corrupts the column formatting in the current
> code.
> 
> Fix it by simply treating change in detected engines used by a client
> as trigger to re-emit a new header. This ensures columns are always
> correctly aligned, albeit with a cost of potentially duplicating the
> header for the same DRM minor.
> 
> This is considered good enough for a reference implementation. The
> alternative would be to add some real per DRM minor state tracking which
> sounds like an overkill, at least until gputop gains a nicer (any) UI.

+Cc few amd devs:

Alex Deucher <alexander.deucher at amd.com>
Christian Koenig <christian.koenig at amd.com>
Vitaly Prosyak <vitaly.prosyak at amd.com>

Change looks ok so from me:
Acked-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

Regards,
Kamil

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> ---
>  tools/gputop.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 71e28f43ee4c..76075c5dde8b 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -119,11 +119,36 @@ print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
>  	return lines;
>  }
>  
> +static bool
> +engines_identical(const struct igt_drm_client *c,
> +		  const struct igt_drm_client *pc)
> +{
> +	unsigned int i;
> +
> +	if (c->engines->num_engines != pc->engines->num_engines ||
> +	    c->engines->max_engine_id != pc->engines->max_engine_id)
> +		return false;
> +
> +	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] ||
> +		    strcmp(c->engines->names[i], pc->engines->names[i]))
> +			return false;
> +
> +	return true;
> +}
>  
>  static bool
>  newheader(const struct igt_drm_client *c, const struct igt_drm_client *pc)
>  {
> -	return !pc || c->drm_minor != pc->drm_minor;
> +	return !pc || c->drm_minor != pc->drm_minor ||
> +	       /*
> +		* Below is a a hack for drivers like amdgpu which omit listing
> +		* unused engines. Simply treat them as separate minors which
> +		* will ensure the per-engine columns are correctly sized in all
> +		* cases.
> +		*/
> +	       !engines_identical(c, pc);
>  }
>  
>  static int
> -- 
> 2.44.0
>