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

[Tvrtko Ursulin <tursulin@ursulin.net>]

On 19/04/2024 17:21, Lucas De Marchi wrote:
> On Fri, Apr 19, 2024 at 08:58:25AM GMT, Tvrtko Ursulin wrote:
>>
>> 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@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 can change this in igt, but...
> 
>>
>> 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.
> 
> that would break userspace. Both nvtop and htop already do a strcmp
> with " ns". nvtop seems to use it for specific platforms (so as long as
> we don't change the existing one, it wouldn't break), but htop just
> handles that as "GPUs in Linux".

The joys of text formats failing to deliver flexibility. Might as well 
use binary..

Strictly speaking changing the spec would not break anything, as long as 
existing kernel side implementation stays. It would just ensure future 
implementation (on both sides) can be flexible and have been warned.

Regards,

Tvrtko