[PATCH v3] tests/intel/xe_exec_capture: Enhance test to check with DUMPABLE flag
Gurram, Pravalika
pravalika.gurram at intel.com
Mon Feb 17 06:06:53 UTC 2025
> -----Original Message-----
> From: Dong, Zhanjun <zhanjun.dong at intel.com>
> Sent: Thursday, February 13, 2025 3:13 AM
> To: Gurram, Pravalika <pravalika.gurram at intel.com>; igt-
> dev at lists.freedesktop.org
> Subject: Re: [PATCH v3] tests/intel/xe_exec_capture: Enhance test to check with
> DUMPABLE flag
>
>
>
> On 2025-02-12 10:32 a.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.
> > VM address is located after END_TAG and is at the end of dump, that's
> > why "stop on END_TAG" is removed.
> >
> > Signed-off-by: pravalika gurram <pravalika.gurram at intel.com>
> > ---
> > tests/intel/xe_exec_capture.c | 63 +++++++++++++++++++++++++----------
> > 1 file changed, 45 insertions(+), 18 deletions(-)
> >
> > diff --git a/tests/intel/xe_exec_capture.c
> > b/tests/intel/xe_exec_capture.c index 55ec3d4bd..4cc58928e 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\\.]+)"
> > #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;
> > }
> > 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,43 @@ static char
> > return NULL;
> > }
> >
> > +/* example i/p : [1580001a0000].length: 0x10000 */
> The comment right above a function is typically for function comments.
> For a static function, function comment is optional, but put example here
> without function comments is not a good practice.
> > +
> > +static uint64_t
> > +compare_hex_value(const char *output) {
> > + char result[64];
> > + uint64_t ret_val;
> > + char *src = (char *)output, *dst = result;
> > +
> You might put it here.
>
> With the comment corrected:
> Reviewed-by: Zhanjun Dong <zhanjun.dong at intel.com>
>
> But I have question about the text format, for:
> [1580001a0000].length: 0x10000
> Questions:
> 1. About [ ]
> How many values inside [ ]? From your implementation, it looks like a single
> value, then why put the single value inside [ ]?
> 2. About "."
> Right now, we don't have any float values, but if in the future we have it, "." is
> no longer a delimiter.
>
> So the text format could becomes:
>
> Block(or whatever name) address: 0x1580001a0000 length: 0x10000
>
> which might be more easy to read.
>
> I know this is out of scope of this review, but I want to raise the concern and if
> we don't have much tool is processing this, we are able to change it.
>
> Regards
> Zhanjun Dong
>
@Dong, Zhanjun This can be taken in a separate task once @Wajdeczko, Michal agree. Will raise another JIRA
I will go ahead with the current change to close the current JIRA.
FYI @Wajdeczko, Michal
When I have taken the devcoredump vm address is printed in following format "[1580001a0000].length: 0x10000"
Is it possible to change the format to " Block(or whatever name) address: 0x1580001a0000 length: 0x10000"
void xe_vm_snapshot_print(struct xe_vm_snapshot *snap, struct drm_printer *p)
{
unsigned long i, j;
if (IS_ERR_OR_NULL(snap)) {
drm_printf(p, "[0].error: %li\n", PTR_ERR(snap));
return;
}
for (i = 0; i < snap->num_snaps; i++) {
drm_printf(p, "[%llx].length: 0x%lx\n", snap->snap[i].ofs, snap->snap[i].len);
> > + 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 +459,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 +475,13 @@ static void test_card(int fd)
> >
> > check_item_str(®ex, lines, "Capture_source:", INDEX_KEY,
> INDEX_VALUE,
> > "GuC", false);
> > +
> > check_item_u64(®ex, lines, "ACTHD:", addr,
> > - addr + BATCH_DW_COUNT * sizeof(u32));
> > + addr + BATCH_DW_COUNT * sizeof(u32),
> INDEX_KEY,
> > +INDEX_VALUE);
> > check_item_u64(®ex, lines, "RING_BBADDR:", addr,
> > - addr + BATCH_DW_COUNT * sizeof(u32));
> > + addr + BATCH_DW_COUNT * sizeof(u32),
> INDEX_KEY, INDEX_VALUE);
> > + check_item_u64(®ex, lines, "length:", addr,
> > + addr + BATCH_DW_COUNT * sizeof(u32),
> INDEX_VALUE,
> > +INDEX_KEY);
> >
> > /* clear devcoredump */
> > rm_devcoredump(path);
More information about the igt-dev
mailing list