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

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

On Fri, 10 Jul 2026 16:34:55 -0700, Dixit, Ashutosh wrote:
>
> On Wed, 01 Jul 2026 22:23:15 -0700, Shekhar Chauhan wrote:
> >
> > Add mmio-trigger support to xe-perf-recorder, to justify the
> > whitelisting of OA MMIO Trigger Registers in XeKMD.
> >
> > v2: Add all OA unit types, fix includes, compile warnings and adding
> > extra triggger for file mode.
> >
> > Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> > ---
> >  tools/xe-perf/xe_perf_recorder.c | 122 ++++++++++++++++++++++++++++++-
> >  1 file changed, 120 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/xe-perf/xe_perf_recorder.c b/tools/xe-perf/xe_perf_recorder.c
> > index f200fe9c9..ecf1c7c27 100644
> > --- a/tools/xe-perf/xe_perf_recorder.c
> > +++ b/tools/xe-perf/xe_perf_recorder.c
> > @@ -26,19 +26,29 @@
> >  #include <unistd.h>
> >
> >  #include "igt_core.h"
> > +#include "intel_batchbuffer.h"
> >  #include "intel_chipset.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"
> >
> >  #include "xe_perf_recorder_commands.h"
> >
> > -#define ALIGN(v, a) (((v) + (a)-1) & ~((a)-1))
> > -#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 OAMERT_MMIOTRIGGER 0x1453cc
> > +#define OAM_MMIOTRIGGER_OFFSET 0x1d0
> > +#define MEDIA_GT_GSI_OFFSET 0x380000
> > +#define XE_OAM_SAG_BASE_ADJ (MEDIA_GT_GSI_OFFSET + 0x13000)
> > +#define XE_OAM_SCMI_0_BASE_ADJ (MEDIA_GT_GSI_OFFSET + 0x14000)
> > +#define XE_OAM_SCMI_1_BASE_ADJ (MEDIA_GT_GSI_OFFSET + 0x14800)
> > +#define OAREPORT_REASON_MASK 0x3f
> > +#define OAREPORT_REASON_SHIFT 19
> >
> >  struct circular_buffer {
> >	char   *data;
> > @@ -354,8 +364,47 @@ 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)
> > +{
> > +	const struct drm_xe_oa_unit *oau = ctx->oa_unit;
> > +
> > +	switch (oau->oa_unit_type) {
> > +	case DRM_XE_OA_UNIT_TYPE_OAM: {
> > +		struct drm_xe_query_oa_units *qoa = xe_oa_units(ctx->drm_fd);
> > +		uint8_t *poau = (uint8_t *)&qoa->oa_units[0];
> > +		int first_oam_id = -1;
> > +
> > +		/* Find the first OAM unit, like oa_unit_by_type() in the IGT */
>
> Let's change the comment to: "Find the first OAM unit, as in oa_unit_by_type()".
>
> > +		for (int i = 0; i < qoa->num_oa_units; i++) {
> > +			struct drm_xe_oa_unit *u = (struct drm_xe_oa_unit *)poau;
> > +
> > +			if (u->oa_unit_type == DRM_XE_OA_UNIT_TYPE_OAM) {
> > +				first_oam_id = u->oa_unit_id;
> > +				break;
> > +			}
> > +			poau += sizeof(*u) + u->num_engines * sizeof(u->eci[0]);
> > +		}
> > +
>
> assert here that first_oam_id is not -1.
>
>
> > +		if ((int)oau->oa_unit_id == first_oam_id)
>
> (int) cast is not needed, can you check.
>
> > +			return XE_OAM_SCMI_0_BASE_ADJ + OAM_MMIOTRIGGER_OFFSET;
> > +		return XE_OAM_SCMI_1_BASE_ADJ + OAM_MMIOTRIGGER_OFFSET;
> > +	}
> > +	case DRM_XE_OA_UNIT_TYPE_OAM_SAG:
> > +		return XE_OAM_SAG_BASE_ADJ + OAM_MMIOTRIGGER_OFFSET;
> > +	case DRM_XE_OA_UNIT_TYPE_MERT:
> > +		return OAMERT_MMIOTRIGGER;
> > +	case DRM_XE_OA_UNIT_TYPE_OAG:
> > +	default:
> > +		return OAG_MMIOTRIGGER;
>
> Let's do 'assert(0)' for the default case.
>
> > +	}
> > +}
> > +
> >  static void set_fd_flags(int fd, int flags)
> >  {
> >	int old = fcntl(fd, F_GETFL, 0);
> > @@ -569,6 +618,28 @@ static bool write_stream_status(struct recording_context *ctx, FILE *output)
> >	return true;
> >  }
> >
> > +static uint32_t
> > +report_reason(const uint32_t *report)
> > +{
> > +	return (report[0] >> OAREPORT_REASON_SHIFT) & OAREPORT_REASON_MASK;
> > +}
> > +
> > +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;
> > +	uint64_t value;
> > +
> > +	if (report_reason(report_32))
> > +		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);
> > +}
> > +
> >  static bool write_stream_data(struct recording_context *ctx,
> >			      char *data, ssize_t size, FILE *output)
> >  {
> > @@ -582,6 +653,8 @@ static bool write_stream_data(struct recording_context *ctx,
> >			.size = sizeof(header) + format_size,
> >		};
> >
> > +		check_mmio_trigger_report(ctx, data + i * format_size);
> > +
>
> I don't like adding check_mmio_trigger_report() here in the middle randomly when
> mmio triggers are not happening (since the two mmio triggers happen only once in
> the beginning and once in the end). Checking should be coupled with
> emit_mmio_triggered_report(). See below.
>
> >		if (fwrite(&header, sizeof(header), 1, output) != 1)
> >			return false;
> >
> > @@ -697,6 +770,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)
> >  {
> > @@ -712,6 +801,9 @@ read_command_file(struct recording_context *ctx)
> >		uint8_t *dump = malloc(len);
> >		FILE *file;
> >
> > +		emit_oa_trigger(ctx, 0xc0ffee02);
> > +		write_perf_data(ctx->output_stream, ctx);
> > +
>
> So thinking about this, since we are doing this right at the end, because
> of timing, write_perf_data() may or may not capture the mmio triggered
> report. There are all sorts of reasons why this mmio triggered report might
> be delayed (due to caching in the GPU etc.) and so not available in the OA
> buffer and therefore not read by write_perf_data().
>
> The way to confirm that the mmio triggered report has been captured would
> be to keep reading from perf_fd till we have verified that ctx_id in
> captured reports matches what we are inserting via emit_oa_trigger()
> (0xc0ffeexx).
>
> To me it looks like the best place to do this would be to modify
> write_perf_data() (which reads and writes out the data). We can do this in
> a future patch, if needed.
>
> So I want to remove check_mmio_trigger_report() from this patch. As I see
> it, it is added incorrectly to write_stream_data(). Instead, after
> inserting the mmio trigger, we should read repeatedly till we see the
> inserted ctx_id. Let's do this in another future patch, we can merge this
> patch first without check_mmio_trigger_report() and taking care of the
> other review comments. So let's make the initial problem a little bit
> simpler and then solve the second problem in a separate patch.

On second thoughts, let us leave check_mmio_trigger_report() in (as is) for
now, so we will at least have some indication and my guess is we will
occasionally see the second mmio triggered report missing.

So let us leave it in for now and later fix it by a future patch.

>
> >		while (offset < len &&
> >		       ((ret = read(ctx->command_fifo_fd,
> >				    (void *) dump + offset, len - offset)) > 0
> > @@ -835,6 +927,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);
>
> So if we make init_trigger_ctx() void (see below), I think all these 3 if
> statements can also be dropped.
>
> > +
> >	if (ctx->topology)
> >		free(ctx->topology);
> >
> > @@ -912,6 +1011,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, BATCH_SZ);
> > +	if (!ctx->ibb)
> > +		return -1;
>
> Looks like all the functions called here just assert, so to me there is no
> point checking for NULL etc. We can just make this function void?
>
> > +
> > +	return 0;
> > +}
> > +
> >  int
> >  main(int argc, char *argv[])
> >  {
> > @@ -1180,6 +1291,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;
> >
> > @@ -1227,6 +1344,7 @@ main(int argc, char *argv[])
> >		}
> >	}
> >
> > +	emit_oa_trigger(&ctx, 0xc0ffee02);
>
> Let's add this comment just above this line:
>
>	/* Emit second OA trigger for the case where circular buffer is not used */
>
> >	fprintf(stdout, "Exiting...\n");
> >
> >	if (!write_perf_data(ctx.output_stream, &ctx)) {
> > --
> > 2.53.0
> >