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

On Thu, 30 Jul 2026 22:45:43 -0700, Shekhar Chauhan wrote:
>

Hi Shekhar,

Looking good, just a few cleanup comments.

> 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 <[email protected]>
> ---
>  tools/xe-perf/xe_perf_recorder.c | 34 ++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xe-perf/xe_perf_recorder.c b/tools/xe-perf/xe_perf_recorder.c
> index c69050b43..01ec84cbb 100644
> --- a/tools/xe-perf/xe_perf_recorder.c
> +++ b/tools/xe-perf/xe_perf_recorder.c
> @@ -364,6 +364,10 @@ struct recording_context {
>	struct drm_xe_oa_unit *oa_unit;
>	struct drm_xe_engine_class_instance *hwe;
>
> +	/* OA stream open properties (defaults overridable via command line) */

Delete this comment. These are not the only OA properties, there were
already some here in recording_context.

> +	uint64_t oa_buffer_size;
> +	uint64_t wait_num_reports;
> +

No need to be fancy here with uint64_t, just make these int. Max
oa_buffer_size is only 128 MB (Bspec 61100).

>	uint32_t vm;
>	uint32_t exec_queue;
>	struct intel_bb *ibb;
> @@ -487,6 +491,13 @@ perf_open(struct recording_context *ctx)
>		DRM_XE_OA_PROPERTY_OA_METRIC_SET, ctx->metric_set->perf_oa_metrics_set,
>		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(ctx->metric_set->perf_oa_format),
>		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, ctx->oa_exponent,
> +
> +		/* Open disabled; the stream is enabled right after open */
> +		DRM_XE_OA_PROPERTY_OA_DISABLED, true,

Let's move this and the comment to the last property.

> +
> +		/* OA buffer size and wait-num-reports (overridable via cmdline) */

Delete this comment too, previous properties can also be overridden via cmdline.

> +		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, ctx->oa_buffer_size,
> +		DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS, ctx->wait_num_reports,
>	};
>	struct intel_xe_oa_open_prop param = {
>		.num_properties = ARRAY_SIZE(properties) / 2,
> @@ -924,7 +935,10 @@ 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-buffer-size      -b <value>  OA buffer size in bytes (default = 64M)\n"
> +		"     --wait-num-reports    -w <value>  Min reports before poll()/read() unblocks\n"

For the description just say "Num reports before unblocking poll()/read()\n"

> +		"                                       (default = 1)\n",
>		name);
>  }
>
> @@ -1038,6 +1052,8 @@ 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-buffer-size",	required_argument, 0, 'b'},
> +		{"wait-num-reports",	required_argument, 0, 'w'},
>		{0, 0, 0, 0}
>	};
>	const struct {
> @@ -1066,9 +1082,11 @@ main(int argc, char *argv[])
>		.command_fifo_fd = -1,
>
>		.oa_unit_id = 0,
> +		.oa_buffer_size = 64 * 1024 * 1024,
> +		.wait_num_reports = 1,
>	};
>
> -	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:b:w:", long_options, NULL)) != -1) {
>		switch (opt) {
>		case 'h':
>			usage(argv[0]);
> @@ -1119,6 +1137,12 @@ main(int argc, char *argv[])
>		case 'u':
>			ctx.oa_unit_id = atoi(optarg);
>			break;
> +		case 'b':
> +			ctx.oa_buffer_size = strtoull(optarg, NULL, 0);
> +			break;
> +		case 'w':
> +			ctx.wait_num_reports = strtoull(optarg, NULL, 0);
> +			break;

And with these int, just use atoi here.

>		default:
>			fprintf(stderr, "Internal error: "
>				"unexpected getopt value: %d\n", opt);
> @@ -1291,6 +1315,12 @@ main(int argc, char *argv[])
>		goto fail;
>	}
>
> +	if (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
>

Thanks.
--
Ashutosh