[PATCH i-g-t 07/12] lib/igt_drm_fdinfo: Parse unit for engine utilization

[Tvrtko Ursulin]

On 18/04/2024 23:48, Umesh Nerlige Ramappa wrote:
> On Fri, Apr 05, 2024 at 01:00:51AM -0500, Lucas De Marchi wrote:
>> Kernel adds a " ns" at the end of engine utilization. Make sure we parse
>> it so we don't fail if there's another suitable unit chosen by the
>> driver or another format.
>>
>> This prepares the ground for xe driver which will use 2 timestamps
>> rather than 1 with a different unit, to make sure it's compatible with
>> SR-IOV so we don't have to handle the conversion between GPU and CPU
>> clock domains.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> lib/igt_drm_fdinfo.c | 22 +++++++++++++++-------
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
>> index 5f05f210e..1541a62c9 100644
>> --- a/lib/igt_drm_fdinfo.c
>> +++ b/lib/igt_drm_fdinfo.c
>> @@ -63,9 +63,10 @@ static const char *ignore_space(const char *s)
>>
>> static int parse_engine(const char *name, struct drm_client_fdinfo *info,
>>             const char **name_map, unsigned int map_entries,
>> -            uint64_t *val)
>> +            uint64_t *val, const char **unit)
>> {
>>     const char *p;
>> +    char *end_ptr;
>>     size_t name_len;
>>     int found = -1;
>>     unsigned int i;
>> @@ -103,8 +104,15 @@ static int parse_engine(const char *name, struct 
>> drm_client_fdinfo *info,
>>         }
>>     }
>>
>> -    if (found >= 0)
>> -        *val = strtoull(p, NULL, 10);
>> +    if (found < 0)
>> +        return found;
>> +
>> +    *val = strtoull(p, &end_ptr, 10);
>> +    if (p == end_ptr)
>> +        return -1;
>> +
>> +    if (unit)
>> +        *unit = end_ptr;
>>
>>     return found;
>> }
>> @@ -212,7 +220,7 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, 
>> struct drm_client_fdinfo *info,
>>     while ((l = strtok_r(_buf, "\n", &ctx))) {
>>         uint64_t val = 0;
>>         size_t keylen;
>> -        const char *v;
>> +        const char *v, *unit;
>>         char *end_ptr;
>>         int idx;
>>
>> @@ -237,15 +245,15 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, 
>> struct drm_client_fdinfo *info,
>>             strncpy(info->pdev, v, sizeof(info->pdev) - 1);
>>         } else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
>>             idx = parse_engine(l + keylen, info,
>> -                       name_map, map_entries, &val);
>> +                       name_map, map_entries, &val, NULL);
>>             if (idx >= 0) {
>>                 info->capacity[idx] = val;
>>                 num_capacity++;
>>             }
>>         } else if (strstartswith(l, "drm-engine-", &keylen)) {
>>             idx = parse_engine(l + keylen, info,
>> -                       name_map, map_entries, &val);
>> -            if (idx >= 0) {
>> +                       name_map, map_entries, &val, &unit);
> 
> Should we ignore spaces in unit and then do a strcmp(unit, "ns")? OR are 
> we sure that there is just one space prior to unit here?

Any type and number of whitespace shouldbe handled.

I guess in drm-usage-stats.rst this needs to be improved:

"""
- Whitespace between the delimiter and first non-whitespace character 
shall be
   ignored when parsing.
- Keys are not allowed to contain whitespace characters.
- Numerical key value pairs can end with optional unit string.
"""

Perhaps append:

- Whitespace between the value and the unit shall be ignored when parsing.

Regards,

Tvrtko

> 
> Either ways,
> 
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> 
> Umesh
> 
>> +            if (idx >= 0 && !strcmp(unit, " ns")) {
>>                 if (!info->capacity[idx])
>>                     info->capacity[idx] = 1;
>>                 info->busy[idx] = val;
>> -- 
>> 2.44.0
>>