Re: [PATCH] tools/xe-perf-recorder: expose some OA stream open properties

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

On Thu, 30 Jul 2026 01:13:19 -0700, Shekhar Chauhan wrote:
> On 7/30/2026 8:24, Dixit, Ashutosh wrote:
> > On Mon, 27 Jul 2026 23:36:06 -0700, Shekhar Chauhan wrote:
> >> xe-perf-recorder only passes 5 of the OA stream open properties to
> >> DRM_XE_OBSERVATION_OP_STREAM_OPEN, leaving the rest without an open
> >> source consumer. Add few of the remaining ones.
> >>
> >> Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> >> ---
> >>   tools/xe-perf/xe_perf_recorder.c | 105 ++++++++++++++++++++++++++++---
> >>   1 file changed, 98 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/xe-perf/xe_perf_recorder.c b/tools/xe-perf/xe_perf_recorder.c
> >> index c69050b43..06d1016a0 100644
> >> --- a/tools/xe-perf/xe_perf_recorder.c
> >> +++ b/tools/xe-perf/xe_perf_recorder.c
> >> @@ -26,6 +26,7 @@
> >>   #include <unistd.h>
> >>
> >>   #include "igt_core.h"
> >> +#include "igt_sizes.h"
> > Not needed, see below.
> >
> >>   #include "intel_batchbuffer.h"
> >>   #include "intel_chipset.h"
> >>   #include "ioctl_wrappers.h"
> >> @@ -364,6 +365,16 @@ struct recording_context {
> >>	struct drm_xe_oa_unit *oa_unit;
> >>	struct drm_xe_engine_class_instance *hwe;
> >>
> >> +	/* Optional OA stream open properties */
> >> +	bool oa_disabled;
> >> +	bool no_preempt;
> >> +	bool exec_queue_id_set;
> >> +	uint32_t exec_queue_id;
> >> +	bool oa_engine_instance_set;
> >> +	uint32_t oa_engine_instance;
> >> +	uint64_t oa_buffer_size;
> >> +	uint64_t wait_num_reports;
> >> +
> >>	uint32_t vm;
> >>	uint32_t exec_queue;
> >>	struct intel_bb *ibb;
> >> @@ -477,7 +488,7 @@ perf_open(struct recording_context *ctx)
> >>   {
> >>	int stream_fd;
> >>
> >> -	uint64_t properties[] = {
> >> +	uint64_t properties[32] = {
> >>		DRM_XE_OA_PROPERTY_OA_UNIT_ID, ctx->oa_unit->oa_unit_id,
> >>
> >>		/* Include OA reports in samples */
> >> @@ -488,10 +499,39 @@ perf_open(struct recording_context *ctx)
> >>		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(ctx->metric_set->perf_oa_format),
> >>		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, ctx->oa_exponent,
> >>	};
> >> -	struct intel_xe_oa_open_prop param = {
> >> -		.num_properties = ARRAY_SIZE(properties) / 2,
> >> -		.properties_ptr = to_user_pointer(properties),
> >> -	};
> >> +	struct intel_xe_oa_open_prop param = { 0 };
> >> +	uint32_t n = 10; /* 5 mandatory properties (10 u64 slots) set above */
> >> +
> >> +	/* Optional properties, only passed when given on the command line */
> > Let us not do this new business of mandatory and optional properties. Just
> > add the two properties we are adding in this patch (see below) to the
> > properties array above and populate their default values: 64 MB for
> > oa_buffer_size and 1 for wait_num_reports (and override the defaults if
> > commond line input is provided), similar to what is done for the previously
> > existing properties.
> >
> >> +	if (ctx->oa_disabled) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_OA_DISABLED;
> >> +		properties[n++] = 1;
> >> +	}
> >> +	if (ctx->exec_queue_id_set) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID;
> >> +		properties[n++] = ctx->exec_queue_id;
> >> +	}
> >> +	if (ctx->oa_engine_instance_set) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE;
> >> +		properties[n++] = ctx->oa_engine_instance;
> >> +	}
> >> +	if (ctx->no_preempt) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_NO_PREEMPT;
> >> +		properties[n++] = 1;
> >> +	}
> >> +	if (ctx->oa_buffer_size) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE;
> >> +		properties[n++] = ctx->oa_buffer_size;
> >> +	}
> >> +	if (ctx->wait_num_reports) {
> >> +		properties[n++] = DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS;
> >> +		properties[n++] = ctx->wait_num_reports;
> >> +	}
> >> +
> >> +	igt_assert(n <= ARRAY_SIZE(properties));
> >> +
> >> +	param.num_properties = n / 2;
> >> +	param.properties_ptr = to_user_pointer(properties);
> >>
> >>	stream_fd = intel_xe_perf_ioctl(ctx->drm_fd, DRM_XE_OBSERVATION_OP_STREAM_OPEN, &param);
> >>	if (stream_fd < 0) {
> >> @@ -924,7 +964,14 @@ usage(const char *name)
> >>		"     --output,             -o <path>   Output file (default = xe_perf.record)\n"
> >>		"     --cpu-clock,          -k <path>   Cpu clock to use for correlations\n"
> >>		"                                       Values: boot, mono, mono_raw (default = mono)\n"
> >> -		"     --oa-unit-id          -u <value>  OA unit id for the capture.\n",
> >> +		"     --oa-unit-id          -u <value>  OA unit id for the capture.\n"
> >> +		"     --oa-disabled         -D          Open the OA stream in the disabled state\n"
> >> +		"                                       (the stream is enabled right after open)\n"
> >> +		"     --exec-queue-id       -q <value>  Exec queue id for context scoped capture\n"
> >> +		"     --oa-engine-instance  -e <value>  Engine instance for context scoped capture\n"
> >> +		"     --no-preempt          -n          Disable preemption (requires --exec-queue-id)\n"
> >> +		"     --oa-buffer-size      -b <value>  OA buffer size in bytes (pow2, 128K - 128M)\n"
> >> +		"     --wait-num-reports    -w <value>  Min reports before poll()/read() unblocks\n",
> >>		name);
> >>   }
> >>
> >> @@ -1038,6 +1085,12 @@ main(int argc, char *argv[])
> >>		{"command-fifo",	required_argument, 0, 'f'},
> >>		{"cpu-clock",		required_argument, 0, 'k'},
> >>		{"oa-unit-id",		required_argument, 0, 'u'},
> >> +		{"oa-disabled",		no_argument,	   0, 'D'},
> > There is no point adding this command line option, if we are just going to
> > re-enable the stream right after.
> >
> > Let us remove this command line option. But then always add the
> > DRM_XE_OA_PROPERTY_OA_DISABLED internally, and re-enable the stream right
> > after opening it.
> But then, why do we even have this thing? Having a disabled option but not
> giving it as a command line param, not allowing the user to disable the
> stream, and even just add the DISABLED flag and then re-enable the stream
> again, makes it counter-intuitive.
>
> Or is this because we can't touch the kernel code or if that thing is now
> part of the uAPI and we can't edit it now.?
>
> I'll address rest of the changes as you asked.

So the issue is not the command line options, the issue is that we want to
exercise the kernel uAPI from xe_perf_recorder. So if the command line
options also can be exposed, good. But if exposing command line options
doesn't make sense, we need to implement things internally in
xe_perf_recorder to exercise the kernel uAPI.

So in this case we should add DRM_XE_OA_PROPERTY_OA_DISABLED internally and
not expose it in the command line options. Thanks.


>
> >
> >> +		{"exec-queue-id",	required_argument, 0, 'q'},
> >> +		{"oa-engine-instance",	required_argument, 0, 'e'},
> >> +		{"no-preempt",		no_argument,	   0, 'n'},
> > I don't think these 3 can be command line options. Because we are not doing
> > anything with them in xe_perf_recorder yet.
> >
> > The issue is: to pass exec-queue-id, the user will have to create an
> > exec_queue, and then pass in the exec-queue-id. But even if the caller does
> > this (say fork the xe_perf_recorder process), the exec-queue-id is created
> > against the drm_fd in the parent process and the xe_perf_recorder will not
> > find the exec_queue. So the command line option is useless.
> >
> > So in this patch I think just drop these 3 command line options. These will
> > need to be implemented internally in xe_perf_recorder later and actually
> > used (by creating a context specific buffer and issuing a
> > MI_REPORT_PERF_COUNT command there and then dumping that report out,
> > similar to what we do for mmio_trigger.
> >
> > So let's drop these 3 args from this patch. I will also confirm this with
> > Umesh.
> >
> >> +		{"oa-buffer-size",	required_argument, 0, 'b'},
> >> +		{"wait-num-reports",	required_argument, 0, 'w'},
> > These two can be added in this patch.
> >
> >>		{0, 0, 0, 0}
> >>	};
> >>	const struct {
> >> @@ -1068,7 +1121,7 @@ main(int argc, char *argv[])
> >>		.oa_unit_id = 0,
> >>	};
> >>
> >> -	while ((opt = getopt_long(argc, argv, "hc:d:p:m:Co:s:f:k:P:u:", long_options, NULL)) != -1) {
> >> +	while ((opt = getopt_long(argc, argv, "hc:d:p:m:Co:s:f:k:P:u:Dq:e:nb:w:", long_options, NULL)) != -1) {
> >>		switch (opt) {
> >>		case 'h':
> >>			usage(argv[0]);
> >> @@ -1119,6 +1172,26 @@ main(int argc, char *argv[])
> >>		case 'u':
> >>			ctx.oa_unit_id = atoi(optarg);
> >>			break;
> >> +		case 'D':
> >> +			ctx.oa_disabled = true;
> >> +			break;
> >> +		case 'q':
> >> +			ctx.exec_queue_id = strtoul(optarg, NULL, 0);
> >> +			ctx.exec_queue_id_set = true;
> >> +			break;
> >> +		case 'e':
> >> +			ctx.oa_engine_instance = strtoul(optarg, NULL, 0);
> >> +			ctx.oa_engine_instance_set = true;
> >> +			break;
> >> +		case 'n':
> >> +			ctx.no_preempt = true;
> >> +			break;
> >> +		case 'b':
> >> +			ctx.oa_buffer_size = strtoull(optarg, NULL, 0);
> >> +			break;
> >> +		case 'w':
> >> +			ctx.wait_num_reports = strtoull(optarg, NULL, 0);
> >> +			break;
> >>		default:
> >>			fprintf(stderr, "Internal error: "
> >>				"unexpected getopt value: %d\n", opt);
> >> @@ -1127,6 +1200,17 @@ main(int argc, char *argv[])
> >>		}
> >>	}
> >>
> >> +	if (ctx.no_preempt && !ctx.exec_queue_id_set) {
> >> +		fprintf(stderr, "--no-preempt requires --exec-queue-id\n");
> >> +		return EXIT_FAILURE;
> >> +	}
> >> +	if (ctx.oa_buffer_size &&
> >> +	    ((ctx.oa_buffer_size & (ctx.oa_buffer_size - 1)) ||
> >> +	     ctx.oa_buffer_size < SZ_128K || ctx.oa_buffer_size > SZ_128M)) {
> >> +		fprintf(stderr, "--oa-buffer-size must be a power of 2 in [128K, 128M]\n");
> >> +		return EXIT_FAILURE;
> >> +	}
> > No need to do this check here. Kernel will check and fail the OA stream
> > open if a weird size is passed in :-)
> >
> >> +
> >>	if (dev_node_id == -2) {
> >>		print_intel_devices();
> >>		return EXIT_SUCCESS;
> >> @@ -1291,6 +1375,13 @@ main(int argc, char *argv[])
> >>		goto fail;
> >>	}
> >>
> >> +	if (ctx.oa_disabled &&
> >> +	    perf_ioctl(ctx.perf_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0) < 0) {
> >> +		fprintf(stderr, "Unable to enable xe oa stream: %s\n",
> >> +			strerror(errno));
> >> +		goto fail;
> >> +	}
> >> +
> >>	init_mmio_trigger_ctx(&ctx);
> >>	emit_oa_trigger(&ctx, 0xc0ffee01);
> >>
> >> --
> >> 2.53.0
> >>
> --
> Shekhar Chauhan
> Linux Graphics Software Engineer
> Intel Corporation
>