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

[Lucas De Marchi]

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

Lucas De Marchi

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