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

[Shekhar Chauhan <shekhar.chauhan@intel.com>]

On 6/30/2026 7:20, Dixit, Ashutosh wrote:
> 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.
Fixed in the new revision.
>
>> @@ -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.
Fixed in the new revision.
>
>> +#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.
Added this in the new revision. Borrowed some code from assign_oa_unit() 
and assign_hwe().
>
>> +}
>> +
>>   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.
Done.
>
>> +	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?

Yes, tried it on a PTL device with both OAG and OAM units. I opened two 
terminals, once for recording and other for sending a trigger, I could 
see that it was getting dumped. I also tried with a single terminal and 
then stopping it (via CTRL+C) and since the new revision is supposed to 
add a new trigger at the end, it showed me c0ffee02 when I stopped 
recording. Hence, I believe it works fine.

>
>> +}
>> +
>>   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:
Done this as well, makes sense, dump/write the triggers as soon as we 
get it and not wait for any other process.
>
> 		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.

Done this as well, explained this in the above comment on how it appears 
in testing.

-shekhar

>
>> --
>> 2.53.0
>>
> Thanks.
> --
> Ashutosh

-- 
Shekhar Chauhan
Linux Graphics Software Engineer
Intel Corporation