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 AB2AAC55162 for ; Thu, 30 Jul 2026 16:21:05 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5B52010E4D5; Thu, 30 Jul 2026 16:21:05 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="ficZxMSd"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3116210E4D5 for ; Thu, 30 Jul 2026 16:20:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1785428430; x=1816964430; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=jJnXMDYU9XN0MKlXTxxcFLxLxj4KiI9+9i0xT68zcqs=; b=ficZxMSdNs/Av8TvhGOpXuI1ei/kNt7DwTpfdDv4nh5rIH2FAN4ZDFPL 43oo552Y9psOo6H2jKrPGYHmG9Qj3ohpMG/5cDtPScH36XNHFpCH7lJ+7 W2SlMZxNymrBza95MZQTMqpip/rJoJt3lq1nFgLq5rCcCQ71OCsWcVtV1 AtmjFuSt1m4FMLW0DFbb+oliEfI1++88u0S7ZLOCYzmNWyW+XO7v1pieo 7/HM/GTd7KiyD6AQtHZzwjvsa8L14If4MIDyrTFpBC3dgP6VS4xCNNf5d 5AnfFW44DH1yv/B3wi5d2JKZASAh1DpAeOwt8/M49fs0aYwkGK57U4+xQ w==; X-CSE-ConnectionGUID: Na4IqqxEQWe/yCgKI+DLkA== X-CSE-MsgGUID: yaSJdI2SSISz+UFYku/yBg== X-IronPort-AV: E=McAfee;i="6800,10657,11860"; a="86067526" X-IronPort-AV: E=Sophos;i="6.25,194,1779174000"; d="scan'208";a="86067526" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jul 2026 09:20:29 -0700 X-CSE-ConnectionGUID: BnGSHf65QYSLXv31lV7D3Q== X-CSE-MsgGUID: 3OdehuBqSamseuLw5/HDOw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.25,194,1779174000"; d="scan'208";a="283685687" Received: from mcapezzu-mobl1.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.38.25]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jul 2026 09:20:29 -0700 Date: Thu, 30 Jul 2026 09:20:28 -0700 Message-ID: <877bmch2k3.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Shekhar Chauhan Cc: , Umesh Nerlige Ramappa Subject: Re: [PATCH] tools/xe-perf-recorder: expose some OA stream open properties In-Reply-To: References: <20260728063606.2874276-1-shekhar.chauhan@intel.com> <878q6tgpae.wl-ashutosh.dixit@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, 30 Jul 2026 01:13:19 -0700, Shekhar Chauhan wrote: > On 7/30/2026 8:24, Dixit, Ashutosh wrote: > > On Mon, 27 Jul 2026 23:36:06 -0700, Shekhar Chauhan wrote: > >> 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 > >> --- > >> tools/xe-perf/xe_perf_recorder.c | 105 ++++++++++++++++++++++++++++--- > >> 1 file changed, 98 insertions(+), 7 deletions(-) > >> > >> diff --git a/tools/xe-perf/xe_perf_recorder.c b/tools/xe-perf/xe_perf_recorder.c > >> index c69050b43..06d1016a0 100644 > >> --- a/tools/xe-perf/xe_perf_recorder.c > >> +++ b/tools/xe-perf/xe_perf_recorder.c > >> @@ -26,6 +26,7 @@ > >> #include > >> > >> #include "igt_core.h" > >> +#include "igt_sizes.h" > > Not needed, see below. > > > >> #include "intel_batchbuffer.h" > >> #include "intel_chipset.h" > >> #include "ioctl_wrappers.h" > >> @@ -364,6 +365,16 @@ struct recording_context { > >> struct drm_xe_oa_unit *oa_unit; > >> struct drm_xe_engine_class_instance *hwe; > >> > >> + /* Optional OA stream open properties */ > >> + bool oa_disabled; > >> + bool no_preempt; > >> + bool exec_queue_id_set; > >> + uint32_t exec_queue_id; > >> + bool oa_engine_instance_set; > >> + uint32_t oa_engine_instance; > >> + uint64_t oa_buffer_size; > >> + uint64_t wait_num_reports; > >> + > >> uint32_t vm; > >> uint32_t exec_queue; > >> struct intel_bb *ibb; > >> @@ -477,7 +488,7 @@ perf_open(struct recording_context *ctx) > >> { > >> int stream_fd; > >> > >> - uint64_t properties[] = { > >> + uint64_t properties[32] = { > >> DRM_XE_OA_PROPERTY_OA_UNIT_ID, ctx->oa_unit->oa_unit_id, > >> > >> /* Include OA reports in samples */ > >> @@ -488,10 +499,39 @@ perf_open(struct recording_context *ctx) > >> DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(ctx->metric_set->perf_oa_format), > >> DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, ctx->oa_exponent, > >> }; > >> - struct intel_xe_oa_open_prop param = { > >> - .num_properties = ARRAY_SIZE(properties) / 2, > >> - .properties_ptr = to_user_pointer(properties), > >> - }; > >> + struct intel_xe_oa_open_prop param = { 0 }; > >> + uint32_t n = 10; /* 5 mandatory properties (10 u64 slots) set above */ > >> + > >> + /* Optional properties, only passed when given on the command line */ > > Let us not do this new business of mandatory and optional properties. Just > > add the two properties we are adding in this patch (see below) to the > > properties array above and populate their default values: 64 MB for > > oa_buffer_size and 1 for wait_num_reports (and override the defaults if > > commond line input is provided), similar to what is done for the previously > > existing properties. > > > >> + if (ctx->oa_disabled) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_OA_DISABLED; > >> + properties[n++] = 1; > >> + } > >> + if (ctx->exec_queue_id_set) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID; > >> + properties[n++] = ctx->exec_queue_id; > >> + } > >> + if (ctx->oa_engine_instance_set) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE; > >> + properties[n++] = ctx->oa_engine_instance; > >> + } > >> + if (ctx->no_preempt) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_NO_PREEMPT; > >> + properties[n++] = 1; > >> + } > >> + if (ctx->oa_buffer_size) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE; > >> + properties[n++] = ctx->oa_buffer_size; > >> + } > >> + if (ctx->wait_num_reports) { > >> + properties[n++] = DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS; > >> + properties[n++] = ctx->wait_num_reports; > >> + } > >> + > >> + igt_assert(n <= ARRAY_SIZE(properties)); > >> + > >> + param.num_properties = n / 2; > >> + param.properties_ptr = to_user_pointer(properties); > >> > >> stream_fd = intel_xe_perf_ioctl(ctx->drm_fd, DRM_XE_OBSERVATION_OP_STREAM_OPEN, ¶m); > >> if (stream_fd < 0) { > >> @@ -924,7 +964,14 @@ usage(const char *name) > >> " --output, -o Output file (default = xe_perf.record)\n" > >> " --cpu-clock, -k Cpu clock to use for correlations\n" > >> " Values: boot, mono, mono_raw (default = mono)\n" > >> - " --oa-unit-id -u OA unit id for the capture.\n", > >> + " --oa-unit-id -u OA unit id for the capture.\n" > >> + " --oa-disabled -D Open the OA stream in the disabled state\n" > >> + " (the stream is enabled right after open)\n" > >> + " --exec-queue-id -q Exec queue id for context scoped capture\n" > >> + " --oa-engine-instance -e Engine instance for context scoped capture\n" > >> + " --no-preempt -n Disable preemption (requires --exec-queue-id)\n" > >> + " --oa-buffer-size -b OA buffer size in bytes (pow2, 128K - 128M)\n" > >> + " --wait-num-reports -w Min reports before poll()/read() unblocks\n", > >> name); > >> } > >> > >> @@ -1038,6 +1085,12 @@ 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-disabled", no_argument, 0, 'D'}, > > There is no point adding this command line option, if we are just going to > > re-enable the stream right after. > > > > Let us remove this command line option. But then always add the > > DRM_XE_OA_PROPERTY_OA_DISABLED internally, and re-enable the stream right > > after opening it. > But then, why do we even have this thing? Having a disabled option but not > giving it as a command line param, not allowing the user to disable the > stream, and even just add the DISABLED flag and then re-enable the stream > again, makes it counter-intuitive. > > Or is this because we can't touch the kernel code or if that thing is now > part of the uAPI and we can't edit it now.? > > I'll address rest of the changes as you asked. So the issue is not the command line options, the issue is that we want to exercise the kernel uAPI from xe_perf_recorder. So if the command line options also can be exposed, good. But if exposing command line options doesn't make sense, we need to implement things internally in xe_perf_recorder to exercise the kernel uAPI. So in this case we should add DRM_XE_OA_PROPERTY_OA_DISABLED internally and not expose it in the command line options. Thanks. > > > > >> + {"exec-queue-id", required_argument, 0, 'q'}, > >> + {"oa-engine-instance", required_argument, 0, 'e'}, > >> + {"no-preempt", no_argument, 0, 'n'}, > > I don't think these 3 can be command line options. Because we are not doing > > anything with them in xe_perf_recorder yet. > > > > The issue is: to pass exec-queue-id, the user will have to create an > > exec_queue, and then pass in the exec-queue-id. But even if the caller does > > this (say fork the xe_perf_recorder process), the exec-queue-id is created > > against the drm_fd in the parent process and the xe_perf_recorder will not > > find the exec_queue. So the command line option is useless. > > > > So in this patch I think just drop these 3 command line options. These will > > need to be implemented internally in xe_perf_recorder later and actually > > used (by creating a context specific buffer and issuing a > > MI_REPORT_PERF_COUNT command there and then dumping that report out, > > similar to what we do for mmio_trigger. > > > > So let's drop these 3 args from this patch. I will also confirm this with > > Umesh. > > > >> + {"oa-buffer-size", required_argument, 0, 'b'}, > >> + {"wait-num-reports", required_argument, 0, 'w'}, > > These two can be added in this patch. > > > >> {0, 0, 0, 0} > >> }; > >> const struct { > >> @@ -1068,7 +1121,7 @@ main(int argc, char *argv[]) > >> .oa_unit_id = 0, > >> }; > >> > >> - 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:Dq:e:nb:w:", long_options, NULL)) != -1) { > >> switch (opt) { > >> case 'h': > >> usage(argv[0]); > >> @@ -1119,6 +1172,26 @@ main(int argc, char *argv[]) > >> case 'u': > >> ctx.oa_unit_id = atoi(optarg); > >> break; > >> + case 'D': > >> + ctx.oa_disabled = true; > >> + break; > >> + case 'q': > >> + ctx.exec_queue_id = strtoul(optarg, NULL, 0); > >> + ctx.exec_queue_id_set = true; > >> + break; > >> + case 'e': > >> + ctx.oa_engine_instance = strtoul(optarg, NULL, 0); > >> + ctx.oa_engine_instance_set = true; > >> + break; > >> + case 'n': > >> + ctx.no_preempt = true; > >> + break; > >> + case 'b': > >> + ctx.oa_buffer_size = strtoull(optarg, NULL, 0); > >> + break; > >> + case 'w': > >> + ctx.wait_num_reports = strtoull(optarg, NULL, 0); > >> + break; > >> default: > >> fprintf(stderr, "Internal error: " > >> "unexpected getopt value: %d\n", opt); > >> @@ -1127,6 +1200,17 @@ main(int argc, char *argv[]) > >> } > >> } > >> > >> + if (ctx.no_preempt && !ctx.exec_queue_id_set) { > >> + fprintf(stderr, "--no-preempt requires --exec-queue-id\n"); > >> + return EXIT_FAILURE; > >> + } > >> + if (ctx.oa_buffer_size && > >> + ((ctx.oa_buffer_size & (ctx.oa_buffer_size - 1)) || > >> + ctx.oa_buffer_size < SZ_128K || ctx.oa_buffer_size > SZ_128M)) { > >> + fprintf(stderr, "--oa-buffer-size must be a power of 2 in [128K, 128M]\n"); > >> + return EXIT_FAILURE; > >> + } > > No need to do this check here. Kernel will check and fail the OA stream > > open if a weird size is passed in :-) > > > >> + > >> if (dev_node_id == -2) { > >> print_intel_devices(); > >> return EXIT_SUCCESS; > >> @@ -1291,6 +1375,13 @@ main(int argc, char *argv[]) > >> goto fail; > >> } > >> > >> + if (ctx.oa_disabled && > >> + 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 > >> > -- > Shekhar Chauhan > Linux Graphics Software Engineer > Intel Corporation >