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

["Christian König" <christian.koenig@amd.com>]

Am 03.04.24 um 20:18 schrieb Kamil Konieczny:
> Hi Tvrtko,
> On 2024-04-03 at 16:00:57 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@amd.com>
> Christian Koenig <christian.koenig@amd.com>
> Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> Change looks ok so from me:
> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Acked-by: Christian König <christian.koenig@amd.com>

Thanks for taking care of this,
Christian.

>
> Regards,
> Kamil
>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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
>>