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

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

On 22/11/2024 20:19, Christian Gmeiner wrote:
> Hi
> 
>>> Hi Tvrtko
>>>
>>>>> 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?
>>>
>>> - Avoid early return in print_client(..) -> no output show.
>>> - Avoid a crash in strcmp(..).
>>>
>>>>
>>>>> 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?
>>>>
>>>
>>> Correct.
>>>
>>>> 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.
>>>>
>>>
>>> Will do in v2.
>>>
>>>> With this fixed what remains broken?
>>>>
>>>
>>> Show any output :)
>>
>> I totally missed it. Shows the value of commit messages. ;)
>>
>> Okay.. I wonder if we can get away without adding
>> UTILIZATION_TYPE_MEMORY_ONLY and instead make the code skip unavailable
>> data.
>>
>> Would something like this work:
>>
>> diff --git a/tools/gputop.c b/tools/gputop.c
>> index 43b01f566ac8..9639a7f5fde8 100644
>> --- a/tools/gputop.c
>> +++ b/tools/gputop.c
>> @@ -137,6 +137,9 @@ engines_identical(const struct igt_drm_client *c,
>>    {
>>          unsigned int i;
>>
>> +       if (!c->engines->num_engines && !pc->engines->num_engines)
>> +               return true;
>> +
>>          if (c->engines->num_engines != pc->engines->num_engines ||
>>              c->engines->max_engine_id != pc->engines->max_engine_id)
>>                  return false;
>> @@ -188,27 +191,20 @@ print_client(struct igt_drm_client *c, struct
>> igt_drm_client **prevc,
>>          uint64_t sz;
>>          int len;
>>
>> +       if (c->samples < 2)
>> +               return 0;
>> +
>>          if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_TOTAL_CYCLES &&
>> -           c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES)
>> +           c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES) {
>>                  utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
>> -       else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
>> +               /* Filter out idle clients. */
>> +               if (c->engines->num_engines && !c->total_total_cycles)
>> +                       return 0;
>> +       } else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME) {
>>                  utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
>> -       else
>> -               return 0;
>> -
>> -       if (c->samples < 2)
>> -               return 0;
>> -
>> -       /* Filter out idle clients. */
>> -       switch (utilization_type) {
>> -       case UTILIZATION_TYPE_ENGINE_TIME:
>> -              if (!c->total_engine_time)
>> -                      return 0;
>> -              break;
>> -       case UTILIZATION_TYPE_TOTAL_CYCLES:
>> -              if (!c->total_total_cycles)
>> -                      return 0;
>> -              break;
>> +               /* Filter out idle clients. */
>> +               if (c->engines->num_engines && !c->total_engine_time)
>> +                       return 0;
>>          }
>>
>>          /* Print header when moving to a different DRM card. */
>> @@ -234,7 +230,9 @@ print_client(struct igt_drm_client *c, struct
>> igt_drm_client **prevc,
>>
>>          lines++;
>>
>> -       for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
>> +       for (i = 0;
>> +            c->engines->num_engines && i <= c->engines->max_engine_id;
>> +            i++) {
>>                  double pct;
>>
>>                  if (!c->engines->capacity[i])
>>
> 
> With your patch I get:
> 
> DRM minor 128
> PID      MEM      RSS  NAME
> 1308      16M      16M  kmscube
> 1308       0B       0B  kmscube
> 
> 
> I think we should filter out the 0B MEM's one too - or?

Yeah it makes sense to be consistent.

> diff --git a/tools/gputop.c b/tools/gputop.c
> index 43b01f566..459cf213f 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -137,6 +137,9 @@ engines_identical(const struct igt_drm_client *c,
>   {
>       unsigned int i;
> 
> +    if (!c->engines->num_engines && !pc->engines->num_engines)
> +        return true;
> +
>       if (c->engines->num_engines != pc->engines->num_engines ||
>           c->engines->max_engine_id != pc->engines->max_engine_id)
>           return false;
> @@ -188,27 +191,28 @@ print_client(struct igt_drm_client *c, struct
> igt_drm_client **prevc,
>       uint64_t sz;
>       int len;
> 
> +    if (c->samples < 2)
> +        return 0;
> +
>       if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_TOTAL_CYCLES &&
> -        c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES)
> +        c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES) {
>           utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
> -    else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
> +        /* Filter out idle clients. */
> +        if (c->engines->num_engines && !c->total_total_cycles)
> +            return 0;
> +    } else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME) {
>           utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
> -    else
> -        return 0;
> +        /* Filter out idle clients. */
> +        if (c->engines->num_engines && !c->total_engine_time)
> +            return 0;
> +    }
> 
> -    if (c->samples < 2)
> -        return 0;
> +    if (c->regions->num_regions) {
> +        for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
> +            sz += c->memory[i].total;

Looks like this should work just fine. I could suggest how the loop 
could be consolidated with the one a bit lower in the function and maybe 
even "idle client" check consolidated to a helper but it is probably not 
worth the effort.

Regards,

Tvrtko

> 
> -    /* Filter out idle clients. */
> -    switch (utilization_type) {
> -    case UTILIZATION_TYPE_ENGINE_TIME:
> -           if (!c->total_engine_time)
> -               return 0;
> -           break;
> -    case UTILIZATION_TYPE_TOTAL_CYCLES:
> -           if (!c->total_total_cycles)
> -               return 0;
> -           break;
> +        if (sz == 0)
> +            return 0;
>       }
> 
>       /* Print header when moving to a different DRM card. */
> @@ -234,7 +238,9 @@ print_client(struct igt_drm_client *c, struct
> igt_drm_client **prevc,
> 
>       lines++;
> 
> -    for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
> +    for (i = 0;
> +        c->engines->num_engines && i <= c->engines->max_engine_id;
> +        i++) {
>           double pct;
> 
>           if (!c->engines->capacity[i])
> 
>