From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DCE61C43458 for ; Tue, 30 Jun 2026 01:50:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5E82210E128; Tue, 30 Jun 2026 01:50:33 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="jlmjPj9S"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) by gabe.freedesktop.org (Postfix) with ESMTPS id E824810E128 for ; Tue, 30 Jun 2026 01:50:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1782784206; x=1814320206; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=cQ6JpiT50j0pWIfg13rYGWyfKCQ6JU5nF9C2zqVnCog=; b=jlmjPj9SltF5X4oMWIA/eG9zAGGrlMwi7YkSAlHj4wHvzCOXw5fk8NgX 8BC+xf+iZBWNTvwh+lUsdgC+CfcbHB7P5yLsGNnxQpgMSAE+USEXXteyQ vVmZMbTOeIMOFShJtCOG4earC0PeqZHLxEPwVIGYQjd85k/vqPv85bs3r 7UN+u9BWpf/oG1J9mx/6HivJl9HWY1eMtlY4WtV8VNxwc8i5i/v8X/EUl QdGWBz3YsgOYDcuN/RM6r9RZuAM8n2+z/DhBI5z0wYkSJFr8l02K0RvVc 9m10Lwx/P+UoWnA545/eWJGmih4f3MRCiRrJvMXS7EwPXtXcZ1ndbB7NG Q==; X-CSE-ConnectionGUID: De2zexmnQ3eHFeCweebpMg== X-CSE-MsgGUID: KNrHBY7DR9qB+rVY+D5ktQ== X-IronPort-AV: E=McAfee;i="6800,10657,11832"; a="106283701" X-IronPort-AV: E=Sophos;i="6.24,233,1774335600"; d="scan'208";a="106283701" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by orvoesa101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jun 2026 18:50:05 -0700 X-CSE-ConnectionGUID: mv11Iey5QNOXZok4795HdQ== X-CSE-MsgGUID: /gtuWiY+Qzu0fzuW8rsrRw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.24,233,1774335600"; d="scan'208";a="248145123" Received: from phunguye-mobl.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.64.148]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jun 2026 18:50:05 -0700 Date: Mon, 29 Jun 2026 18:50:04 -0700 Message-ID: <87a4sckd9v.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Shekhar Chauhan Cc: Subject: Re: [PATCH] tools/xe-perf-recorder: Add mmio-trigger support In-Reply-To: <20260619050217.2112851-1-shekhar.chauhan@intel.com> References: <20260619050217.2112851-1-shekhar.chauhan@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-BeenThere: igt-dev@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development mailing list for IGT GPU Tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" 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 > --- > 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