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

[Tvrtko Ursulin]

On 03/04/2024 21:27, Lucas De Marchi wrote:
> On Wed, Apr 03, 2024 at 04:00:57PM +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
> 
> That would be kind of ugly if each client uses a different engine.
> 
> Why not outputing all the engine classes regardless of what the
> clients are using?

It is a bit ugly but gputop doesn't know what are all the engine classes 
- it simply auto-detects them from the ones listed in fdinfo. To know 
"all" would need more code to be added. See ->

> 
> Lucas De Marchi
> 
>> 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.

-> this paragraph.

Can you live with it for now? Until someone decides to write a nicer UI 
for it?

Regards,

Tvrtko

>> 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
>>