[igt-dev] [PATCH 2/2] i915/perf: Sanity check reports in mapped OA buffer
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Fri Jul 24 23:33:19 UTC 2020
On Tue, Jul 21, 2020 at 09:21:44AM +0300, Lionel Landwerlin wrote:
>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?
Will try this out in the next iteration of patches.
>
>>+
>> /**
>> * 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...
>
Looks like calling close() in the above sequence will not result in a
call to i915_perf_release (because mmap holds a reference to the file).
Based on the latest patch series in the mailing list, if you see
something we can add, please let me know. Note that we block mremap of
an mmap-ped address by setting VM_DONTEXPAND in i915 perf mmap
implementation.
Thanks,
Umesh
>
>-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);
>> }
>> }
>
>
More information about the igt-dev
mailing list