[PATCH] test/intel/xe_pmt: Add testing for BMG crashlog

[Kamil Konieczny]

Hi Ruhl,,
On 2025-06-30 at 17:32:37 +0000, Ruhl, Michael J wrote:
> 
> >-----Original Message-----
> >From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> >Sent: Monday, June 30, 2025 6:13 AM
> >To: Ruhl, Michael J <michael.j.ruhl at intel.com>
> >Cc: igt-dev at lists.freedesktop.org; De Marchi, Lucas
> ><lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> >david.e.box at linux.intel.com
> >Subject: Re: [PATCH] test/intel/xe_pmt: Add testing for BMG crashlog
> >
> >Hi Michael,
> >On 2025-06-27 at 16:48:42 -0400, Michael J. Ruhl wrote:
> >> The BMG devices has the PMT crashlog feature. If the devices present
> >> is a BMG, test PMT api.
> >>
> >> NOTE: the testing order is not flexible and must be done in
> >> the currently specified order.
> >>
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
> >> ---
> >>  tests/intel/xe_pmt.c | 561
> >+++++++++++++++++++++++++++++++++++++++++++
> >>  tests/meson.build    |   1 +
> >>  2 files changed, 562 insertions(+)
> >>  create mode 100644 tests/intel/xe_pmt.c
> >>
> >> diff --git a/tests/intel/xe_pmt.c b/tests/intel/xe_pmt.c
> >> new file mode 100644
> >> index 000000000..21a2e74e7
> >> --- /dev/null
> >> +++ b/tests/intel/xe_pmt.c
> >> @@ -0,0 +1,561 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * Copyright © 2025 Intel Corporation
> >> + */
> >> +
> >> +/**
> >> + * TEST: Verify BMG PMT files operations
> >
> >Please remove 'BMG' from test description here, as this could be
> >used for future GPUs. Also write full name for 'PMT', my guess
> >it is "Power Menagement Telemetry" but I could be wrong here.
> >
> >Second note - this is for crash logs, what about PM telemetry
> >for GPU? Could it also be tested?
> >
> >> + * Category: Core
> >> + * Mega feature: General Core features
> >> + * Sub-category: uapi
> >> + * Functionality: sysfs
> >> + * Description: Verify BMG PMT files are created and are accessable
> >
> >Same here, remove 'BMG'
> >
> >> + */
> >> +
> >> +#include <dirent.h>
> >> +#include <limits.h>
> >> +#include <string.h>
> >> +#include <fcntl.h>
> >
> >Move 'fcntl.h' to proper place.
> >
> >> +#include <unistd.h>
> >
> >You could place it as first one (this is an exception), up to
> >you.
> >
> >> +
> >> +#include "igt.h"
> >> +#include "igt_sysfs.h"
> >> +#include "linux_scaffold.h"
> >> +#include "xe_drm.h"
> >> +#include "xe/xe_ioctl.h"
> >> +#include "xe/xe_query.h"
> >> +
> >> +/* base directory names */
> >> +#define VSEC_CRASHLOG_DIR "intel_vsec.crashlog."
> >> +#define VSEC_TELEMETRY_DIR "intel_vsec.telemetry."
> >> +#define CRASHLOG_DIR "crashlog"
> >> +#define TELEMETRY_DIR "telem"
> >> +#define BMG_CRASHLOG_CNT 2
> >> +#define BMG_TELEMETRY_CNT 2
> >
> >Why 'BMG_' prefix here?
> >
> >> +
> >> +enum bmg_crashlog_instances {
> >> +	bmg_crashlog_punit = 0,
> >> +	bmg_crashlog_oobmsm,
> >> +	bmg_crashlog_max
> >> +};
> >
> >What is "oobmsm" shortcut?
> >
> >> +
> >> +enum bmg_telemety_instances {
> >> +	bmg_telemetry_punit = 0,
> >> +	bmg_telemetry_oobmsm,
> >> +	bmg_telemetry_max
> >> +};
> >> +
> >> +static char dev_path[PATH_MAX];
> >> +static char work_path[PATH_MAX * 2];
> >> +
> >> +/*
> >> + * In most case there should be a single instance of crashlog an telemetry
> >directories.
> >
> >'an'? Did you mean 'in'?
> >
> >> + * If DVSEC entries are separate the structure will be different.
> >> + */
> >> +static char crashlog_vsec_dir[32];
> >> +static char telemetry_vsec_dir[32];
> >> +
> >> +/* This needs to be specific for each supported device */
> >> +static char crashlog_dir[bmg_crashlog_max][32];
> >> +static char telemetry_dir[bmg_telemetry_max][32];
> >> +
> >> +/* telemetry file names */
> >> +static const char *telem = "telem";
> >> +
> >> +/* crashlog filenames and descriptors */
> >> +static const char *clear = "clear";
> >> +static const char *consumed = "consumed";
> >> +static const char *crashlog = "crashlog";
> >> +static const char *enable = "enable";
> >> +static const char *error = "error";
> >> +static const char *dev_guid = "guid";
> >> +static const char *rearm = "rearm";
> >> +static const char *trigger = "trigger";
> >> +
> >> +struct crashlog_v2_info {
> >> +	int clear_fd;
> >> +	int consumed_fd;
> >> +	int crashlog_fd;
> >> +	int enable_fd;
> >> +	int error_fd;
> >> +	int guid_fd;
> >> +	int rearm_fd;
> >> +	int trigger_fd;
> >> +	u_int32_t guid;
> >> +} bmg_info[bmg_crashlog_max];
> >> +
> >> +#define DEV_PATH_LEN 80
> >> +
> >> +/**
> >> + * device_sysfs_path:
> >> + * @fd: opened device file descriptor
> >> + * @path: buffer to store sysfs path to device directory
> >> + *
> >> + * Returns:
> >> + * On successfull path resolution sysfs path to device directory,
> >> + * NULL otherwise
> >> + */
> >> +static char *device_sysfs_path(int fd, char *path)
> >> +{
> >> +        char sysfs[DEV_PATH_LEN];
> >> +
> >> +        if (!igt_sysfs_path(fd, sysfs, sizeof(sysfs)))
> >> +                return NULL;
> >> +
> >> +        if (DEV_PATH_LEN <= (strlen(sysfs) + strlen("/device")))
> >> +                return NULL;
> >> +
> >> +        strcat(sysfs, "/device");
> >> +
> >> +        return realpath(sysfs, path);
> >> +}
> >> +
> >> +/**
> >> + * SUBTEST: test_pmt_directories
> >> + * BMG PMT directory structure:
> >> + * device/intel_vsec.crashlog.x/intel_pmt/crashlog<a,b>
> >> + * device/intel_vsec.telemetry.x/intel_pmt/telemetry<c,d>
> >> + *
> >> + * If this is done for a different platform, this could be
> >> + * different.
> >> + *
> >> + */
> >> +static void test_pmt_directories(int dev_fd)
> >> +{
> >> +	struct dirent *ent;
> >> +	int index;
> >> +	DIR *dir;
> >> +
> >> +        igt_assert(device_sysfs_path(dev_fd, dev_path));
> >> +
> >> +	/* verify top level PMT directories */
> >> +	dir = opendir(dev_path);
> >> +	igt_assert_f(dir, "no directories found\n");
> >
> >Why not igt_require_f()?
> 
> Hi Kamil,
> 
> Looking at the documentation for igt_require_f() I see:
> 
> "Should be used everywhere where a test checks results to decide about skipping."
> 
> I do not want to skip this test.  If the directory is not there, there is something wrong, and the test
> should fail.
> 
> Am I mis-understanding the usage here?
> 
> Thanks,
> 
> Mike

This is up to you, if these will be present in Xe driver
then yes, you could have igt_assert_f.

Regards,
Kamil

[...cut...]