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 177E4C52D71 for ; Thu, 8 Aug 2024 20:21:16 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A45B910E7FD; Thu, 8 Aug 2024 20:21:15 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="eF9VBOCT"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6938610E7FD for ; Thu, 8 Aug 2024 20:21:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1723148475; x=1754684475; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=JuhAsBcNfI8xj7+ZhJ+E2V6umJK1Qazk+Zbel1xm7eg=; b=eF9VBOCTIxsgz7VHZoLEWXbjbJUcvFdY2LvNm/4EcViNtMutF1sLVGoW zDwNZqknureby3pD7emNYMg91ekJ7rFKNU5fcSydITvW80+AB4px9x/m8 i8oaPra1IWY0BaPKpqHGWc0Dc2MaVDyqRt1P8lJI4pT42IBMdnt55GaEY 5q/sLTVo9ePxFb8/FobzpVoxb+/skZSPoo5MaCdr+YyGFUH9UPcShEPWr pVTxokyiT0d8QKpLGEu9R/wbIpJl9jtoSDOhz/xeHMuSyqPvgfCuvf8OV lFvC6UpFRWQ30DiBr1CwquHEkTsj8RJ2rO6KwFdemSKPjvdDkP5pYFHBp A==; X-CSE-ConnectionGUID: k5wmnPbBSiSpC5XxZWkFQA== X-CSE-MsgGUID: dACSEnaWR9aO636qTc8iYw== X-IronPort-AV: E=McAfee;i="6700,10204,11158"; a="38758226" X-IronPort-AV: E=Sophos;i="6.09,274,1716274800"; d="scan'208";a="38758226" Received: from orviesa006.jf.intel.com ([10.64.159.146]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Aug 2024 13:21:14 -0700 X-CSE-ConnectionGUID: q7D2MREnS8WCrDkZOCPj0Q== X-CSE-MsgGUID: sVuhvtJjRD6uMRBJyxDRHg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,274,1716274800"; d="scan'208";a="57570616" Received: from dgeminix-mobl6.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.241.225.69]) by orviesa006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Aug 2024 13:21:14 -0700 Date: Thu, 08 Aug 2024 13:21:12 -0700 Message-ID: <87o7623gw7.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Cavitt, Jonathan" Cc: "igt-dev@lists.freedesktop.org" , "Nerlige Ramappa, Umesh" , "Souza, Jose" , "Landwerlin, Lionel G" Subject: Re: [PATCH i-g-t] tests/intel/xe_oa: Add syncs-signal test for OA syncs In-Reply-To: References: <20240808173632.4027344-1-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/29.4 (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, 08 Aug 2024 12:45:40 -0700, Cavitt, Jonathan wrote: > Hi Jonathan, > -----Original Message----- > From: igt-dev On Behalf Of Ashutosh Dixit > Sent: Thursday, August 8, 2024 10:37 AM > To: igt-dev@lists.freedesktop.org > Cc: Nerlige Ramappa, Umesh ; Souza, Jose ; Landwerlin, Lionel G > Subject: [PATCH i-g-t] tests/intel/xe_oa: Add syncs-signal test for OA syncs > > > > Verify OA syncs signal correctly in open and change config code paths. > > > > Signed-off-by: Ashutosh Dixit > > I have a non-blocking suggestion detailed below that there may be merit to > separating the "open" and "change" code path sync tests into two different > tests. Although, I may be misinterpreting how the test works such that doing > so breaks the test due to the sync on open being necessary in all cases, so feel > free to disregard. > Reviewed-by: Jonathan Cavitt > > > --- > > include/drm-uapi/xe_drm.h | 12 +++++ > > lib/xe/xe_oa.c | 6 +-- > > lib/xe/xe_oa.h | 2 + > > tests/intel/xe_oa.c | 97 +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 114 insertions(+), 3 deletions(-) > > > > diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h > > index 29425d7fdc..f9f0d706cf 100644 > > --- a/include/drm-uapi/xe_drm.h > > +++ b/include/drm-uapi/xe_drm.h > > @@ -1632,6 +1632,18 @@ enum drm_xe_oa_property_id { > > * to be disabled for the stream exec queue. > > */ > > DRM_XE_OA_PROPERTY_NO_PREEMPT, > > + > > + /** > > + * @DRM_XE_OA_PROPERTY_NUM_SYNCS: Number of syncs in the sync array > > + * specified in @DRM_XE_OA_PROPERTY_SYNCS > > + */ > > + DRM_XE_OA_PROPERTY_NUM_SYNCS, > > + > > + /** > > + * @DRM_XE_OA_PROPERTY_SYNCS: Pointer to struct @drm_xe_sync array > > + * with array size specified via @DRM_XE_OA_PROPERTY_NUM_SYNCS > > + */ > > + DRM_XE_OA_PROPERTY_SYNCS, > > }; > > > > /** > > diff --git a/lib/xe/xe_oa.c b/lib/xe/xe_oa.c > > index 4764ed1fcf..b1a9ff7fa4 100644 > > --- a/lib/xe/xe_oa.c > > +++ b/lib/xe/xe_oa.c > > @@ -1027,8 +1027,8 @@ const char *intel_xe_perf_read_report_reason(const struct intel_xe_perf *perf, > > return "unknown"; > > } > > > > -static void xe_oa_prop_to_ext(struct intel_xe_oa_open_prop *properties, > > - struct drm_xe_ext_set_property *extn) > > +void intel_xe_oa_prop_to_ext(struct intel_xe_oa_open_prop *properties, > > + struct drm_xe_ext_set_property *extn) > > { > > __u64 *prop = from_user_pointer(properties->properties_ptr); > > struct drm_xe_ext_set_property *ext = extn; > > @@ -1065,7 +1065,7 @@ int intel_xe_perf_ioctl(int fd, enum drm_xe_observation_op op, void *arg) > > struct intel_xe_oa_open_prop *oprop = (struct intel_xe_oa_open_prop *)arg; > > > > igt_assert_lte(oprop->num_properties, XE_OA_MAX_SET_PROPERTIES); > > - xe_oa_prop_to_ext(oprop, ext); > > + intel_xe_oa_prop_to_ext(oprop, ext); > > } > > > > return igt_ioctl(fd, DRM_IOCTL_XE_OBSERVATION, &p); > > diff --git a/lib/xe/xe_oa.h b/lib/xe/xe_oa.h > > index 962f9dddcc..7d3d074560 100644 > > --- a/lib/xe/xe_oa.h > > +++ b/lib/xe/xe_oa.h > > @@ -398,6 +398,8 @@ uint64_t intel_xe_perf_read_record_timestamp_raw(const struct intel_xe_perf *per > > const char *intel_xe_perf_read_report_reason(const struct intel_xe_perf *perf, > > const struct intel_xe_perf_record_header *record); > > > > +void intel_xe_oa_prop_to_ext(struct intel_xe_oa_open_prop *properties, > > + struct drm_xe_ext_set_property *extn); > > int intel_xe_perf_ioctl(int fd, enum drm_xe_observation_op op, void *arg); > > void intel_xe_perf_ioctl_err(int fd, enum drm_xe_observation_op op, void *arg, int err); > > > > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c > > index f2c6d53007..0b9c3349cb 100644 > > --- a/tests/intel/xe_oa.c > > +++ b/tests/intel/xe_oa.c > > @@ -22,6 +22,7 @@ > > #include "drm.h" > > #include "igt.h" > > #include "igt_device.h" > > +#include "igt_syncobj.h" > > #include "igt_sysfs.h" > > #include "xe/xe_ioctl.h" > > #include "xe/xe_query.h" > > @@ -4454,6 +4455,98 @@ static void test_mapped_oa_buffer(map_oa_buffer_test_t test_with_fd_open, > > __perf_close(stream_fd); > > } > > > > +static bool find_alt_oa_config(u32 config_id, u32 *alt_config_id) > > +{ > > + struct dirent *entry; > > + int metrics_fd, dir_fd; > > + DIR *metrics_dir; > > + bool ret; > > + > > + metrics_fd = openat(sysfs, "metrics", O_DIRECTORY); > > + igt_assert_lte(0, metrics_fd); > > + > > + metrics_dir = fdopendir(metrics_fd); > > + igt_assert(metrics_dir); > > + > > + while ((entry = readdir(metrics_dir))) { > > + if (entry->d_type != DT_DIR) > > + continue; > > + > > + dir_fd = openat(metrics_fd, entry->d_name, O_RDONLY); > > + ret = __igt_sysfs_get_u32(dir_fd, "id", alt_config_id); > > + close(dir_fd); > > + if (!ret) > > + continue; > > + > > + if (config_id != *alt_config_id) { > > + closedir(metrics_dir); > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > +/** > > + * SUBTEST: syncs-signal > > Here, we could have two sync-signal tests. Say: > syncs-signal-open > syncs-signal-change > > > + * Description: Test OA syncs signal correctly > > + */ > > +static void > > +test_syncs_signal(const struct drm_xe_engine_class_instance *hwe) > > Add a flag here, such as: > test_syncs_signal(const struct drm_xe_engine_class_instance *hwe, bool open_sync) > In this case, syncs-signal-open would use open_sync = true, and syncs-signal-change > would use open_sync = false. > > > +{ > > + struct drm_xe_ext_set_property extn[XE_OA_MAX_SET_PROPERTIES] = {}; > > + struct intel_xe_perf_metric_set *test_set = metric_set(hwe); > > + struct drm_xe_sync sync = { > > + .type = DRM_XE_SYNC_TYPE_SYNCOBJ, > > + .flags = DRM_XE_SYNC_FLAG_SIGNAL, > > + }; > > + uint64_t open_properties[] = { > > + DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0, > > + DRM_XE_OA_PROPERTY_SAMPLE_OA, true, > > + DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set, > > + DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(test_set->perf_oa_format), > > + DRM_XE_OA_PROPERTY_NUM_SYNCS, 1, > > + DRM_XE_OA_PROPERTY_SYNCS, to_user_pointer(&sync), > > + }; > > + struct intel_xe_oa_open_prop open_param = { > > + .num_properties = ARRAY_SIZE(open_properties) / 2, > > + .properties_ptr = to_user_pointer(open_properties), > > + }; > > + uint64_t config_properties[] = { > > + DRM_XE_OA_PROPERTY_OA_METRIC_SET, 0, /* Filled later */ > > + DRM_XE_OA_PROPERTY_NUM_SYNCS, 1, > > + DRM_XE_OA_PROPERTY_SYNCS, to_user_pointer(&sync), > > + }; > > + struct intel_xe_oa_open_prop config_param = { > > + .num_properties = ARRAY_SIZE(config_properties) / 2, > > + .properties_ptr = to_user_pointer(config_properties), > > + }; > > + uint32_t syncobj = syncobj_create(drm_fd, 0); > > + uint32_t alt_config_id; > > + int ret; > > + > > + sync.handle = syncobj; > > + stream_fd = __perf_open(drm_fd, &open_param, false); > > + > > + igt_assert(syncobj_wait(drm_fd, &syncobj, 1, INT64_MAX, 0, NULL)); > > Then, we'd want to separate the two using a conditional: > if (open_sync) { > igt_assert(syncobj_wait(drm_fd, &syncobj, 1, INT64_MAX, 0, NULL)); > goto out; > } Yeah I have similar ideas but wanted to get the kernel series out on the mailing list in a hurry. Will tweak these tests along the lines you have suggested. Thanks. -- Ashutosh > > + > > + /* Change stream configuration */ > > + syncobj_reset(drm_fd, &syncobj, 1); > > + if (!find_alt_oa_config(test_set->perf_oa_metrics_set, &alt_config_id)) > > + goto out; > > + > > + config_properties[1] = alt_config_id; > > + intel_xe_oa_prop_to_ext(&config_param, extn); > > + > > + ret = igt_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_CONFIG, extn); > > + igt_assert_eq(ret, test_set->perf_oa_metrics_set); > > + > > + igt_assert(syncobj_wait(drm_fd, &syncobj, 1, INT64_MAX, 0, NULL)); > > +out: > > + __perf_close(stream_fd); > > + syncobj_destroy(drm_fd, syncobj); > > +} > > + > > static const char *xe_engine_class_name(uint32_t engine_class) > > { > > switch (engine_class) { > > @@ -4702,6 +4795,10 @@ igt_main > > } > > } > > > > + igt_subtest_with_dynamic("syncs-signal") > > + __for_one_render_engine(hwe) > > + test_syncs_signal(hwe); > > + > > igt_fixture { > > /* leave sysctl options in their default state... */ > > write_u64_file("/proc/sys/dev/xe/observation_paranoid", 1); > > -- > > 2.41.0 > > > >