Re: [PATCH] tests/intel/xe_exec_capture: Enhance test to check with DUMPABLE flag

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2025-02-10 11:34 p.m., Gurram, Pravalika wrote:
> 
> 
>> -----Original Message-----
>> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Dong,
>> Zhanjun
>> Sent: Friday, February 7, 2025 8:46 PM
>> To: igt-dev@lists.freedesktop.org
>> Subject: Re: [PATCH] tests/intel/xe_exec_capture: Enhance test to check with
>> DUMPABLE flag
>>
>>
>>
>> On 2025-02-07 10:05 a.m., Dong, Zhanjun wrote:
>>>
>>>
>>> On 2025-02-05 1:41 p.m., pravalika gurram wrote:
>>>> check if the VM is there when DRM_XE_VM_BIND_FLAG_DUMPABLE is set in
>>>> the generated devcoredump.
>>>> check VM address within the range
>>>>
>>>> Signed-off-by: pravalika gurram <pravalika.gurram@intel.com>
>>>> ---
>>>>    tests/intel/xe_exec_capture.c | 64
>>>> +++++++++++++++++++++++++----------
>>>>    1 file changed, 46 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/tests/intel/xe_exec_capture.c b/tests/intel/
>>>> xe_exec_capture.c index 55ec3d4bd..4eaa3f4d1 100644
>>>> --- a/tests/intel/xe_exec_capture.c
>>>> +++ b/tests/intel/xe_exec_capture.c
>>>> @@ -53,16 +53,15 @@
>>>>    #define DUMP_PATH            "/sys/class/drm/card%d/device/
>>>> devcoredump/data"
>>>>    #define START_TAG            "**** Job ****"
>>>> -#define END_TAG                "**** VM state ****"
>>>>    /* Optional Space */
>>>> -#define SPC_O                "[ \t]*"
>>>> +#define SPC_O                "[ \t\\.]*"
>>>>    /* Required Space */
>>>> -#define SPC                "[ \t]+"
>>>> +#define SPC                "[ \t\\.]+"
>>>>    /* Optional Non-Space */
>>>> -#define NSPC_O                "([^ \t]*)"
>>>> +#define NSPC_O                "([^ \t\\.]*)"
>>>>    /* Required Non-Space */
>>>> -#define NSPC                "([^ \t]+)"
>>>> +#define NSPC                "([^ \t\\.]+)"
>>> Add "." as delimiter, that's fine.
>>>
>>>>    #define BEG                "^" SPC_O
>>>>    #define REQ_FIELD            NSPC SPC
>>>>    #define REQ_FIELD_LAST            NSPC SPC_O @@ -77,6 +76,8 @@
>>>>    #define INDEX_ENGINE_PHYSICAL        2
>>>>    #define INDEX_ENGINE_NAME        1
>>>>    #define INDEX_ENGINE_INSTANCE        4
>>>> +#define INDEX_VM_LENGTH            2 #define INDEX_VM_SIZE
>>>> +3
>>>>    static u64
>>>>    xe_sysfs_get_job_timeout_ms(int fd, struct
>>>> drm_xe_engine_class_instance *eci) @@ -177,7 +178,8 @@
>>>> test_legacy_mode(int fd, struct drm_xe_engine_class_instance *eci,
>>>> int n_exec_qu
>>>>        };
>>>>        sync[0].handle = syncobj_create(fd, 0);
>>>> -    xe_vm_bind_async(fd, vm, 0, bo, 0, addr, bo_size, sync, 1);
>>>> +    __xe_vm_bind_assert(fd, vm, 0, bo, 0, addr, bo_size,
>>>> +                DRM_XE_VM_BIND_OP_MAP, flags, sync, 1, 0, 0);
>>>>        for (i = 0; i < n_execs; i++) {
>>>>            u64 base_addr = addr;
>>>> @@ -287,10 +289,6 @@ static int load_all(FILE *fd, char **lines, char
>>>> *buf)
>>>>            /* Only save up to MAX_LINE_LEN to buffer */
>>>>            safe_strncpy(lines[i++], buf, MAX_LINE_LEN);
>>>> -
>>>> -        /* Stop on END_TAG */
>>>> -        if (!strncmp(END_TAG, buf, strlen(END_TAG)))
>>>> -            break;
>>> The target line located after END_TAG and is at the end of dump,
>>> that's why this search is removed. But could put comment in comment
>>> message about this? Remove search for something need some description,
>>> we don't want it to be removed silently.
>> And add example line, like:
>> 204 [1580001a0000].length: 0x10000
>> in comments somewhere, like check_item_u64 or where it was called, make it
>> easy to read.
>>
>> Regards,
>> Zhanjun Dong
>>>>        }
>>>>        return start_line;
>>>>    }
>>>> @@ -351,7 +349,6 @@ static char
>>>>                    value = &line[match[target_index].rm_so];
>>>>                    line[match[target_index].rm_eo] = '\0';
>>>>                }
>>>> -
>>>>                if (key && value && strcmp(tag, key) == 0)
>>>>                    return value;
>>>>                /* if key != tag,  keep searching and loop to next line
>>>> */ @@ -361,16 +358,44 @@ static char
>>>>        return NULL;
>>>>    }
>>>> +static uint64_t
>>>> +compare_hex_value(const char *output) {
>>>> +    char result[64];
>>>> +    uint64_t ret_val;
>>>> +    char *src = (char *)output, *dst = result;
>>>> +
>>>> +    if (src[0] == '0' && (src[1] == 'x' || src[1] == 'X'))
>>>> +        src += 2;
>>>       this is works with "0x123" case, to skip the leading "0x"
>>>       how about "[0x123]" case?
>          if (*src == '[' || *src == ']') {   ===> works for [0x123]
>          ret_val = strtoull(result, NULL, 16); this is skipping 0x output will be 123
> 
> Regards,
> Pravlika

2 things:
1. The leading "0x" removal not works for "[0x123]" case, you get 
correct value converted is because strtol can handle it. print result 
before convert, you will see the "0X" exist.
2. strtol(strtoull) can handle this leading "0x", so the above removal 
logic is not necessary.

Overall, the removal logic is either have bug or not necessary.
To be clear, this is an NACK.

Regards,
Zhanjun Dong
>>>> +
>>>> +    while (*src) {
>>>> +        if (*src == '[' || *src == ']') {
>>>> +            src++;
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        *dst = toupper((unsigned char)*src);
>>>> +        dst++;
>>>> +        src++;
>>>> +    }
>>>> +    *dst = '\0';
>>>> +    ret_val = strtoull(result, NULL, 16);
>>>> +    return ret_val;
>>>> +}
>>>> +
>>>>    static void
>>>> -check_item_u64(regex_t *regex, char **lines, const char *tag, u64
>>>> addr_lo, u64 addr_hi)
>>>> +check_item_u64(regex_t *regex, char **lines, const char *tag, u64
>>>> addr_lo,
>>>> +          u64 addr_hi, int tag_index, int target_index)
>>>>    {
>>>>        u64 result;
>>>>        char *output;
>>>> -    igt_assert_f((output = get_coredump_item(regex, lines, tag,
>>>> INDEX_KEY, INDEX_VALUE)),
>>>> +    igt_assert_f((output = get_coredump_item(regex, lines, tag,
>>>> tag_index, target_index)),
>>>>                 "Target not found:%s\n", tag);
>>>> -    result = strtoul(output, NULL, 16);
>>>> -    igt_debug("Compare %s %s vs [0x%lX-0x%lX]\n", tag, output,
>>>> addr_lo, addr_hi);
>>>> +
>>>> +    result = compare_hex_value(output);
>>>> +    igt_debug("Compare %s %s vs [0x%lX-0x%lX] result %lX\n", tag,
>>>> output,
>>>> +        addr_lo, addr_hi, result);
>>>>        igt_assert_f((addr_lo <= result) && (result <= addr_hi),
>>>>                 "value %lX out of range[0x%lX-0x%lX]\n", result,
>>>> addr_lo, addr_hi);
>>>>    }
>>>> @@ -435,7 +460,7 @@ static void test_card(int fd)
>>>>            igt_debug("Running on engine class: %x instance: %x\n",
>>>> hwe-
>>>>> engine_class,
>>>>                  hwe->engine_instance);
>>>> -        test_legacy_mode(fd, hwe, 1, 1, 0, addr);
>>>> +        test_legacy_mode(fd, hwe, 1, 1,
>>>> +DRM_XE_VM_BIND_FLAG_DUMPABLE,
>>>> addr);
>>>>            /* Wait 1 sec for devcoredump complete */
>>>>            sleep(1);
>>>> @@ -451,10 +476,13 @@ static void test_card(int fd)
>>>>            check_item_str(&regex, lines, "Capture_source:", INDEX_KEY,
>>>> INDEX_VALUE,
>>>>                       "GuC", false);
>>>> +
>>>>            check_item_u64(&regex, lines, "ACTHD:", addr,
>>>> -                   addr + BATCH_DW_COUNT * sizeof(u32));
>>>> +                   addr + BATCH_DW_COUNT * sizeof(u32), INDEX_KEY,
>>>> INDEX_VALUE);
>>>>            check_item_u64(&regex, lines, "RING_BBADDR:", addr,
>>>> -                   addr + BATCH_DW_COUNT * sizeof(u32));
>>>> +                   addr + BATCH_DW_COUNT * sizeof(u32), INDEX_KEY,
>>>> INDEX_VALUE);
>>>> +        check_item_u64(&regex, lines, "length:", addr,
>>>> +                   addr + BATCH_DW_COUNT * sizeof(u32), INDEX_VALUE,
>>>> INDEX_KEY);
>>>>            /* clear devcoredump */
>>>>            rm_devcoredump(path);
>>>
>