Re: [PATCH i-g-t] tests/intel/xe_oa: Add syncs-signal test for OA syncs

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

On Thu, 08 Aug 2024 12:45:40 -0700, Cavitt, Jonathan wrote:
>

Hi Jonathan,

> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Thursday, August 8, 2024 10:37 AM
> To: igt-dev@lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; Souza, Jose <jose.souza@intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
> 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 <ashutosh.dixit@intel.com>
>
> 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 <jonathan.cavitt@intel.com>
>
> > ---
> >  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
> >
> >