Re: [PATCH i-g-t 5/5] tests/v3d_perfmon: Add global perfmon and counter isolation tests

[Iago Toral <itoral@igalia.com>]

Hi Maíra,

patches 1-4 are:

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

For patch 5, I have some comments below:

El vie, 08-05-2026 a las 09:42 -0300, Maíra Canal escribió:

(...)
> +	igt_describe("Make sure that the global perfmon is tracking
> all jobs consistently.");
> +	igt_subtest("global-perfmon-valid-perfmon") {
> +		struct drm_v3d_perfmon_set_global set_global = { 0
> };
> +		struct v3d_cl_job *job = igt_v3d_noop_job(fd);
> +		uint64_t *values1, *values2;
> +		uint32_t id, out_sync;
> +		uint8_t counters[2];
> +
> +		/*
> +		 * Different V3D versions have different performance
> counters
> +		 * IDs for the same counters.
> +		 */
> +		if (ver >= 71) {
> +			counters[0] = 0; /* cycle-count */
> +			counters[1] = 1; /* core-active */
> +		} else if (ver >= 42) {
> +			counters[0] = V3D_PERFCNT_CYCLE_COUNT;
> +			counters[1] = V3D_PERFCNT_CLE_ACTIVE;
> +		} else
> +			igt_abort_on_f(true, "This V3D version is
> not supported.");
> +
> +		id = igt_v3d_perfmon_create(fd,
> ARRAY_SIZE(counters), counters);
> +
> +		values1 = calloc(ARRAY_SIZE(counters),
> sizeof(*values1));
> +		values2 = calloc(ARRAY_SIZE(counters),
> sizeof(*values2));
> +
> +		set_global.id = id;
> +		do_ioctl(fd, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		igt_v3d_perfmon_get_values(fd, id, values1);
> +
> +		out_sync = syncobj_create(fd, 0);
> +		job->submit->out_sync = out_sync;
> +
> +		/* Submit an arbitrary number of CL jobs to stress
> the perfcnts. */
> +		for (int i = 0; i < 10; i++)
> +			do_ioctl(fd, DRM_IOCTL_V3D_SUBMIT_CL, job-
> >submit);
> +
> +		/* Wait for the last job to complete so counters are
> captured. */
> +		igt_assert(syncobj_wait(fd, &out_sync, 1, INT64_MAX,
> 0, NULL));

If the intent was to wait for all jobs to complete, should we not move
the syncobj_wait into the loop above and reset the syncobj after each
iteration? Otherwise I think this is only really waiting for the first
job's completion. Alternatively, you can set out_sync only for the last
job instead.

> +
> +		igt_v3d_perfmon_get_values(fd, id, values2);
> +
> +		/* As the global perfmon is active, the cycle-count
> must have
> +		 * increased with time.
> +		 */
> +		for (int i = 0; i < ARRAY_SIZE(counters); i++)
> +			igt_assert_lt(values1[i], values2[i]);
> +
> +		set_global.flags = DRM_V3D_PERFMON_CLEAR_GLOBAL;
> +		do_ioctl(fd, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		igt_v3d_perfmon_get_values(fd, id, values1);
> +
> +		/* Submit an arbitrary number of CL jobs to stress
> the perfcnts. */
> +		for (int i = 0; i < 10; i++)
> +			do_ioctl(fd, DRM_IOCTL_V3D_SUBMIT_CL, job-
> >submit);
> +

I think we would also want wait for completion of the jobs here before
retrieving the values below.

> +		igt_v3d_perfmon_get_values(fd, id, values2);
> +
> +		/* As the global perfmon was cleared, the perfmon is
> inactive
> +		 * and it must preserve its values.
> +		 */
> +		for (int i = 0; i < ARRAY_SIZE(counters); i++)
> +			igt_assert_eq(values1[i], values2[i]);
> +
> +		igt_v3d_perfmon_destroy(fd, id);
> +
> +		syncobj_destroy(fd, out_sync);
> +		igt_v3d_free_cl_job(fd, job);
> +
> +		free(values1);
> +		free(values2);
> +	}
> +
> +	igt_describe("Make sure that a global perfmon tracks jobs
> from other file descriptors.");
> +	igt_subtest("global-perfmon-multifd") {
> +		struct drm_v3d_perfmon_set_global set_global = { 0
> },
> +						  set_global2 = { 0
> };
> +		int fd2 = drm_open_driver_render(DRIVER_V3D);
> +		struct v3d_cl_job *job = igt_v3d_noop_job(fd2);
> +		uint32_t id1, id2, out_sync;
> +		uint64_t values1, values2;
> +		uint8_t counter;
> +
> +		/*
> +		 * Different V3D versions have different performance
> counters
> +		 * IDs for the same counters.
> +		 */
> +		if (ver >= 71)
> +			counter = 0; /* cycle-count */
> +		else if (ver >= 42)
> +			counter = V3D_PERFCNT_CYCLE_COUNT;
> +		else
> +			igt_abort_on_f(true, "This V3D version is
> not supported.");
> +
> +		/* Create and set global perfmon from fd. */
> +		id1 = igt_v3d_perfmon_create(fd, 1, &counter);
> +		set_global.id = id1;
> +		do_ioctl(fd, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		igt_v3d_perfmon_get_values(fd, id1, &values1);
> +
> +		/* Submit jobs from the second fd; the global
> perfmon should track them. */
> +		out_sync = syncobj_create(fd2, 0);
> +		job->submit->out_sync = out_sync;
> +		for (int i = 0; i < 10; i++)
> +			do_ioctl(fd2, DRM_IOCTL_V3D_SUBMIT_CL, job-
> >submit);
> +		igt_assert(syncobj_wait(fd2, &out_sync, 1,
> INT64_MAX, 0, NULL));
> +

Same issue here about using the same out_sync on all jobs.

> +		igt_v3d_perfmon_get_values(fd, id1, &values2);
> +		igt_assert_lt(values1, values2);
> +
> +		/* fd2 cannot set its own global perfmon while fd's
> is active. */
> +		id2 = igt_v3d_perfmon_create(fd2, 1, &counter);
> +		set_global2.id = id2;
> +		do_ioctl_err(fd2, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global2, EBUSY);
> +
> +		igt_v3d_perfmon_destroy(fd2, id2);
> +		igt_v3d_perfmon_destroy(fd, id1);
> +
> +		syncobj_destroy(fd2, out_sync);
> +		igt_v3d_free_cl_job(fd2, job);
> +
> +		drm_close_driver(fd2);
> +	}
> +
> +	igt_describe("Make sure closing a file descriptor with an
> active global "
> +		     "perfmon does not crash the driver.");
> +	igt_subtest("global-perfmon-destroy-multifd") {
> +		struct drm_v3d_perfmon_set_global set_global = { 0
> };
> +		struct v3d_cl_job *job = igt_v3d_noop_job(fd);
> +		int fd2 = drm_open_driver(DRIVER_V3D);
> +		uint32_t id, new_id, out_sync;
> +		uint8_t counter = 0;
> +
> +		/* Create and set global perfmon from fd2. */
> +		id = igt_v3d_perfmon_create(fd2, 1, &counter);
> +		set_global.id = id;
> +		do_ioctl(fd2, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		/* Keep HW busy from fd while fd2 is closed. */
> +		out_sync = syncobj_create(fd, 0);
> +		job->submit->out_sync = out_sync;
> +		for (int i = 0; i < 10; i++)
> +			do_ioctl(fd, DRM_IOCTL_V3D_SUBMIT_CL, job-
> >submit);
> +

Same here.


> +		/* Closing fd2 must cleanly stop the active global
> perfmon and
> +		 * clear the global perfmon without crashing.
> +		 */
> +		drm_close_driver(fd2);
> +
> +		igt_assert(syncobj_wait(fd, &out_sync, 1, INT64_MAX,
> 0, NULL));
> +
> +		/* fd can now set a new global perfmon without
> EBUSY. */
> +		new_id = igt_v3d_perfmon_create(fd, 1, &counter);
> +
> +		set_global.id = new_id;
> +		do_ioctl(fd, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		set_global.flags = DRM_V3D_PERFMON_CLEAR_GLOBAL;
> +		do_ioctl(fd, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL,
> &set_global);
> +
> +		igt_v3d_perfmon_destroy(fd, new_id);
> +
> +		syncobj_destroy(fd, out_sync);
> +		igt_v3d_free_cl_job(fd, job);
> +	}
> +

(...)

> +
> +	igt_describe("Make sure a non-perfmon CL job submitted after
> a perfmon CL job "
> +		     "is serialized behind it and does not
> contaminate its counter values.");
> +	igt_subtest("perfmon-counter-isolation-subsequent-cl") {
> +		struct v3d_cl_job *job1 = igt_v3d_noop_job(fd);
> +		struct v3d_cl_job *job2 = igt_v3d_noop_job(fd);
> +		uint64_t id, values, values_after_j2;
> +		uint32_t blocker, out1, out2;
> +		int timeline, fence_fd;
> +		uint8_t counter;
> +
> +		igt_require_sw_sync();
> +
> +		/*
> +		 * Different V3D versions have different performance
> counters IDs
> +		 * for the same counters.
> +		 */
> +		if (ver >= 71)
> +			counter = 0; /* cycle-count */
> +		else if (ver >= 42)
> +			counter = V3D_PERFCNT_CYCLE_COUNT;
> +		else
> +			igt_abort_on_f(true, "This V3D version is
> not supported.");
> +
> +		id = igt_v3d_perfmon_create(fd, 1, &counter);
> +
> +		/*
> +		 * Create an unsignaled fence via sw_sync. job1 will
> wait on
> +		 * this fence and stay blocked until we increment
> the timeline.
> +		 */
> +		timeline = sw_sync_timeline_create();
> +		fence_fd = sw_sync_timeline_create_fence(timeline,
> 1);
> +
> +		blocker = syncobj_create(fd, 0);
> +		syncobj_import_sync_file(fd, blocker, fence_fd);
> +		close(fence_fd);
> +
> +		out1 = syncobj_create(fd, 0);
> +		out2 = syncobj_create(fd, 0);
> +
> +		/* job1: BIN job is blocked until we signal the
> blocker. */
> +		job1->submit->in_sync_bcl = blocker;
> +		job1->submit->perfmon_id = id;
> +		job1->submit->out_sync = out1;
> +		do_ioctl(fd, DRM_IOCTL_V3D_SUBMIT_CL, job1->submit);
> +
> +		/* job2: no perfmon, must wait for job1 due to job
> serialization. */
> +		job2->submit->out_sync = out2;
> +		do_ioctl(fd, DRM_IOCTL_V3D_SUBMIT_CL, job2->submit);
> +
> +		/* job2 is serialized behind job1, so it cannot have
> completed yet. */
> +		igt_assert_eq(syncobj_wait_err(fd, &out2, 1, 0, 0),
> -ETIME);
> +
> +		/* While job1 has not started, the perfmon must
> still read zero. */
> +		igt_v3d_perfmon_get_values(fd, id, &values);
> +		igt_assert_eq(values, 0);
> +
> +		/* Release the blocker. */
> +		sw_sync_timeline_inc(timeline, 1);
> +
> +		igt_assert(syncobj_wait(fd, &out1, 1, INT64_MAX, 0,
> NULL));
> +		igt_assert(syncobj_wait(fd, &out2, 1, INT64_MAX, 0,
> NULL));

I think that for the tests below to make sense you would want to move
the second wait after checking values for the first time....

> +
> +		/* job1 must have contributed cycles to the perfmon.
> */
> +		igt_v3d_perfmon_get_values(fd, id, &values);
> +		igt_assert_lt(0, values);

Otherwise, this is really checking potential contributions from both
jobs, not just job1.

> +
> +		/* job2 doesn't have a perfmon attached, so a second
> read must
> +		 * return the same value.
> +		 */
> +		igt_v3d_perfmon_get_values(fd, id,
> &values_after_j2);
> +		igt_assert_eq(values, values_after_j2);

And this is not really checking anything about job2, so it is always
going to give the same values.

> +
> +		close(timeline);
> +		syncobj_destroy(fd, blocker);
> +		syncobj_destroy(fd, out1);
> +		syncobj_destroy(fd, out2);
> +
> +		igt_v3d_perfmon_destroy(fd, id);
> +
> +		igt_v3d_free_cl_job(fd, job1);
> +		igt_v3d_free_cl_job(fd, job2);
> +	}
> +
>  	igt_fixture()
>  		drm_close_driver(fd);
>  }