[igt-dev] [PATCH 2/2] i915/perf: Sanity check reports in mapped OA buffer

[Lionel Landwerlin]

Looks good, just one nit and a test suggestion.

On 21/07/2020 04:57, Umesh Nerlige Ramappa wrote:
> For applications that need a faster way to access reports in the OA
> buffer, i915 now provides a way to map the OA buffer to privileged user
> space. Add a test to sanity check reports in the mapped OA buffer.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> ---
>   include/drm-uapi/i915_drm.h |  32 +++++
>   tests/i915/perf.c           | 241 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 273 insertions(+)
>
> diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
> index 2b55af13..f7523d55 100644
> --- a/include/drm-uapi/i915_drm.h
> +++ b/include/drm-uapi/i915_drm.h
> @@ -2048,6 +2048,38 @@ struct drm_i915_perf_open_param {
>    */
>   #define I915_PERF_IOCTL_CONFIG	_IO('i', 0x2)
>   
> +/**
> + * Returns OA buffer properties to be used with mmap.
> + *
> + * This ioctl is available in perf revision 6.
> + */
> +#define I915_PERF_IOCTL_GET_OA_BUFFER_INFO _IO('i', 0x3)
> +
> +/**
> + * OA buffer size and offset.
> + */
> +struct drm_i915_perf_oa_buffer_info {
> +	__u32 size;
> +	__u32 offset;
> +	__u64 reserved[4];
> +};
> +
> +/**
> + * Returns current position of OA buffer head and tail.
> + *
> + * This ioctl is available in perf revision 6.
> + */
> +#define I915_PERF_IOCTL_GET_OA_BUFFER_HEAD_TAIL _IO('i', 0x4)
> +
> +/**
> + * OA buffer head and tail.
> + */
> +struct drm_i915_perf_oa_buffer_head_tail {
> +	__u32 head;
> +	__u32 tail;
> +	__u64 reserved[4];
> +};

We should probably test head/tail values.

Open the stream with a fast enough exponent and verify they loop correctly?

> +
>   /**
>    * Common to all i915 perf records
>    */
> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> index eb38ea12..b7aa1596 100644
> --- a/tests/i915/perf.c
> +++ b/tests/i915/perf.c
> @@ -206,6 +206,7 @@ static struct intel_perf *intel_perf = NULL;
>   static struct intel_perf_metric_set *test_set = NULL;
>   static bool *undefined_a_counters;
>   static uint64_t oa_exp_1_millisec;
> +struct intel_mmio_data mmio_data;
>   
>   static igt_render_copyfunc_t render_copy = NULL;
>   static uint32_t (*read_report_ticks)(uint32_t *report,
> @@ -5011,6 +5012,220 @@ test_whitelisted_registers_userspace_config(void)
>   	i915_perf_remove_config(drm_fd, config_id);
>   }
>   
> +#define OA_BUFFER_DATA(tail, head, oa_buffer_size) \
> +	(((tail) - (head)) & ((oa_buffer_size) - 1))
> +
> +#ifndef MAP_FAILED
> +#define MAP_FAILED ((void *)-1)
> +#endif
> +
> +static uint32_t oa_status_reg(void)
> +{
> +	if (IS_HASWELL(devid))
> +		return intel_register_read(&mmio_data, 0x2346) & 0x7;
> +	else if (IS_GEN12(devid))
> +		return intel_register_read(&mmio_data, 0xdafc) & 0x7;
> +	else
> +		return intel_register_read(&mmio_data, 0x2b08) & 0xf;
> +}
> +
> +static void invalid_map_oa_buffer(void)
> +{
> +	struct drm_i915_perf_oa_buffer_info oa_buffer;
> +	void *oa_vaddr = NULL;
> +
> +	do_ioctl(stream_fd, I915_PERF_IOCTL_GET_OA_BUFFER_INFO, &oa_buffer);
> +
> +	igt_debug("size        = %d\n", oa_buffer.size);
> +	igt_debug("offset      = %x\n", oa_buffer.offset);
> +
> +	igt_assert_eq(oa_buffer.size & (oa_buffer.size - 1), 0);
> +
> +	/* try a couple invalid mmaps */
> +	/* bad offsets */
> +	oa_vaddr = mmap(0, oa_buffer.size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
> +	igt_assert(oa_vaddr == MAP_FAILED);
> +
> +	oa_vaddr = mmap(0, oa_buffer.size, PROT_READ, MAP_PRIVATE, stream_fd, 8192);
> +	igt_assert(oa_vaddr == MAP_FAILED);
> +
> +	oa_vaddr = mmap(0, oa_buffer.size, PROT_READ, MAP_PRIVATE, stream_fd, 11);
> +	igt_assert(oa_vaddr == MAP_FAILED);
> +
> +	/* bad size */
> +	oa_vaddr = mmap(0, oa_buffer.size + 1, PROT_READ, MAP_PRIVATE, stream_fd, oa_buffer.offset);
> +	igt_assert(oa_vaddr == MAP_FAILED);
> +
> +	/* do the right thing */
> +	oa_vaddr = mmap(0, oa_buffer.size, PROT_READ, MAP_PRIVATE, stream_fd, oa_buffer.offset);
> +	igt_assert(oa_vaddr != NULL);

oa_vaddr != NULL && oa_vaddr != MAP_FAILED ;)


> +
> +	munmap(oa_vaddr, oa_buffer.size);
> +}
> +
> +static void *map_oa_buffer(uint32_t *size)
> +{

> ...

> +
>   static unsigned
>   read_i915_module_ref(void)
>   {
> @@ -5179,6 +5394,9 @@ igt_main
>   
>   		render_copy = igt_get_render_copyfunc(devid);
>   		igt_require_f(render_copy, "no render-copy function\n");
> +
> +		intel_register_access_init(&mmio_data, intel_get_pci_device(),
> +					   0, drm_fd);
>   	}
>   
>   	igt_subtest("non-system-wide-paranoid")
> @@ -5346,6 +5564,28 @@ igt_main
>   		test_triggered_oa_reports();
>   	}
>   
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_require(i915_perf_revision(drm_fd) >= 8);
> +		}
> +
> +		igt_describe("Verify mapping of oa buffer");
> +		igt_subtest("map-oa-buffer")
> +			test_mapped_oa_buffer(check_reports_from_mapped_buffer);
> +
> +		igt_describe("Verify invalid mappings of oa buffer");
> +		igt_subtest("invalid-map-oa-buffer")
> +			test_mapped_oa_buffer(invalid_map_oa_buffer);
> +
> +		igt_describe("Verify if non-privileged user can map oa buffer");
> +		igt_subtest("non-privileged-map-oa-buffer")
> +			test_mapped_oa_buffer(test_unprivileged_map_oa_buffer);
> +
> +		igt_describe("Close FD and access oa buffer");
> +		igt_subtest("close-fd-and-access-oa-buffer")
> +			close_fd_and_access_oa_buffer();

I think one additional test that would be helpful is to do the following :


for (i = 0; i < 256; i++) {

   int fd = perf_open();

   void *ptr = mmap(fd);

   close(fd);

}

16Mb * 256 = 4Gb

That way you verify that we're not leaking GGTT space when closing the 
perf fd.

You might want to tweak the noa_wait sysfs value before/after the loop.

This might also only work on !32bits machines with enough memory...


-Lionel


> +	}
> +
>   	igt_fixture {
>   		/* leave sysctl options in their default state... */
>   		write_u64_file("/proc/sys/dev/i915/oa_max_sample_rate", 100000);
> @@ -5354,6 +5594,7 @@ igt_main
>   		if (intel_perf)
>   			intel_perf_free(intel_perf);
>   
> +		intel_register_access_fini(&mmio_data);
>   		close(drm_fd);
>   	}
>   }