Re: [PATCH i-g-t 4/4] tests/intel/xe_oa: Add an option to capture ftrace

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Sat, 23 Aug 2025 12:01:08 -0700, Dixit, Ashutosh wrote:
>
> On Fri, 22 Aug 2025 17:34:06 -0700, Umesh Nerlige Ramappa wrote:
> >
> > Capture ftrace with the --trace option when needed. This is not intended
> > to run as default, but more of a debug functionality when manually
> > running individual tests.
>
> Maybe mention here or in the commit title that we are capturing register
> read/writes in ftrace.
>
> >
> > In addition configure the ftrace buffer using the --trace_buf_size_mb
> >
> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > ---
> >  tests/intel/xe_oa.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> > index 7381e7b9bd44..e5473a5ff70f 100644
> > --- a/tests/intel/xe_oa.c
> > +++ b/tests/intel/xe_oa.c
> > @@ -307,6 +307,8 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
> >		.bc_report = 0 },
> >  };
> >
> > +static bool oa_trace = false;
> > +static uint32_t oa_trace_buf_mb = 20;
> >  static int drm_fd = -1;
> >  static int sysfs = -1;
> >  static int pm_fd = -1;
> > @@ -389,6 +391,31 @@ static u32 get_stream_status(int fd)
> >	return status.oa_status;
> >  }
> >
> > +static void enable_trace_log(void)
> > +{
> > +	char cmd[64] = {0};
> > +
> > +	if (!oa_trace || oa_trace_buf_mb > 20)

Actually, why is this oa_trace_buf_mb condition there in enable/disable? If
we don't want the trace buf to be > 20 MB, maybe return an error in
opt_handler?

Also, do we really need this oa_trace_buf_mb option? Maybe just select a
size and hardcode it? Or we need the option because we don't know the
hardcoded value?

Since we are only tracing xe_reg_rw, maybe even the default size is ok? How
many reg r/w's are there for an OA IGT? Maybe not that many? Since the reg
r/w's done by batches won't be captured here anyway, just the direct mmio
r/w's?

When we generate lots of ftrace, I have previously found the default ftrace
buffer size is not sufficient and printf's get dropped. Previously I used
512 KB (the largest size which worked in those old kernels) and that seemed
sufficient. But anyway even larger values are ok.

Anyway, ok to leave as is too. Just some ideas.

> > +		return;
> > +
> > +	snprintf(cmd, sizeof(cmd) - 1, "echo %d > /sys/kernel/debug/tracing/buffer_size_kb", oa_trace_buf_mb * 1000);

Maybe * 1024 instead?

> > +	system(cmd);
> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> > +	system("echo > /sys/kernel/debug/tracing/trace");
> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/enable");
> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/xe_reg_rw/enable");
> > +	system("echo 1 > /sys/kernel/debug/tracing/tracing_on");
> > +}
> > +
> > +static void disable_trace_log(void)
> > +{
> > +	if (!oa_trace || oa_trace_buf_mb > 20)
> > +		return;
> > +
> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> > +	system("cat /sys/kernel/debug/tracing/trace");
> > +}
> > +
> >  static void
> >  dump_report(const uint32_t *report, uint32_t size, const char *message) {
> >	uint32_t i;
> > @@ -4937,7 +4964,34 @@ static const char *xe_engine_class_name(uint32_t engine_class)
> >	igt_require_f(hwe, "no render engine\n"); \
> >	igt_dynamic_f("rcs-%d", hwe->engine_instance)
> >
> > -igt_main
> > +static int opt_handler(int opt, int opt_index, void *data)
> > +{
> > +	switch (opt) {
> > +	case 'b':
> > +		oa_trace_buf_mb = strtoul(optarg, NULL, 0);
> > +		igt_debug("Trace buffer %d Mb\n", oa_trace_buf_mb);
> > +		break;
> > +	case 't':
> > +		oa_trace = true;
> > +		igt_debug("Trace enabled\n");
> > +		break;
> > +	default:
> > +		return IGT_OPT_HANDLER_ERROR;
> > +	}
> > +
> > +	return IGT_OPT_HANDLER_SUCCESS;
> > +}
> > +
> > +static const char *help_str =  "  --trace | -t\t\tEnable ftrace\n"
> > +			       "  --trace_buf_size_mb | -b\t\tSet ftrace buffer size in Mb (default = 0, max = 20)\n";

s/Mb/MB/ ?

Thanks.
--
Ashutosh

> > +
> > +static struct option long_options[] = {
> > +	{"trace", 0, 0, 't'},
> > +	{"trace_buf_size_mb", 0, 0, 'b'},
> > +	{ NULL, 0, 0, 0 }
> > +};
> > +
> > +igt_main_args("b:t", long_options, help_str, opt_handler, NULL)
> >  {
> >	const struct sync_section {
> >		const char *name;
> > @@ -4981,6 +5035,7 @@ igt_main
> >		 */
> >		igt_assert_eq(drm_fd, -1);
> >
> > +		enable_trace_log();
> >		drm_fd = drm_open_driver(DRIVER_XE);
> >		xe_dev = xe_device_get(drm_fd);
> >
> > @@ -5210,5 +5265,6 @@ igt_main
> >			intel_xe_perf_free(intel_xe_perf);
> >
> >		drm_close_driver(drm_fd);
> > +		disable_trace_log();
>
> Does this get disabled even if the test crashes? Anyway since it is for
> debug and not enabled by default, this is:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> >	}
> >  }
> > --
> > 2.43.0
> >