Re: [PATCH] tools/xe-perf-recorder: Add mmio-trigger support

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

On Thu, 18 Jun 2026 22:02:17 -0700, Shekhar Chauhan wrote:
>
> Add mmio-trigger support to xe-perf-recorder, to justify the
> whitelisting of OA MMIO Trigger Registers in XeKMD.

Good description!

>
> Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> ---
>  tools/xe-perf/xe_perf_recorder.c | 83 ++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>
> diff --git a/tools/xe-perf/xe_perf_recorder.c b/tools/xe-perf/xe_perf_recorder.c
> index f200fe9c9..48a7d55d4 100644
> --- a/tools/xe-perf/xe_perf_recorder.c
> +++ b/tools/xe-perf/xe_perf_recorder.c
> @@ -27,8 +27,11 @@
>
>  #include "igt_core.h"
>  #include "intel_chipset.h"
> +#include "intel_batchbuffer.h"
> +#include "intel_reg.h"
>  #include "ioctl_wrappers.h"
>  #include "linux_scaffold.h"
> +#include "xe/xe_ioctl.h"
>  #include "xe/xe_oa.h"
>  #include "xe/xe_oa_data.h"
>  #include "xe/xe_query.h"

Could you fix these includes (even previous includes) to be in alphabetical
order, which is the convention we generally follow.

> @@ -39,6 +42,10 @@
>  #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof((arr)[0]))
>  #define MAX(a,b) ((a) > (b) ? (a) : (b))
>  #define MIN(a,b) ((a) < (b) ? (a) : (b))
> +#define OAG_MMIOTRIGGER 0xdb1c
> +#define TRIGGER_BB_SIZE 4096

Just use BATCH_SZ.

> +#define OAREPORT_REASON_MASK 0x3f
> +#define OAREPORT_REASON_SHIFT 19
>
>  struct circular_buffer {
>	char   *data;
> @@ -354,8 +361,21 @@ struct recording_context {
>	int oa_unit_id;
>	struct drm_xe_oa_unit *oa_unit;
>	struct drm_xe_engine_class_instance *hwe;
> +
> +	uint32_t vm;
> +	uint32_t exec_queue;
> +	struct intel_bb *ibb;
>  };
>
> +static uint32_t oa_unit_mmio_trigger_reg(struct recording_context *ctx)
> +{
> +	switch (ctx->oa_unit->oa_unit_type) {
> +	case DRM_XE_OA_UNIT_TYPE_OAG:
> +	default:
> +		return OAG_MMIOTRIGGER;
> +	}

This is not sufficient. The recorder currently supports all OA unit
types. Can we do similar to:

	struct xe_oa_regs regs = oa_unit_regs(oau);

as is done in __test_mmio_triggered_reports() in the IGT and initialize the
register(s) in the beginning. Different is also ok, but at least let us try
to cover all OA units.

Also, please test these changes with OAG and one of the
DRM_XE_OA_UNIT_TYPE_OAM OA unit.

> +}
> +
>  static void set_fd_flags(int fd, int flags)
>  {
>	int old = fcntl(fd, F_GETFL, 0);
> @@ -569,6 +589,23 @@ static bool write_stream_status(struct recording_context *ctx, FILE *output)
>	return true;
>  }
>
> +static void
> +check_mmio_trigger_report(struct recording_context *ctx, const void *report)
> +{
> +	const struct xe_oa_format *fmt = &oa_formats[ctx->metric_set->perf_oa_format];
> +	const uint32_t *report_32 = report;
> +	uint32_t reason = (report_32[0] >> OAREPORT_REASON_SHIFT) & OAREPORT_REASON_MASK;

Let's copy report_reason() function from the IGT too.

> +	uint64_t value;
> +
> +	if (reason)
> +		return;
> +
> +	value = (fmt->header == HDR_64_BIT) ? ((const uint64_t *)report)[2] : report_32[2];
> +
> +	if (value == 0xc0ffee01  || value == 0xc0ffee02)
> +		fprintf(stdout, "Received trigger report with value 0x%" PRIx64 "\n", value);

When you test this, do you see both these values?

> +}
> +
>  static bool write_stream_data(struct recording_context *ctx,
>			      char *data, ssize_t size, FILE *output)
>  {
> @@ -582,6 +619,8 @@ static bool write_stream_data(struct recording_context *ctx,
>			.size = sizeof(header) + format_size,
>		};
>
> +		check_mmio_trigger_report(ctx, data + i * format_size);
> +
>		if (fwrite(&header, sizeof(header), 1, output) != 1)
>			return false;
>
> @@ -697,6 +736,22 @@ write_correlation_timestamps(struct recording_context *ctx, FILE *output)
>	return write_saved_correlation_timestamps(output, &corr);
>  }
>
> +static void emit_mmio_triggered_report(struct intel_bb *ibb, uint32_t reg, uint32_t value)
> +{
> +	intel_bb_out(ibb, MI_LOAD_REGISTER_IMM(1));
> +	intel_bb_out(ibb, reg);
> +	intel_bb_out(ibb, value);
> +}
> +
> +static void emit_oa_trigger(struct recording_context *ctx, uint32_t value)
> +{
> +	emit_mmio_triggered_report(ctx->ibb, oa_unit_mmio_trigger_reg(ctx), value);
> +
> +	intel_bb_flush_render(ctx->ibb);
> +	intel_bb_sync(ctx->ibb);
> +	intel_bb_reset(ctx->ibb, false);
> +}
> +
>  static void
>  read_command_file(struct recording_context *ctx)
>  {
> @@ -726,6 +781,9 @@ read_command_file(struct recording_context *ctx)
>		if (file) {
>			struct chunk chunks[2];
>
> +			emit_oa_trigger(ctx, 0xc0ffee02);
> +			write_perf_data(ctx->output_stream, ctx);
> +

Looks correct to me, but I am still wondering if we should move these two
lines slightly above, just before the line:

		while (offset < len &&

>			fflush(ctx->output_stream);
>			get_chunks(chunks, &ctx->circular_buffer,
>				   false, ctx->circular_buffer.size);
> @@ -835,6 +893,13 @@ usage(const char *name)
>  static void
>  teardown_recording_context(struct recording_context *ctx)
>  {
> +	if (ctx->ibb)
> +		intel_bb_destroy(ctx->ibb);
> +	if (ctx->exec_queue)
> +		xe_exec_queue_destroy(ctx->drm_fd, ctx->exec_queue);
> +	if (ctx->vm)
> +		xe_vm_destroy(ctx->drm_fd, ctx->vm);
> +
>	if (ctx->topology)
>		free(ctx->topology);
>
> @@ -912,6 +977,18 @@ static int assign_oa_unit(int fd, struct recording_context *ctx)
>	return -1;
>  }
>
> +static int init_trigger_ctx(struct recording_context *ctx)
> +{
> +	ctx->vm = xe_vm_create(ctx->drm_fd, 0, 0);
> +	ctx->exec_queue = xe_exec_queue_create(ctx->drm_fd, ctx->vm, ctx->hwe, 0);
> +	ctx->ibb = intel_bb_create_with_context(ctx->drm_fd, ctx->exec_queue, ctx->vm,
> +				 NULL, TRIGGER_BB_SIZE);
> +	if (!ctx->ibb)
> +		return -1;
> +
> +	return 0;
> +}
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -1180,6 +1257,12 @@ main(int argc, char *argv[])
>		goto fail;
>	}
>
> +	if (init_trigger_ctx(&ctx) < 0) {
> +		fprintf(stderr, "Unable to initialize trigger context\n");
> +		goto fail;
> +	}
> +	emit_oa_trigger(&ctx, 0xc0ffee01);
> +
>	corr_period_ns = corr_period * 1000000000ul;
>	poll_time_ns = corr_period_ns;

Also, I think we need another emit_oa_trigger() just before the following
line:

	fprintf(stdout, "Exiting...\n");

Because this is the other case where data is directly written to an output
file, not to the circular buffer. For gpuvis we use the circular buffer, so
that is why likely you don't see an issue.

>
> --
> 2.53.0
>

Thanks.
--
Ashutosh