Re: [PATCH i-g-t 10/28] tests/intel/xe_oa: Add first tests

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

On Thu, 20 Jun 2024 16:11:21 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> > +/**
> > + * SUBTEST: xe-ref-count
> > + * Description: Check that an open oa stream holds a reference on the xe module
> > + */
> > +static void
> > +test_xe_ref_count(void)
> > +{
> > +	uint64_t properties[] = {
> > +		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
> > +
> > +		/* Include OA reports in samples */
> > +		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
> > +
> > +		/* OA unit configuration */
> > +		DRM_XE_OA_PROPERTY_OA_METRIC_SET, 0 /* updated below */,
> > +		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(0), /* update below */
> > +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, 0, /* update below */
> > +	};
> > +	struct intel_xe_oa_open_prop param = {
> > +		.num_properties = ARRAY_SIZE(properties) / 2,
> > +		.properties_ptr = to_user_pointer(properties),
> > +	};
> > +	unsigned baseline, ref_count0, ref_count1;
> > +	uint32_t oa_report0[64];
> > +	uint32_t oa_report1[64];
> > +
> > +	/* This should be the first test before the first fixture so no drm_fd
> > +	 * should have been opened so far...
> > +	 */
> > +	igt_assert_eq(drm_fd, -1);
> > +
> > +	baseline = read_xe_module_ref();
> > +	igt_debug("baseline ref count (drm fd closed) = %u\n", baseline);
> > +
> > +	drm_fd = __drm_open_driver(DRIVER_XE);
> > +	if (is_xe_device(drm_fd))
> > +		xe_device_get(drm_fd);
> > +	devid = intel_get_drm_devid(drm_fd);
> > +	sysfs = igt_sysfs_open(drm_fd);
> > +
> > +	/* Note: these global variables are only initialized after calling
> > +	 * init_sys_info()...
> > +	 */
> > +	igt_require(init_sys_info());
> > +	properties[5] = default_test_set->perf_oa_metrics_set;
> > +	properties[7] = __ff(default_test_set->perf_oa_format);
> > +	properties[9] = oa_exp_1_millisec;
> > +
> > +	ref_count0 = read_xe_module_ref();
> > +	igt_debug("initial ref count with drm_fd open = %u\n", ref_count0);
> > +
> > +	stream_fd = __perf_open(drm_fd, &param, false);
> > +        set_fd_flags(stream_fd, O_CLOEXEC);
> > +	ref_count1 = read_xe_module_ref();
> > +	igt_debug("ref count after opening oa stream = %u\n", ref_count1);
> > +
> > +	drm_close_driver(drm_fd);
> > +	close(sysfs);
> > +	drm_fd = -1;
> > +	sysfs = -1;
> > +	ref_count0 = read_xe_module_ref();
> > +	igt_debug("ref count after closing drm fd = %u\n", ref_count0);
> > +
> > +	read_2_oa_reports(default_test_set->perf_oa_format,
> > +			  oa_exp_1_millisec,
> > +			  oa_report0,
> > +			  oa_report1,
> > +			  false); /* not just timer reports */
> > +
> > +	__perf_close(stream_fd);
> > +	ref_count0 = read_xe_module_ref();
> > +	igt_debug("ref count after closing oa stream fd = %u\n", ref_count0);
>
> Looks like the asserts are removed, so we will never know if this failed
> :). I think we should drop the test and create an issue to make it more
> robust.

Unfortunately I don't want to drop the test in this patch, since it will
mean moving several other functions called by this code to other patches
etc. (to make sure this patch continues to compile). It is a non trivial
amount of work, not worth it IMO. So instead, I have added a patch at the
end deleting the "xe-ref-count" subtest.

I have also created an issue to re-add the test back after making it
robust.

I have already merged the IGT library and tools. So it is just the Xe OA
IGT test patches left to merge. So now only the first and the second-last
patches don't have a R-b. You can see these here:

https://patchwork.freedesktop.org/series/130033/#rev5

Thanks.
--
Ashutosh

>
> Ideally we should take a snapshot of the entire /proc/modules file and
> ensure that no new driver has added a reference to xe while we are running
> this test. That way, we know that an assert may fail due to a new driver
> taking up a drm ref or dropping one. If we can attribute the failure to
> such an event, we can re-run the test.