[PATCH i-g-t] tests/intel/xe_oa: Add syncs-signal test for OA syncs
Dixit, Ashutosh
ashutosh.dixit at intel.com
Thu Aug 8 20:21:12 UTC 2024
On Thu, 08 Aug 2024 12:45:40 -0700, Cavitt, Jonathan wrote:
>
Hi Jonathan,
> -----Original Message-----
> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Thursday, August 8, 2024 10:37 AM
> To: igt-dev at lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; Souza, Jose <jose.souza at intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin at 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 at 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 at 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
> >
> >
More information about the igt-dev
mailing list