* [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers
@ 2018-02-26 17:59 Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests Lionel Landwerlin
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2018-02-26 17:59 UTC (permalink / raw)
To: igt-dev
For testing only, to be updated with proper commit id from drm-next.
---
include/drm-uapi/i915_drm.h | 146 +++++++++++++++++++++++++++++++++++++++++++-
lib/igt_perf.h | 7 ---
2 files changed, 145 insertions(+), 8 deletions(-)
diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
index 7f28eea4..9dfebbbe 100644
--- a/include/drm-uapi/i915_drm.h
+++ b/include/drm-uapi/i915_drm.h
@@ -102,6 +102,46 @@ enum drm_i915_gem_engine_class {
I915_ENGINE_CLASS_INVALID = -1
};
+/**
+ * DOC: perf_events exposed by i915 through /sys/bus/event_sources/drivers/i915
+ *
+ */
+
+enum drm_i915_pmu_engine_sample {
+ I915_SAMPLE_BUSY = 0,
+ I915_SAMPLE_WAIT = 1,
+ I915_SAMPLE_SEMA = 2
+};
+
+#define I915_PMU_SAMPLE_BITS (4)
+#define I915_PMU_SAMPLE_MASK (0xf)
+#define I915_PMU_SAMPLE_INSTANCE_BITS (8)
+#define I915_PMU_CLASS_SHIFT \
+ (I915_PMU_SAMPLE_BITS + I915_PMU_SAMPLE_INSTANCE_BITS)
+
+#define __I915_PMU_ENGINE(class, instance, sample) \
+ ((class) << I915_PMU_CLASS_SHIFT | \
+ (instance) << I915_PMU_SAMPLE_BITS | \
+ (sample))
+
+#define I915_PMU_ENGINE_BUSY(class, instance) \
+ __I915_PMU_ENGINE(class, instance, I915_SAMPLE_BUSY)
+
+#define I915_PMU_ENGINE_WAIT(class, instance) \
+ __I915_PMU_ENGINE(class, instance, I915_SAMPLE_WAIT)
+
+#define I915_PMU_ENGINE_SEMA(class, instance) \
+ __I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
+
+#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
+
+#define I915_PMU_ACTUAL_FREQUENCY __I915_PMU_OTHER(0)
+#define I915_PMU_REQUESTED_FREQUENCY __I915_PMU_OTHER(1)
+#define I915_PMU_INTERRUPTS __I915_PMU_OTHER(2)
+#define I915_PMU_RC6_RESIDENCY __I915_PMU_OTHER(3)
+
+#define I915_PMU_LAST I915_PMU_RC6_RESIDENCY
+
/* Each region is a minimum of 16k, and there are at most 255 of them.
*/
#define I915_NR_TEX_REGIONS 255 /* table size 2k - maximum due to use
@@ -278,6 +318,7 @@ typedef struct _drm_i915_sarea {
#define DRM_I915_PERF_OPEN 0x36
#define DRM_I915_PERF_ADD_CONFIG 0x37
#define DRM_I915_PERF_REMOVE_CONFIG 0x38
+#define DRM_I915_QUERY 0x39
#define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
#define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -335,6 +376,7 @@ typedef struct _drm_i915_sarea {
#define DRM_IOCTL_I915_PERF_OPEN DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
#define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
#define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
+#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
/* Allow drivers to submit batchbuffers directly to hardware, relying
* on the security mechanisms provided by hardware.
@@ -1318,7 +1360,9 @@ struct drm_intel_overlay_attrs {
* active on a given plane.
*/
-#define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */
+#define I915_SET_COLORKEY_NONE (1<<0) /* Deprecated. Instead set
+ * flags==0 to disable colorkeying.
+ */
#define I915_SET_COLORKEY_DESTINATION (1<<1)
#define I915_SET_COLORKEY_SOURCE (1<<2)
struct drm_intel_sprite_colorkey {
@@ -1573,6 +1617,106 @@ struct drm_i915_perf_oa_config {
__u64 flex_regs_ptr;
};
+struct drm_i915_query_item {
+ __u64 query_id;
+#define DRM_I915_QUERY_TOPOLOGY_INFO 1
+
+ /*
+ * When set to zero by userspace, this is filled with the size of the
+ * data to be written at the data_ptr pointer. The kernel set this
+ * value to a negative value to signal an error on a particular query
+ * item.
+ */
+ __s32 length;
+
+ /*
+ * Unused for now.
+ */
+ __u32 flags;
+
+ /*
+ * Data will be written at the location pointed by data_ptr when the
+ * value of length matches the length of the data to be written by the
+ * kernel.
+ */
+ __u64 data_ptr;
+};
+
+struct drm_i915_query {
+ __u32 num_items;
+
+ /*
+ * Unused for now.
+ */
+ __u32 flags;
+
+ /*
+ * This point to an array of num_items drm_i915_query_item structures.
+ */
+ __u64 items_ptr;
+};
+
+/*
+ * Data written by the kernel with query DRM_I915_QUERY_TOPOLOGY_INFO :
+ *
+ * data: contains the 3 pieces of information :
+ *
+ * - the slice mask with one bit per slice telling whether a slice is
+ * available. The availability of slice X can be queried with the following
+ * formula :
+ *
+ * (data[X / 8] >> (X % 8)) & 1
+ *
+ * - the subslice mask for each slice with one bit per subslice telling
+ * whether a subslice is available. The availability of subslice Y in slice
+ * X can be queried with the following formula :
+ *
+ * (data[subslice_offset +
+ * X * subslice_stride +
+ * Y / 8] >> (Y % 8)) & 1
+ *
+ * - the EU mask for each subslice in each slice with one bit per EU telling
+ * whether an EU is available. The availability of EU Z in subslice Y in
+ * slice X can be queried with the following formula :
+ *
+ * (data[eu_offset +
+ * (X * max_subslices Y) * eu_stride +
+ * Z / 8] >> (Z % 8)) & 1
+ */
+struct drm_i915_query_topology_info {
+ /*
+ * Unused for now.
+ */
+ __u16 flags;
+
+ __u16 max_slices;
+ __u16 max_subslices;
+ __u16 max_eus_per_subslice;
+
+ /*
+ * Offset in data[] at which the subslice masks are stored.
+ */
+ __u16 subslice_offset;
+
+ /*
+ * Stride at which each of the subslice masks for each slice are
+ * stored.
+ */
+ __u16 subslice_stride;
+
+ /*
+ * Offset in data[] at which the EU masks are stored.
+ */
+ __u16 eu_offset;
+
+ /*
+ * Stride at which each of the EU masks for each subslice are stored.
+ */
+ __u16 eu_stride;
+
+ __u8 data[];
+};
+
#if defined(__cplusplus)
}
#endif
diff --git a/lib/igt_perf.h b/lib/igt_perf.h
index 7b66fc58..105b8cd9 100644
--- a/lib/igt_perf.h
+++ b/lib/igt_perf.h
@@ -31,13 +31,6 @@
#include "igt_gt.h"
-enum drm_i915_pmu_engine_sample {
- I915_SAMPLE_BUSY = 0,
- I915_SAMPLE_WAIT = 1,
- I915_SAMPLE_SEMA = 2,
- I915_ENGINE_SAMPLE_MAX /* non-ABI */
-};
-
#define I915_PMU_SAMPLE_BITS (4)
#define I915_PMU_SAMPLE_MASK (0xf)
#define I915_PMU_SAMPLE_INSTANCE_BITS (8)
--
2.16.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-02-26 17:59 [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers Lionel Landwerlin
@ 2018-02-26 17:59 ` Lionel Landwerlin
2018-03-08 11:22 ` Tvrtko Ursulin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels Lionel Landwerlin
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Lionel Landwerlin @ 2018-02-26 17:59 UTC (permalink / raw)
To: igt-dev
v2: Complete invalid cases (Chris)
Some styling (to_user_pointer, etc...) (Chris)
New error check, through item.length (Chris)
v3: Update for new uAPI iteration (Lionel)
v4: Return errno from a single point (Chris)
Poising checks (Chris)
v5: Add more debug traces (Lionel)
Update uAPI (Joonas/Lionel)
Make sure Haswell is tested (Lionel)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
tests/Makefile.sources | 1 +
tests/i915_query.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++
tests/meson.build | 1 +
3 files changed, 316 insertions(+)
create mode 100644 tests/i915_query.c
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 23f859be..b4c8a913 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -168,6 +168,7 @@ TESTS_progs = \
gen3_render_tiledy_blits \
gen7_forcewake_mt \
gvt_basic \
+ i915_query \
kms_3d \
kms_addfb_basic \
kms_atomic \
diff --git a/tests/i915_query.c b/tests/i915_query.c
new file mode 100644
index 00000000..21621b78
--- /dev/null
+++ b/tests/i915_query.c
@@ -0,0 +1,314 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+
+#include <limits.h>
+
+IGT_TEST_DESCRIPTION("Testing the i915 query uAPI.");
+
+static int
+__i915_query(int fd, struct drm_i915_query *q)
+{
+ if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+ return -errno;
+ return 0;
+}
+
+static int
+__i915_query_item(int fd, struct drm_i915_query_item *items, uint32_t n_items)
+{
+ struct drm_i915_query q = {
+ .num_items = n_items,
+ .items_ptr = to_user_pointer(items),
+ };
+ return __i915_query(fd, &q);
+}
+
+#define i915_query_item(fd, items, n_items) do { \
+ igt_assert_eq(__i915_query_item(fd, items, n_items), 0); \
+ errno = 0; \
+ } while (0)
+#define i915_query_item_err(fd, items, n_items, err) do { \
+ igt_assert_eq(__i915_query_item(fd, items, n_items), -err); \
+ } while (0)
+
+static bool has_query_supports(int fd)
+{
+ struct drm_i915_query query = {};
+
+ return __i915_query(fd, &query) == 0;
+}
+
+static void test_query_garbage(int fd)
+{
+ struct drm_i915_query_item items[2];
+ struct drm_i915_query_item *items_ptr;
+ int i, len, ret;
+
+ i915_query_item_err(fd, (void *) ULONG_MAX, 1, EFAULT);
+
+ i915_query_item_err(fd, (void *) 0, 1, EFAULT);
+
+ memset(items, 0, sizeof(items));
+ i915_query_item_err(fd, items, 1, EINVAL);
+
+ memset(items, 0, sizeof(items));
+ items[0].query_id = ULONG_MAX;
+ items[1].query_id = ULONG_MAX - 2;
+ i915_query_item(fd, items, 2);
+ igt_assert_eq(items[0].length, -EINVAL);
+ igt_assert_eq(items[1].length, -EINVAL);
+
+ memset(items, 0, sizeof(items));
+ items[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ items[1].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ items[1].length = sizeof(struct drm_i915_query_topology_info) - 1;
+ i915_query_item(fd, items, 2);
+ igt_assert_lte(0, items[0].length);
+ igt_assert_eq(items[1].length, -EINVAL);
+
+ items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
+ items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ i915_query_item(fd, items_ptr, 1);
+ igt_assert(items_ptr[0].length >= sizeof(struct drm_i915_query_topology_info));
+ munmap(items_ptr, 4096);
+ i915_query_item_err(fd, items_ptr, 1, EFAULT);
+
+ len = sizeof(struct drm_i915_query_item) * 10;
+ items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
+ for (i = 0; i < 10; i++)
+ items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ ret = __i915_query_item(fd, items_ptr,
+ INT_MAX / sizeof(struct drm_i915_query_item) + 1);
+ igt_assert(ret == -EFAULT || ret == -EINVAL);
+ munmap(items_ptr, len);
+}
+
+static bool query_topology_supported(int fd)
+{
+ struct drm_i915_query_item item = {
+ .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
+ };
+
+ return __i915_query_item(fd, &item, 1) == 0 && item.length > 0;
+}
+
+static void test_query_topology_pre_gen8(int fd)
+{
+ struct drm_i915_query_item item = {
+ .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
+ };
+
+ i915_query_item(fd, &item, 1);
+ igt_assert_eq(item.length, -ENODEV);
+}
+
+#define DIV_ROUND_UP(val, div) (ALIGN(val, div) / div)
+
+static bool
+slice_available(const struct drm_i915_query_topology_info *topo_info,
+ int s)
+{
+ return (topo_info->data[s / 8] >> (s % 8)) & 1;
+}
+
+static bool
+subslice_available(const struct drm_i915_query_topology_info *topo_info,
+ int s, int ss)
+{
+ return (topo_info->data[topo_info->subslice_offset +
+ s * topo_info->subslice_stride +
+ ss / 8] >> (ss % 8)) & 1;
+}
+
+static bool
+eu_available(const struct drm_i915_query_topology_info *topo_info,
+ int s, int ss, int eu)
+{
+ return (topo_info->data[topo_info->eu_offset +
+ (s * topo_info->max_subslices + ss) * topo_info->eu_stride +
+ eu / 8] >> (eu % 8)) & 1;
+}
+
+static void
+test_query_topology_coherent_slice_mask(int fd)
+{
+ struct drm_i915_query_item item;
+ uint8_t *_topo_info;
+ struct drm_i915_query_topology_info *topo_info;
+ drm_i915_getparam_t gp;
+ int slice_mask, subslice_mask;
+ int s, topology_slices, topology_subslices_slice0;
+
+ gp.param = I915_PARAM_SLICE_MASK;
+ gp.value = &slice_mask;
+ igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
+
+ gp.param = I915_PARAM_SUBSLICE_MASK;
+ gp.value = &subslice_mask;
+ igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
+
+ /* Slices */
+ memset(&item, 0, sizeof(item));
+ item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ i915_query_item(fd, &item, 1);
+ igt_assert(item.length > 0);
+
+ _topo_info = malloc(3 * item.length);
+ memset(_topo_info, 0xff, 3 * item.length);
+ topo_info = (struct drm_i915_query_topology_info *) (_topo_info + item.length);
+ memset(topo_info, 0, item.length);
+ igt_assert(topo_info);
+
+ item.data_ptr = to_user_pointer(topo_info);
+ i915_query_item(fd, &item, 1);
+ igt_assert(item.length > 0);
+
+ topology_slices = 0;
+ for (s = 0; s < topo_info->max_slices; s++) {
+ if (slice_available(topo_info, s))
+ topology_slices |= 1UL << s;
+ }
+
+ igt_debug("slice mask getparam=0x%x / query=0x%x\n",
+ slice_mask, topology_slices);
+
+ /* These 2 should always match. */
+ igt_assert_eq_u32(slice_mask, topology_slices);
+
+ topology_subslices_slice0 = 0;
+ for (s = 0; s < topo_info->max_subslices; s++) {
+ if (subslice_available(topo_info, 0, s))
+ topology_subslices_slice0 |= 1UL << s;
+ }
+
+ igt_debug("subslice mask getparam=0x%x / query=0x%x\n",
+ subslice_mask, topology_subslices_slice0);
+
+ /* I915_PARAM_SUBSLICE_MASK returns the value for slice0, we
+ * should match the values for the first slice of the
+ * topology.
+ */
+ igt_assert_eq_u32(subslice_mask, topology_subslices_slice0);
+
+ for (s = 0; s < item.length; s++) {
+ igt_assert_eq(_topo_info[s], 0xff);
+ igt_assert_eq(_topo_info[2 * item.length + s], 0xff);
+ }
+
+ free(_topo_info);
+}
+
+static void
+test_query_topology_matches_eu_total(int fd)
+{
+ struct drm_i915_query_item item;
+ struct drm_i915_query_topology_info *topo_info;
+ drm_i915_getparam_t gp;
+ int n_eus, n_eus_topology, s;
+
+ gp.param = I915_PARAM_EU_TOTAL;
+ gp.value = &n_eus;
+ do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+ igt_debug("n_eus=%i\n", n_eus);
+
+ memset(&item, 0, sizeof(item));
+ item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
+ i915_query_item(fd, &item, 1);
+
+ topo_info = (struct drm_i915_query_topology_info *) calloc(1, item.length);
+
+ item.data_ptr = to_user_pointer(topo_info);
+ i915_query_item(fd, &item, 1);
+
+ igt_debug("max_slices=%hu max_subslices=%hu max_eus_per_subslice=%hu\n",
+ topo_info->max_slices, topo_info->max_subslices,
+ topo_info->max_eus_per_subslice);
+ igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
+ topo_info->subslice_offset, topo_info->subslice_stride);
+ igt_debug(" eu_offset=%hu eu_stride=%hu\n",
+ topo_info->eu_offset, topo_info->eu_stride);
+
+ n_eus_topology = 0;
+ for (s = 0; s < topo_info->max_slices; s++) {
+ int ss;
+
+ igt_debug("slice%i:\n", s);
+
+ for (ss = 0; ss < topo_info->max_subslices; ss++) {
+ int eu, n_subslice_eus = 0;
+
+ igt_debug("\tsubslice: %i\n", ss);
+
+ igt_debug("\t\teu_mask: 0b");
+ for (eu = 0; eu < topo_info->max_eus_per_subslice; eu++) {
+ uint8_t val = eu_available(topo_info, s, ss,
+ topo_info->max_eus_per_subslice - 1 - eu);
+ igt_debug("%hhi", val);
+ n_subslice_eus += __builtin_popcount(val);
+ n_eus_topology += __builtin_popcount(val);
+ }
+ igt_debug(" (%i)\n", n_subslice_eus);
+ }
+ }
+
+ igt_assert(n_eus_topology == n_eus);
+}
+
+igt_main
+{
+ int fd = -1;
+ int devid;
+
+ igt_fixture {
+ fd = drm_open_driver(DRIVER_INTEL);
+ igt_require(has_query_supports(fd));
+ devid = intel_get_drm_devid(fd);
+ }
+
+ igt_subtest("query-garbage")
+ test_query_garbage(fd);
+
+ igt_subtest("query-topology-pre-gen8") {
+ igt_require(intel_gen(devid) < 8 && !IS_HASWELL(devid));
+ igt_require(query_topology_supported(fd));
+ test_query_topology_pre_gen8(fd);
+ }
+
+ igt_subtest("query-topology-coherent-slice-mask") {
+ igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
+ igt_require(query_topology_supported(fd));
+ test_query_topology_coherent_slice_mask(fd);
+ }
+
+ igt_subtest("query-topology-matches-eu-total") {
+ igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
+ igt_require(query_topology_supported(fd));
+ test_query_topology_matches_eu_total(fd);
+ }
+
+ igt_fixture {
+ close(fd);
+ }
+}
diff --git a/tests/meson.build b/tests/meson.build
index 2a1e6f19..a805011e 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -144,6 +144,7 @@ test_progs = [
'gen3_render_tiledy_blits',
'gen7_forcewake_mt',
'gvt_basic',
+ 'i915_query',
'kms_3d',
'kms_addfb_basic',
'kms_atomic',
--
2.16.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels
2018-02-26 17:59 [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests Lionel Landwerlin
@ 2018-02-26 17:59 ` Lionel Landwerlin
2018-03-07 19:14 ` Chris Wilson
2018-02-26 18:23 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers Patchwork
2018-02-26 22:28 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
3 siblings, 1 reply; 13+ messages in thread
From: Lionel Landwerlin @ 2018-02-26 17:59 UTC (permalink / raw)
To: igt-dev
We introduced a subslice mask storage per slice in newer kernels
(because of the possibility of asymmetry). As a result the debugfs
output has changed a bit.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
tests/pm_sseu.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tests/pm_sseu.c b/tests/pm_sseu.c
index 41e1c9fa..abd89d28 100644
--- a/tests/pm_sseu.c
+++ b/tests/pm_sseu.c
@@ -104,6 +104,14 @@ dbg_get_status_section(const char *title, char **first, char **last)
*last = pos - 1;
}
+static bool
+dbg_has_line(const char *first, const char *last, const char *name)
+{
+ char *pos = strstr(first, name);
+
+ return pos != NULL && pos < last;
+}
+
static int
dbg_get_int(const char *first, const char *last, const char *name)
{
@@ -161,8 +169,13 @@ dbg_get_status(struct status *stat)
dbg_get_int(first, last, "Available Slice Total:");
stat->info.subslice_total =
dbg_get_int(first, last, "Available Subslice Total:");
- stat->info.subslice_per =
- dbg_get_int(first, last, "Available Subslice Per Slice:");
+ if (dbg_has_line(first, last, "Available Subslice Per Slice:")) {
+ stat->info.subslice_per =
+ dbg_get_int(first, last, "Available Subslice Per Slice:");
+ } else {
+ stat->info.subslice_per =
+ dbg_get_int(first, last, "Available Slice0 subslices:");
+ }
stat->info.eu_total =
dbg_get_int(first, last, "Available EU Total:");
stat->info.eu_per =
@@ -182,8 +195,13 @@ dbg_get_status(struct status *stat)
dbg_get_int(first, last, "Enabled Slice Total:");
stat->hw.subslice_total =
dbg_get_int(first, last, "Enabled Subslice Total:");
- stat->hw.subslice_per =
- dbg_get_int(first, last, "Enabled Subslice Per Slice:");
+ if (dbg_has_line(first, last, "Enabled Subslice Per Slice:")) {
+ stat->hw.subslice_per =
+ dbg_get_int(first, last, "Enabled Subslice Per Slice:");
+ } else {
+ stat->hw.subslice_per =
+ dbg_get_int(first, last, "Enabled Slice0 subslices:");
+ }
stat->hw.eu_total =
dbg_get_int(first, last, "Enabled EU Total:");
stat->hw.eu_per =
--
2.16.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers
2018-02-26 17:59 [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels Lionel Landwerlin
@ 2018-02-26 18:23 ` Patchwork
2018-02-26 22:28 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
3 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-02-26 18:23 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers
URL : https://patchwork.freedesktop.org/series/38982/
State : success
== Summary ==
IGT patchset tested on top of latest successful build
dd82bdfa86f2086858e2a4bb554aad08a212604e igt/kms_force_connector_basic: Clear any previous connector override
with latest DRM-Tip kernel build CI_DRM_3834
3a86cab57850 drm-tip: 2018y-02m-26d-15h-41m-02s UTC integration manifest
Testlist changes:
+igt@i915_query@query-garbage
+igt@i915_query@query-topology-coherent-slice-mask
+igt@i915_query@query-topology-matches-eu-total
+igt@i915_query@query-topology-pre-gen8
---- Known issues:
Test debugfs_test:
Subgroup read_all_entries:
incomplete -> PASS (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:419s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:379s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:489s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:285s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:478s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:482s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:469s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:456s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:390s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:559s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:580s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:410s
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:289s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:509s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:387s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:407s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:444s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:415s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:448s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:491s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:493s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:585s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:426s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:504s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:520s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:495s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:477s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:406s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:527s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:392s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1008/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers
2018-02-26 17:59 [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers Lionel Landwerlin
` (2 preceding siblings ...)
2018-02-26 18:23 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers Patchwork
@ 2018-02-26 22:28 ` Patchwork
3 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-02-26 22:28 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers
URL : https://patchwork.freedesktop.org/series/38982/
State : success
== Summary ==
---- Known issues:
Test kms_flip:
Subgroup dpms-vs-vblank-race-interruptible:
fail -> PASS (shard-hsw) fdo#103060
Subgroup plain-flip-fb-recreate-interruptible:
fail -> PASS (shard-hsw) fdo#100368
Test kms_flip_tiling:
Subgroup flip-yf-tiled:
fail -> PASS (shard-apl) fdo#103822
Test kms_rotation_crc:
Subgroup primary-rotation-180:
pass -> FAIL (shard-snb) fdo#103925 +1
Test perf:
Subgroup oa-exponents:
pass -> INCOMPLETE (shard-apl) fdo#102254
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
shard-apl total:3439 pass:1811 dwarn:1 dfail:0 fail:7 skip:1618 time:12007s
shard-hsw total:3464 pass:1767 dwarn:1 dfail:0 fail:1 skip:1694 time:11872s
shard-snb total:3464 pass:1358 dwarn:1 dfail:0 fail:2 skip:2103 time:6690s
Blacklisted hosts:
shard-kbl total:3447 pass:1935 dwarn:1 dfail:0 fail:7 skip:1503 time:9282s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1008/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels Lionel Landwerlin
@ 2018-03-07 19:14 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-03-07 19:14 UTC (permalink / raw)
To: Lionel Landwerlin, igt-dev
Quoting Lionel Landwerlin (2018-02-26 17:59:20)
> We introduced a subslice mask storage per slice in newer kernels
> (because of the possibility of asymmetry). As a result the debugfs
> output has changed a bit.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
> tests/pm_sseu.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/tests/pm_sseu.c b/tests/pm_sseu.c
> index 41e1c9fa..abd89d28 100644
> --- a/tests/pm_sseu.c
> +++ b/tests/pm_sseu.c
> @@ -104,6 +104,14 @@ dbg_get_status_section(const char *title, char **first, char **last)
> *last = pos - 1;
> }
>
> +static bool
> +dbg_has_line(const char *first, const char *last, const char *name)
> +{
> + char *pos = strstr(first, name);
> +
> + return pos != NULL && pos < last;
> +}
> +
> static int
> dbg_get_int(const char *first, const char *last, const char *name)
> {
> @@ -161,8 +169,13 @@ dbg_get_status(struct status *stat)
> dbg_get_int(first, last, "Available Slice Total:");
> stat->info.subslice_total =
> dbg_get_int(first, last, "Available Subslice Total:");
> - stat->info.subslice_per =
> - dbg_get_int(first, last, "Available Subslice Per Slice:");
> + if (dbg_has_line(first, last, "Available Subslice Per Slice:")) {
> + stat->info.subslice_per =
> + dbg_get_int(first, last, "Available Subslice Per Slice:");
> + } else {
> + stat->info.subslice_per =
> + dbg_get_int(first, last, "Available Slice0 subslices:");
Ok, matches the proposed patch.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
I didn't check why we don't care about the rest of the future array; I
assume that's a future problem.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests Lionel Landwerlin
@ 2018-03-08 11:22 ` Tvrtko Ursulin
2018-03-08 14:31 ` Lionel Landwerlin
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-08 11:22 UTC (permalink / raw)
To: Lionel Landwerlin, igt-dev
Commit message is missing.
On 26/02/2018 17:59, Lionel Landwerlin wrote:
> v2: Complete invalid cases (Chris)
> Some styling (to_user_pointer, etc...) (Chris)
> New error check, through item.length (Chris)
>
> v3: Update for new uAPI iteration (Lionel)
>
> v4: Return errno from a single point (Chris)
> Poising checks (Chris)
>
> v5: Add more debug traces (Lionel)
> Update uAPI (Joonas/Lionel)
> Make sure Haswell is tested (Lionel)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
> tests/Makefile.sources | 1 +
> tests/i915_query.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 3 files changed, 316 insertions(+)
> create mode 100644 tests/i915_query.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 23f859be..b4c8a913 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -168,6 +168,7 @@ TESTS_progs = \
> gen3_render_tiledy_blits \
> gen7_forcewake_mt \
> gvt_basic \
> + i915_query \
Interesting question how we want to name this test. We don't have any
i915_ prefix tests, but for instance there is drv_getparams_basic,
suggesting this could be called drv_query_ioctl or something?
Open for discussion I guess.
> kms_3d \
> kms_addfb_basic \
> kms_atomic \
> diff --git a/tests/i915_query.c b/tests/i915_query.c
> new file mode 100644
> index 00000000..21621b78
> --- /dev/null
> +++ b/tests/i915_query.c
> @@ -0,0 +1,314 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +
> +#include <limits.h>
> +
> +IGT_TEST_DESCRIPTION("Testing the i915 query uAPI.");
> +
> +static int
> +__i915_query(int fd, struct drm_i915_query *q)
> +{
> + if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
> + return -errno;
> + return 0;
> +}
> +
> +static int
> +__i915_query_item(int fd, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> + struct drm_i915_query q = {
> + .num_items = n_items,
> + .items_ptr = to_user_pointer(items),
> + };
> + return __i915_query(fd, &q);
> +}
> +
> +#define i915_query_item(fd, items, n_items) do { \
> + igt_assert_eq(__i915_query_item(fd, items, n_items), 0); \
> + errno = 0; \
> + } while (0)
> +#define i915_query_item_err(fd, items, n_items, err) do { \
> + igt_assert_eq(__i915_query_item(fd, items, n_items), -err); \
> + } while (0)
> +
Suggest plural i915_query_items for the above.
> +static bool has_query_supports(int fd)
> +{
> + struct drm_i915_query query = {};
> +
> + return __i915_query(fd, &query) == 0;
> +}
> +
> +static void test_query_garbage(int fd)
> +{
> + struct drm_i915_query_item items[2];
> + struct drm_i915_query_item *items_ptr;
> + int i, len, ret;
> +
> + i915_query_item_err(fd, (void *) ULONG_MAX, 1, EFAULT);
> +
> + i915_query_item_err(fd, (void *) 0, 1, EFAULT);
Could add comments throughout to explain what is being tested.
Test for non-zero flags field? Both in the query and query item.
> +
> + memset(items, 0, sizeof(items));
> + i915_query_item_err(fd, items, 1, EINVAL);
> +
> + memset(items, 0, sizeof(items));
> + items[0].query_id = ULONG_MAX;
> + items[1].query_id = ULONG_MAX - 2;
What is special about ULONG_MAX - 2 versus ULONG_MAX? And not ULONG_MAX
- 1, or some other number?
> + i915_query_item(fd, items, 2);
> + igt_assert_eq(items[0].length, -EINVAL);
> + igt_assert_eq(items[1].length, -EINVAL);
> +
> + memset(items, 0, sizeof(items));
> + items[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> + items[1].query_id = DRM_I915_QUERY_TOPOLOGY_INFO; > + items[1].length = sizeof(struct drm_i915_query_topology_info) - 1;
> + i915_query_item(fd, items, 2);
> + igt_assert_lte(0, items[0].length);
> + igt_assert_eq(items[1].length, -EINVAL);
Tricky one - ideally we would want to split testing of the query API
from specific queries, but then we don't have any queries to test with..
can't split it then.
> +
> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> + i915_query_item(fd, items_ptr, 1);
> + igt_assert(items_ptr[0].length >= sizeof(struct drm_i915_query_topology_info));
> + munmap(items_ptr, 4096);
> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
Another good test would be passing in a read only mapping and checking
for EFAULT when length writeback fails.
> +
> + len = sizeof(struct drm_i915_query_item) * 10;
> + items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
> + for (i = 0; i < 10; i++)
> + items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> + ret = __i915_query_item(fd, items_ptr,
> + INT_MAX / sizeof(struct drm_i915_query_item) + 1);
> + igt_assert(ret == -EFAULT || ret == -EINVAL);
What is this testing? Ten queries, all write only so will EFAULT. But
then you pass in nitems = INT_MAX / sizeof.. which is what exactly? I
don't get it.
And assert cannot be either this or that, it has to know explicitly or
split into two tests or something.
> + munmap(items_ptr, len);
> +}
> +
> +static bool query_topology_supported(int fd)
> +{
> + struct drm_i915_query_item item = {
> + .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
> + };
> +
> + return __i915_query_item(fd, &item, 1) == 0 && item.length > 0;
> +}
> +
> +static void test_query_topology_pre_gen8(int fd)
> +{
> + struct drm_i915_query_item item = {
> + .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
> + };
> +
> + i915_query_item(fd, &item, 1);
> + igt_assert_eq(item.length, -ENODEV);
> +}
> +
> +#define DIV_ROUND_UP(val, div) (ALIGN(val, div) / div)
> +
> +static bool
> +slice_available(const struct drm_i915_query_topology_info *topo_info,
> + int s)
> +{
> + return (topo_info->data[s / 8] >> (s % 8)) & 1;
> +}
> +
> +static bool
> +subslice_available(const struct drm_i915_query_topology_info *topo_info,
> + int s, int ss)
> +{
> + return (topo_info->data[topo_info->subslice_offset +
> + s * topo_info->subslice_stride +
> + ss / 8] >> (ss % 8)) & 1;
> +}
> +
> +static bool
> +eu_available(const struct drm_i915_query_topology_info *topo_info,
> + int s, int ss, int eu)
> +{
> + return (topo_info->data[topo_info->eu_offset +
> + (s * topo_info->max_subslices + ss) * topo_info->eu_stride +
> + eu / 8] >> (eu % 8)) & 1;
> +}
> +
> +static void
> +test_query_topology_coherent_slice_mask(int fd)
> +{
> + struct drm_i915_query_item item;
> + uint8_t *_topo_info;
> + struct drm_i915_query_topology_info *topo_info;
> + drm_i915_getparam_t gp;
> + int slice_mask, subslice_mask;
> + int s, topology_slices, topology_subslices_slice0;
Not in order of use, or width, never mind.
Some of the types look like should be unsigned or u32?
> +
> + gp.param = I915_PARAM_SLICE_MASK;
> + gp.value = &slice_mask;
> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
> +
> + gp.param = I915_PARAM_SUBSLICE_MASK;
> + gp.value = &subslice_mask;
> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
> +
> + /* Slices */
> + memset(&item, 0, sizeof(item));
> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> + i915_query_item(fd, &item, 1);
> + igt_assert(item.length > 0);
Is it possible to check for the exact value here? Or at least minimum
amounting to the v1 query minimum?
> +
> + _topo_info = malloc(3 * item.length);
Assert on malloc.
What is 3?
> + memset(_topo_info, 0xff, 3 * item.length);
> + topo_info = (struct drm_i915_query_topology_info *) (_topo_info + item.length);
> + memset(topo_info, 0, item.length);
> + igt_assert(topo_info);
Unusual to assert _after_ dereference.
> +
> + item.data_ptr = to_user_pointer(topo_info);
> + i915_query_item(fd, &item, 1);
> + igt_assert(item.length > 0);
Could assert more strongly that this length matches the initial query.
> +
> + topology_slices = 0;
> + for (s = 0; s < topo_info->max_slices; s++) {
> + if (slice_available(topo_info, s))
> + topology_slices |= 1UL << s;
> + }
> +
> + igt_debug("slice mask getparam=0x%x / query=0x%x\n",
> + slice_mask, topology_slices);
> +
> + /* These 2 should always match. */
> + igt_assert_eq_u32(slice_mask, topology_slices);
> +
> + topology_subslices_slice0 = 0;
> + for (s = 0; s < topo_info->max_subslices; s++) {
> + if (subslice_available(topo_info, 0, s))
> + topology_subslices_slice0 |= 1UL << s;
> + }
> +
> + igt_debug("subslice mask getparam=0x%x / query=0x%x\n",
> + subslice_mask, topology_subslices_slice0);
> +
> + /* I915_PARAM_SUBSLICE_MASK returns the value for slice0, we
> + * should match the values for the first slice of the
> + * topology.
> + */
> + igt_assert_eq_u32(subslice_mask, topology_subslices_slice0);
> +
> + for (s = 0; s < item.length; s++) {
> + igt_assert_eq(_topo_info[s], 0xff);
> + igt_assert_eq(_topo_info[2 * item.length + s], 0xff);
> + }
Ah so you are checking that kernel did not under or over write?
I'd move this into a more generic test than the one dealing with actual
topology verification.
> +
> + free(_topo_info);
> +}
> +
> +static void
> +test_query_topology_matches_eu_total(int fd)
> +{
> + struct drm_i915_query_item item;
> + struct drm_i915_query_topology_info *topo_info;
> + drm_i915_getparam_t gp;
> + int n_eus, n_eus_topology, s;
> +
> + gp.param = I915_PARAM_EU_TOTAL;
> + gp.value = &n_eus;
> + do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> + igt_debug("n_eus=%i\n", n_eus);
> +
> + memset(&item, 0, sizeof(item));
> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> + i915_query_item(fd, &item, 1);
> +
> + topo_info = (struct drm_i915_query_topology_info *) calloc(1, item.length);
> +
> + item.data_ptr = to_user_pointer(topo_info);
> + i915_query_item(fd, &item, 1);
> +
> + igt_debug("max_slices=%hu max_subslices=%hu max_eus_per_subslice=%hu\n",
> + topo_info->max_slices, topo_info->max_subslices,
> + topo_info->max_eus_per_subslice);
> + igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
> + topo_info->subslice_offset, topo_info->subslice_stride);
> + igt_debug(" eu_offset=%hu eu_stride=%hu\n",
> + topo_info->eu_offset, topo_info->eu_stride);
> +
> + n_eus_topology = 0;
> + for (s = 0; s < topo_info->max_slices; s++) {
> + int ss;
> +
> + igt_debug("slice%i:\n", s);
> +
> + for (ss = 0; ss < topo_info->max_subslices; ss++) {
> + int eu, n_subslice_eus = 0;
> +
> + igt_debug("\tsubslice: %i\n", ss);
> +
> + igt_debug("\t\teu_mask: 0b");
> + for (eu = 0; eu < topo_info->max_eus_per_subslice; eu++) {
> + uint8_t val = eu_available(topo_info, s, ss,
> + topo_info->max_eus_per_subslice - 1 - eu);
Why is the eu parameter reversed on not simply eu?
Do you need to assert that the val matches the expectations from slice
and subslice mask - meaning if the s/ss are zero in their masks val must
be zero here as well?
> + igt_debug("%hhi", val);
> + n_subslice_eus += __builtin_popcount(val);
> + n_eus_topology += __builtin_popcount(val);
> + }
> + igt_debug(" (%i)\n", n_subslice_eus);
> + }
> + }
> +
> + igt_assert(n_eus_topology == n_eus);
> +}
> +
> +igt_main
> +{
> + int fd = -1;
> + int devid;
> +
> + igt_fixture {
> + fd = drm_open_driver(DRIVER_INTEL);
> + igt_require(has_query_supports(fd));
> + devid = intel_get_drm_devid(fd);
> + }
> +
> + igt_subtest("query-garbage")
> + test_query_garbage(fd);
> +
> + igt_subtest("query-topology-pre-gen8") {
> + igt_require(intel_gen(devid) < 8 && !IS_HASWELL(devid));
> + igt_require(query_topology_supported(fd));
Does it even passes this line pre-gen8? If item length will be -ENODEV
then it doesn't, no?
> + test_query_topology_pre_gen8(fd);
I'd call this subtest query-topology-unsupported - since the test
expects ENODEV always.
> + }
> +
> + igt_subtest("query-topology-coherent-slice-mask") {
> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
Why not simply gen >= 8 instead of AT_LEAST_GEN (first time I see this one)?
> + igt_require(query_topology_supported(fd));
I am actually not sure if this should be igt_assert or igt_require. Do
we actually want to support running new igts on old kernels, or make the
test stronger by failing when it thinks it should work. Will need to ask
for second opinions.
It would have skipped already from the fixture if query API is not
supported.
> + test_query_topology_coherent_slice_mask(fd);
> + }
> +
> + igt_subtest("query-topology-matches-eu-total") {
> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
> + igt_require(query_topology_supported(fd));
> + test_query_topology_matches_eu_total(fd);
> + }
> +
Is it possible to add some tests which run on platforms where we can
100% imply the slice/subslice configuration from the devid and then
verify topology query is returning expected data for all slices etc?
> + igt_fixture {
> + close(fd);
> + }
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 2a1e6f19..a805011e 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -144,6 +144,7 @@ test_progs = [
> 'gen3_render_tiledy_blits',
> 'gen7_forcewake_mt',
> 'gvt_basic',
> + 'i915_query',
> 'kms_3d',
> 'kms_addfb_basic',
> 'kms_atomic',
>
Regards,
Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 11:22 ` Tvrtko Ursulin
@ 2018-03-08 14:31 ` Lionel Landwerlin
2018-03-08 14:44 ` Chris Wilson
2018-03-08 15:44 ` Tvrtko Ursulin
0 siblings, 2 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2018-03-08 14:31 UTC (permalink / raw)
To: Tvrtko Ursulin, igt-dev
On 08/03/18 11:22, Tvrtko Ursulin wrote:
>
> Commit message is missing.
>
> On 26/02/2018 17:59, Lionel Landwerlin wrote:
>> v2: Complete invalid cases (Chris)
>> Some styling (to_user_pointer, etc...) (Chris)
>> New error check, through item.length (Chris)
>>
>> v3: Update for new uAPI iteration (Lionel)
>>
>> v4: Return errno from a single point (Chris)
>> Poising checks (Chris)
>>
>> v5: Add more debug traces (Lionel)
>> Update uAPI (Joonas/Lionel)
>> Make sure Haswell is tested (Lionel)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>> tests/Makefile.sources | 1 +
>> tests/i915_query.c | 314
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/meson.build | 1 +
>> 3 files changed, 316 insertions(+)
>> create mode 100644 tests/i915_query.c
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 23f859be..b4c8a913 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -168,6 +168,7 @@ TESTS_progs = \
>> gen3_render_tiledy_blits \
>> gen7_forcewake_mt \
>> gvt_basic \
>> + i915_query \
>
> Interesting question how we want to name this test. We don't have any
> i915_ prefix tests, but for instance there is drv_getparams_basic,
> suggesting this could be called drv_query_ioctl or something?
>
> Open for discussion I guess.
I've been considering renaming perf.c (that's a pretty confusing one...)
One could argue that if IGT tests more than intel devices, we should
have a driver prefix (I see vc4 has).
>
>> kms_3d \
>> kms_addfb_basic \
>> kms_atomic \
>> diff --git a/tests/i915_query.c b/tests/i915_query.c
>> new file mode 100644
>> index 00000000..21621b78
>> --- /dev/null
>> +++ b/tests/i915_query.c
>> @@ -0,0 +1,314 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include "igt.h"
>> +
>> +#include <limits.h>
>> +
>> +IGT_TEST_DESCRIPTION("Testing the i915 query uAPI.");
>> +
>> +static int
>> +__i915_query(int fd, struct drm_i915_query *q)
>> +{
>> + if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
>> + return -errno;
>> + return 0;
>> +}
>> +
>> +static int
>> +__i915_query_item(int fd, struct drm_i915_query_item *items,
>> uint32_t n_items)
>> +{
>> + struct drm_i915_query q = {
>> + .num_items = n_items,
>> + .items_ptr = to_user_pointer(items),
>> + };
>> + return __i915_query(fd, &q);
>> +}
>> +
>> +#define i915_query_item(fd, items, n_items) do { \
>> + igt_assert_eq(__i915_query_item(fd, items, n_items), 0); \
>> + errno = 0; \
>> + } while (0)
>> +#define i915_query_item_err(fd, items, n_items, err) do { \
>> + igt_assert_eq(__i915_query_item(fd, items, n_items), -err); \
>> + } while (0)
>> +
>
> Suggest plural i915_query_items for the above.
Done.
>
>> +static bool has_query_supports(int fd)
>> +{
>> + struct drm_i915_query query = {};
>> +
>> + return __i915_query(fd, &query) == 0;
>> +}
>> +
>> +static void test_query_garbage(int fd)
>> +{
>> + struct drm_i915_query_item items[2];
>> + struct drm_i915_query_item *items_ptr;
>> + int i, len, ret;
>> +
>> + i915_query_item_err(fd, (void *) ULONG_MAX, 1, EFAULT);
>> +
>> + i915_query_item_err(fd, (void *) 0, 1, EFAULT);
>
> Could add comments throughout to explain what is being tested.
Yep.
>
> Test for non-zero flags field? Both in the query and query item.
Done.
>
>> +
>> + memset(items, 0, sizeof(items));
>> + i915_query_item_err(fd, items, 1, EINVAL);
>> +
>> + memset(items, 0, sizeof(items));
>> + items[0].query_id = ULONG_MAX;
>> + items[1].query_id = ULONG_MAX - 2;
>
> What is special about ULONG_MAX - 2 versus ULONG_MAX? And not
> ULONG_MAX - 1, or some other number?
Nothing really... It's just a really bit number that we're unlikely to
reach.
>
>> + i915_query_item(fd, items, 2);
>> + igt_assert_eq(items[0].length, -EINVAL);
>> + igt_assert_eq(items[1].length, -EINVAL);
>> +
>> + memset(items, 0, sizeof(items));
>> + items[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> + items[1].query_id = DRM_I915_QUERY_TOPOLOGY_INFO; > +
>> items[1].length = sizeof(struct drm_i915_query_topology_info) - 1;
>> + i915_query_item(fd, items, 2);
>> + igt_assert_lte(0, items[0].length);
>> + igt_assert_eq(items[1].length, -EINVAL);
>
> Tricky one - ideally we would want to split testing of the query API
> from specific queries, but then we don't have any queries to test
> with.. can't split it then.
>
>> +
>> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>> -1, 0);
>> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> + i915_query_item(fd, items_ptr, 1);
>> + igt_assert(items_ptr[0].length >= sizeof(struct
>> drm_i915_query_topology_info));
>> + munmap(items_ptr, 4096);
>> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
>
> Another good test would be passing in a read only mapping and checking
> for EFAULT when length writeback fails.
Hm... how to do you write something sensible into a read only mapping so
that the kernel would at least try to write to it? :)
>
>> +
>> + len = sizeof(struct drm_i915_query_item) * 10;
>> + items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1,
>> 0);
>> + for (i = 0; i < 10; i++)
>> + items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> + ret = __i915_query_item(fd, items_ptr,
>> + INT_MAX / sizeof(struct drm_i915_query_item) + 1);
>> + igt_assert(ret == -EFAULT || ret == -EINVAL);
>
> What is this testing? Ten queries, all write only so will EFAULT. But
> then you pass in nitems = INT_MAX / sizeof.. which is what exactly? I
> don't get it.
Trying to get the kernel to read into unmapped address space and
expecting an EFAULT or if we're unlucky and it ends up into another
mapping invalid query items.
>
> And assert cannot be either this or that, it has to know explicitly or
> split into two tests or something.
>
>> + munmap(items_ptr, len);
>> +}
>> +
>> +static bool query_topology_supported(int fd)
>> +{
>> + struct drm_i915_query_item item = {
>> + .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
>> + };
>> +
>> + return __i915_query_item(fd, &item, 1) == 0 && item.length > 0;
>> +}
>> +
>> +static void test_query_topology_pre_gen8(int fd)
>> +{
>> + struct drm_i915_query_item item = {
>> + .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
>> + };
>> +
>> + i915_query_item(fd, &item, 1);
>> + igt_assert_eq(item.length, -ENODEV);
>> +}
>> +
>> +#define DIV_ROUND_UP(val, div) (ALIGN(val, div) / div)
>> +
>> +static bool
>> +slice_available(const struct drm_i915_query_topology_info *topo_info,
>> + int s)
>> +{
>> + return (topo_info->data[s / 8] >> (s % 8)) & 1;
>> +}
>> +
>> +static bool
>> +subslice_available(const struct drm_i915_query_topology_info
>> *topo_info,
>> + int s, int ss)
>> +{
>> + return (topo_info->data[topo_info->subslice_offset +
>> + s * topo_info->subslice_stride +
>> + ss / 8] >> (ss % 8)) & 1;
>> +}
>> +
>> +static bool
>> +eu_available(const struct drm_i915_query_topology_info *topo_info,
>> + int s, int ss, int eu)
>> +{
>> + return (topo_info->data[topo_info->eu_offset +
>> + (s * topo_info->max_subslices + ss) *
>> topo_info->eu_stride +
>> + eu / 8] >> (eu % 8)) & 1;
>> +}
>> +
>> +static void
>> +test_query_topology_coherent_slice_mask(int fd)
>> +{
>> + struct drm_i915_query_item item;
>> + uint8_t *_topo_info;
>> + struct drm_i915_query_topology_info *topo_info;
>> + drm_i915_getparam_t gp;
>> + int slice_mask, subslice_mask;
>> + int s, topology_slices, topology_subslices_slice0;
>
> Not in order of use, or width, never mind.
>
> Some of the types look like should be unsigned or u32?
getparam returns int, so I made the rest the same (since we want to
compare them).
>
>> +
>> + gp.param = I915_PARAM_SLICE_MASK;
>> + gp.value = &slice_mask;
>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>> +
>> + gp.param = I915_PARAM_SUBSLICE_MASK;
>> + gp.value = &subslice_mask;
>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>> +
>> + /* Slices */
>> + memset(&item, 0, sizeof(item));
>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> + i915_query_item(fd, &item, 1);
>> + igt_assert(item.length > 0);
>
> Is it possible to check for the exact value here? Or at least minimum
> amounting to the v1 query minimum?
Can't check for an exact value, it completely depends on the GT size.
I guess we can do >= 3 (at least one byte for each section (slice,
subslice, eus).
>
>> +
>> + _topo_info = malloc(3 * item.length);
>
> Assert on malloc.
>
> What is 3?
Some room on both sides, to verify that the kernel doesn't write outside
the bounds.
>
>> + memset(_topo_info, 0xff, 3 * item.length);
>> + topo_info = (struct drm_i915_query_topology_info *) (_topo_info
>> + item.length);
>> + memset(topo_info, 0, item.length);
>> + igt_assert(topo_info);
>
> Unusual to assert _after_ dereference.
Uh... Removing.
>
>> +
>> + item.data_ptr = to_user_pointer(topo_info);
>> + i915_query_item(fd, &item, 1);
>> + igt_assert(item.length > 0);
>
> Could assert more strongly that this length matches the initial query.
>
>> +
>> + topology_slices = 0;
>> + for (s = 0; s < topo_info->max_slices; s++) {
>> + if (slice_available(topo_info, s))
>> + topology_slices |= 1UL << s;
>> + }
>> +
>> + igt_debug("slice mask getparam=0x%x / query=0x%x\n",
>> + slice_mask, topology_slices);
>> +
>> + /* These 2 should always match. */
>> + igt_assert_eq_u32(slice_mask, topology_slices);
>> +
>> + topology_subslices_slice0 = 0;
>> + for (s = 0; s < topo_info->max_subslices; s++) {
>> + if (subslice_available(topo_info, 0, s))
>> + topology_subslices_slice0 |= 1UL << s;
>> + }
>> +
>> + igt_debug("subslice mask getparam=0x%x / query=0x%x\n",
>> + subslice_mask, topology_subslices_slice0);
>> +
>> + /* I915_PARAM_SUBSLICE_MASK returns the value for slice0, we
>> + * should match the values for the first slice of the
>> + * topology.
>> + */
>> + igt_assert_eq_u32(subslice_mask, topology_subslices_slice0);
>> +
>> + for (s = 0; s < item.length; s++) {
>> + igt_assert_eq(_topo_info[s], 0xff);
>> + igt_assert_eq(_topo_info[2 * item.length + s], 0xff);
>> + }
>
> Ah so you are checking that kernel did not under or over write?
Yes, Chris' request.
>
> I'd move this into a more generic test than the one dealing with
> actual topology verification.
Done.
>
>> +
>> + free(_topo_info);
>> +}
>> +
>> +static void
>> +test_query_topology_matches_eu_total(int fd)
>> +{
>> + struct drm_i915_query_item item;
>> + struct drm_i915_query_topology_info *topo_info;
>> + drm_i915_getparam_t gp;
>> + int n_eus, n_eus_topology, s;
>> +
>> + gp.param = I915_PARAM_EU_TOTAL;
>> + gp.value = &n_eus;
>> + do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>> + igt_debug("n_eus=%i\n", n_eus);
>> +
>> + memset(&item, 0, sizeof(item));
>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> + i915_query_item(fd, &item, 1);
>> +
>> + topo_info = (struct drm_i915_query_topology_info *) calloc(1,
>> item.length);
>> +
>> + item.data_ptr = to_user_pointer(topo_info);
>> + i915_query_item(fd, &item, 1);
>> +
>> + igt_debug("max_slices=%hu max_subslices=%hu
>> max_eus_per_subslice=%hu\n",
>> + topo_info->max_slices, topo_info->max_subslices,
>> + topo_info->max_eus_per_subslice);
>> + igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
>> + topo_info->subslice_offset, topo_info->subslice_stride);
>> + igt_debug(" eu_offset=%hu eu_stride=%hu\n",
>> + topo_info->eu_offset, topo_info->eu_stride);
>> +
>> + n_eus_topology = 0;
>> + for (s = 0; s < topo_info->max_slices; s++) {
>> + int ss;
>> +
>> + igt_debug("slice%i:\n", s);
>> +
>> + for (ss = 0; ss < topo_info->max_subslices; ss++) {
>> + int eu, n_subslice_eus = 0;
>> +
>> + igt_debug("\tsubslice: %i\n", ss);
>> +
>> + igt_debug("\t\teu_mask: 0b");
>> + for (eu = 0; eu < topo_info->max_eus_per_subslice; eu++) {
>> + uint8_t val = eu_available(topo_info, s, ss,
>> + topo_info->max_eus_per_subslice - 1 - eu);
>
> Why is the eu parameter reversed on not simply eu?
Because of the printf ordering (starting with most significant bit).
>
> Do you need to assert that the val matches the expectations from slice
> and subslice mask - meaning if the s/ss are zero in their masks val
> must be zero here as well?
Oh right, I can add that. Thanks!
>
>> + igt_debug("%hhi", val);
>> + n_subslice_eus += __builtin_popcount(val);
>> + n_eus_topology += __builtin_popcount(val);
>> + }
>> + igt_debug(" (%i)\n", n_subslice_eus);
>> + }
>> + }
>> +
>> + igt_assert(n_eus_topology == n_eus);
>> +}
>> +
>> +igt_main
>> +{
>> + int fd = -1;
>> + int devid;
>> +
>> + igt_fixture {
>> + fd = drm_open_driver(DRIVER_INTEL);
>> + igt_require(has_query_supports(fd));
>> + devid = intel_get_drm_devid(fd);
>> + }
>> +
>> + igt_subtest("query-garbage")
>> + test_query_garbage(fd);
>> +
>> + igt_subtest("query-topology-pre-gen8") {
>> + igt_require(intel_gen(devid) < 8 && !IS_HASWELL(devid));
>> + igt_require(query_topology_supported(fd));
>
> Does it even passes this line pre-gen8? If item length will be -ENODEV
> then it doesn't, no?
Oh right...
I need to adapt query_topology_supported() for that.
>
>> + test_query_topology_pre_gen8(fd);
>
> I'd call this subtest query-topology-unsupported - since the test
> expects ENODEV always.
Done.
>
>> + }
>> +
>> + igt_subtest("query-topology-coherent-slice-mask") {
>> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>
> Why not simply gen >= 8 instead of AT_LEAST_GEN (first time I see this
> one)?
Sure.
>
>> + igt_require(query_topology_supported(fd));
>
> I am actually not sure if this should be igt_assert or igt_require. Do
> we actually want to support running new igts on old kernels, or make
> the test stronger by failing when it thinks it should work. Will need
> to ask for second opinions.
>
> It would have skipped already from the fixture if query API is not
> supported.
I was under the impression that we wanted some kind of old kernel
support (like 4.15).
>
>> + test_query_topology_coherent_slice_mask(fd);
>> + }
>> +
>> + igt_subtest("query-topology-matches-eu-total") {
>> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>> + igt_require(query_topology_supported(fd));
>> + test_query_topology_matches_eu_total(fd);
>> + }
>> +
>
> Is it possible to add some tests which run on platforms where we can
> 100% imply the slice/subslice configuration from the devid and then
> verify topology query is returning expected data for all slices etc?
That's tricky. I don't know what we actually ship (are any big cores
ever fused off in the field? Is it just atoms?)
I have a production big core BDW laptop with lots of fused stuff, so my
guess is no...
>
>> + igt_fixture {
>> + close(fd);
>> + }
>> +}
>> diff --git a/tests/meson.build b/tests/meson.build
>> index 2a1e6f19..a805011e 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -144,6 +144,7 @@ test_progs = [
>> 'gen3_render_tiledy_blits',
>> 'gen7_forcewake_mt',
>> 'gvt_basic',
>> + 'i915_query',
>> 'kms_3d',
>> 'kms_addfb_basic',
>> 'kms_atomic',
>>
>
> Regards,
>
> Tvrtko
>
Thanks for the review!
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 14:31 ` Lionel Landwerlin
@ 2018-03-08 14:44 ` Chris Wilson
2018-03-08 14:52 ` Lionel Landwerlin
2018-03-08 15:47 ` Tvrtko Ursulin
2018-03-08 15:44 ` Tvrtko Ursulin
1 sibling, 2 replies; 13+ messages in thread
From: Chris Wilson @ 2018-03-08 14:44 UTC (permalink / raw)
To: Lionel Landwerlin, Tvrtko Ursulin, igt-dev
Quoting Lionel Landwerlin (2018-03-08 14:31:16)
> On 08/03/18 11:22, Tvrtko Ursulin wrote:
> > Interesting question how we want to name this test. We don't have any
> > i915_ prefix tests, but for instance there is drv_getparams_basic,
> > suggesting this could be called drv_query_ioctl or something?
> >
> > Open for discussion I guess.
>
> I've been considering renaming perf.c (that's a pretty confusing one...)
>
> One could argue that if IGT tests more than intel devices, we should
> have a driver prefix (I see vc4 has).
Yes. Move all of the i915.ko specific tests to tests/i915/
For build system simplicity, I'd advocate keeping generic tests
separate from driver specific tests. So kms_flip would have kms/ and
i915/ portions, etc.
(And tools/i915; benchmarks/i915 etc)
> >> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
> >> -1, 0);
> >> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
> >> + i915_query_item(fd, items_ptr, 1);
> >> + igt_assert(items_ptr[0].length >= sizeof(struct
> >> drm_i915_query_topology_info));
> >> + munmap(items_ptr, 4096);
> >> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
> >
> > Another good test would be passing in a read only mapping and checking
> > for EFAULT when length writeback fails.
>
> Hm... how to do you write something sensible into a read only mapping so
> that the kernel would at least try to write to it? :)
ptr = mmap(sz);
memset(ptr, 0xc5, sz);
mprotect(ptr, sz, PROT_READ);
So you can setup a valid blob tricking the kernel to write back to it
and send us an EFAULT.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 14:44 ` Chris Wilson
@ 2018-03-08 14:52 ` Lionel Landwerlin
2018-03-08 15:47 ` Tvrtko Ursulin
1 sibling, 0 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2018-03-08 14:52 UTC (permalink / raw)
To: Chris Wilson, Tvrtko Ursulin, igt-dev
On 08/03/18 14:44, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-03-08 14:31:16)
>> On 08/03/18 11:22, Tvrtko Ursulin wrote:
>>> Interesting question how we want to name this test. We don't have any
>>> i915_ prefix tests, but for instance there is drv_getparams_basic,
>>> suggesting this could be called drv_query_ioctl or something?
>>>
>>> Open for discussion I guess.
>> I've been considering renaming perf.c (that's a pretty confusing one...)
>>
>> One could argue that if IGT tests more than intel devices, we should
>> have a driver prefix (I see vc4 has).
> Yes. Move all of the i915.ko specific tests to tests/i915/
>
> For build system simplicity, I'd advocate keeping generic tests
> separate from driver specific tests. So kms_flip would have kms/ and
> i915/ portions, etc.
>
> (And tools/i915; benchmarks/i915 etc)
>
>>>> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>>>> -1, 0);
>>>> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>>> + i915_query_item(fd, items_ptr, 1);
>>>> + igt_assert(items_ptr[0].length >= sizeof(struct
>>>> drm_i915_query_topology_info));
>>>> + munmap(items_ptr, 4096);
>>>> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
>>> Another good test would be passing in a read only mapping and checking
>>> for EFAULT when length writeback fails.
>> Hm... how to do you write something sensible into a read only mapping so
>> that the kernel would at least try to write to it? :)
> ptr = mmap(sz);
> memset(ptr, 0xc5, sz);
> mprotect(ptr, sz, PROT_READ);
>
> So you can setup a valid blob tricking the kernel to write back to it
> and send us an EFAULT.
> -Chris
>
Thanks I ended up answering my own question :)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 14:31 ` Lionel Landwerlin
2018-03-08 14:44 ` Chris Wilson
@ 2018-03-08 15:44 ` Tvrtko Ursulin
2018-03-08 17:51 ` Lionel Landwerlin
1 sibling, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-08 15:44 UTC (permalink / raw)
To: Lionel Landwerlin, igt-dev
On 08/03/2018 14:31, Lionel Landwerlin wrote:
> On 08/03/18 11:22, Tvrtko Ursulin wrote:
>>
>> Commit message is missing.
>>
>> On 26/02/2018 17:59, Lionel Landwerlin wrote:
>>> v2: Complete invalid cases (Chris)
>>> Some styling (to_user_pointer, etc...) (Chris)
>>> New error check, through item.length (Chris)
>>>
>>> v3: Update for new uAPI iteration (Lionel)
>>>
>>> v4: Return errno from a single point (Chris)
>>> Poising checks (Chris)
>>>
>>> v5: Add more debug traces (Lionel)
>>> Update uAPI (Joonas/Lionel)
>>> Make sure Haswell is tested (Lionel)
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> ---
>>> tests/Makefile.sources | 1 +
>>> tests/i915_query.c | 314
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>> tests/meson.build | 1 +
>>> 3 files changed, 316 insertions(+)
>>> create mode 100644 tests/i915_query.c
>>>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..b4c8a913 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -168,6 +168,7 @@ TESTS_progs = \
>>> gen3_render_tiledy_blits \
>>> gen7_forcewake_mt \
>>> gvt_basic \
>>> + i915_query \
>>
>> Interesting question how we want to name this test. We don't have any
>> i915_ prefix tests, but for instance there is drv_getparams_basic,
>> suggesting this could be called drv_query_ioctl or something?
>>
>> Open for discussion I guess.
>
> I've been considering renaming perf.c (that's a pretty confusing one...)
>
> One could argue that if IGT tests more than intel devices, we should
> have a driver prefix (I see vc4 has).
Yes, to an extent, but we got prior use on drv_ so until a grander
scheme is devised I'd stick with it. But no huge complaints towards
keeping i915 either, it's not really that important.
>>> +
>>> + memset(items, 0, sizeof(items));
>>> + i915_query_item_err(fd, items, 1, EINVAL);
>>> +
>>> + memset(items, 0, sizeof(items));
>>> + items[0].query_id = ULONG_MAX;
>>> + items[1].query_id = ULONG_MAX - 2;
>>
>> What is special about ULONG_MAX - 2 versus ULONG_MAX? And not
>> ULONG_MAX - 1, or some other number?
>
> Nothing really... It's just a really bit number that we're unlikely to
> reach.
I'd maybe first go with one invalid query id, then second test try two
different invalid ids. So it is at least more obvious what's happening.
Oh another missed test case: invalid query (in different ways) followed
by a valid one. First must fail, second succeed. And in opposite order
as well.
>>> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>>> -1, 0);
>>> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> + i915_query_item(fd, items_ptr, 1);
>>> + igt_assert(items_ptr[0].length >= sizeof(struct
>>> drm_i915_query_topology_info));
>>> + munmap(items_ptr, 4096);
>>> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
>>
>> Another good test would be passing in a read only mapping and checking
>> for EFAULT when length writeback fails.
>
> Hm... how to do you write something sensible into a read only mapping so
> that the kernel would at least try to write to it? :)
Make it read-only after populating it. :) man mprotect.
>>
>>> +
>>> + len = sizeof(struct drm_i915_query_item) * 10;
>>> + items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1,
>>> 0);
>>> + for (i = 0; i < 10; i++)
>>> + items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> + ret = __i915_query_item(fd, items_ptr,
>>> + INT_MAX / sizeof(struct drm_i915_query_item) + 1);
>>> + igt_assert(ret == -EFAULT || ret == -EINVAL);
>>
>> What is this testing? Ten queries, all write only so will EFAULT. But
>> then you pass in nitems = INT_MAX / sizeof.. which is what exactly? I
>> don't get it.
>
> Trying to get the kernel to read into unmapped address space and
> expecting an EFAULT or if we're unlucky and it ends up into another
> mapping invalid query items.
You need to set up the mappings explicitly and not count on luck. Create
exact sequence of mapped r/w, mapped-r, mapped-w and unmapped query
items. You can use MAP_FIXED, mremap and mprotect to engineer exact
layouts to test against.
Helper which processes input arrays like:
struct ... {
query-id;
enum { unmapped, ro, rw, w, NULL };
len;
expected-result;
expected-lenght;
} query_items[] = {
{ TOPOLOGY, rw, ... ,0, .. },
{ TOPOLOGY, ro, ..., -EFAULT, ... },
{ INVALID, rw, ..., -EINVAL, ... },
{ TOPOLOGY, w, ..., -EFAULT, ... },
{ TOPOLOGY, NULL, 0, 0, len },
};
ret = test_items(fd, query_items, ARRAY_SIZE(query_items));
igt_assert_eq(ret, 0);
Comes to mind. Very roughly though, so see if it makes sense to you.
Then you can have different query item lists defined like that and just
feed them to test helper. Even 2d array of test items for this class of
test scenarios would work so you would just iterate it. Maybe store the
expected return from the drm_i915_query ioctl for each list.
>>> +static void
>>> +test_query_topology_coherent_slice_mask(int fd)
>>> +{
>>> + struct drm_i915_query_item item;
>>> + uint8_t *_topo_info;
>>> + struct drm_i915_query_topology_info *topo_info;
>>> + drm_i915_getparam_t gp;
>>> + int slice_mask, subslice_mask;
>>> + int s, topology_slices, topology_subslices_slice0;
>>
>> Not in order of use, or width, never mind.
>>
>> Some of the types look like should be unsigned or u32?
>
> getparam returns int, so I made the rest the same (since we want to
> compare them).
igt_assert_eq_u32(int, int) below is what caught my eye. Don't know then.
>>
>>> +
>>> + gp.param = I915_PARAM_SLICE_MASK;
>>> + gp.value = &slice_mask;
>>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>> +
>>> + gp.param = I915_PARAM_SUBSLICE_MASK;
>>> + gp.value = &subslice_mask;
>>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>> +
>>> + /* Slices */
>>> + memset(&item, 0, sizeof(item));
>>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> + i915_query_item(fd, &item, 1);
>>> + igt_assert(item.length > 0);
>>
>> Is it possible to check for the exact value here? Or at least minimum
>> amounting to the v1 query minimum?
>
> Can't check for an exact value, it completely depends on the GT size.
> I guess we can do >= 3 (at least one byte for each section (slice,
> subslice, eus).
Isnt' sizeof(struct drm_i915_query_topology_info) included in the
length? So it should be at least that big, no?
>>
>>> +
>>> + _topo_info = malloc(3 * item.length);
>>
>> Assert on malloc.
>>
>> What is 3?
>
> Some room on both sides, to verify that the kernel doesn't write outside
> the bounds.
Yeah figured it out below. Put a comment please.
>>> +
>>> + free(_topo_info);
>>> +}
>>> +
>>> +static void
>>> +test_query_topology_matches_eu_total(int fd)
>>> +{
>>> + struct drm_i915_query_item item;
>>> + struct drm_i915_query_topology_info *topo_info;
>>> + drm_i915_getparam_t gp;
>>> + int n_eus, n_eus_topology, s;
>>> +
>>> + gp.param = I915_PARAM_EU_TOTAL;
>>> + gp.value = &n_eus;
>>> + do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>> + igt_debug("n_eus=%i\n", n_eus);
>>> +
>>> + memset(&item, 0, sizeof(item));
>>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> + i915_query_item(fd, &item, 1);
>>> +
>>> + topo_info = (struct drm_i915_query_topology_info *) calloc(1,
>>> item.length);
>>> +
>>> + item.data_ptr = to_user_pointer(topo_info);
>>> + i915_query_item(fd, &item, 1);
>>> +
>>> + igt_debug("max_slices=%hu max_subslices=%hu
>>> max_eus_per_subslice=%hu\n",
>>> + topo_info->max_slices, topo_info->max_subslices,
>>> + topo_info->max_eus_per_subslice);
>>> + igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
>>> + topo_info->subslice_offset, topo_info->subslice_stride);
>>> + igt_debug(" eu_offset=%hu eu_stride=%hu\n",
>>> + topo_info->eu_offset, topo_info->eu_stride);
>>> +
>>> + n_eus_topology = 0;
>>> + for (s = 0; s < topo_info->max_slices; s++) {
>>> + int ss;
>>> +
>>> + igt_debug("slice%i:\n", s);
>>> +
>>> + for (ss = 0; ss < topo_info->max_subslices; ss++) {
>>> + int eu, n_subslice_eus = 0;
>>> +
>>> + igt_debug("\tsubslice: %i\n", ss);
>>> +
>>> + igt_debug("\t\teu_mask: 0b");
>>> + for (eu = 0; eu < topo_info->max_eus_per_subslice; eu++) {
>>> + uint8_t val = eu_available(topo_info, s, ss,
>>> + topo_info->max_eus_per_subslice - 1 - eu);
>>
>> Why is the eu parameter reversed on not simply eu?
>
> Because of the printf ordering (starting with most significant bit).
For debug.. grumble. Ok. Hm, but are you printing out more than one u8
here, or will you be, and will bit ordering be correct then?
>>> + igt_require(query_topology_supported(fd));
>>
>> I am actually not sure if this should be igt_assert or igt_require. Do
>> we actually want to support running new igts on old kernels, or make
>> the test stronger by failing when it thinks it should work. Will need
>> to ask for second opinions.
>>
>> It would have skipped already from the fixture if query API is not
>> supported.
>
> I was under the impression that we wanted some kind of old kernel
> support (like 4.15).
But in fixture the whole thing skip already if there is no query support
in the kernel. And there will be no kernels with query but no topology.
>>
>>> + test_query_topology_coherent_slice_mask(fd);
>>> + }
>>> +
>>> + igt_subtest("query-topology-matches-eu-total") {
>>> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>>> + igt_require(query_topology_supported(fd));
>>> + test_query_topology_matches_eu_total(fd);
>>> + }
>>> +
>>
>> Is it possible to add some tests which run on platforms where we can
>> 100% imply the slice/subslice configuration from the devid and then
>> verify topology query is returning expected data for all slices etc?
>
> That's tricky. I don't know what we actually ship (are any big cores
> ever fused off in the field? Is it just atoms?)
> I have a production big core BDW laptop with lots of fused stuff, so my
> guess is no...
Haswell maybe? According to haswell_sseu_info_init in the kernel
subslice masks are purely pci id based. EU count wouldn need a register
read.
Bottom line is we could also read fuse registers from IGT and verify
state between the two.
At least Haswell look easy enough for some basic coverage. I won't say
we need to do all, but at least we need to look at more than just
subslice 0.
Regards,
Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 14:44 ` Chris Wilson
2018-03-08 14:52 ` Lionel Landwerlin
@ 2018-03-08 15:47 ` Tvrtko Ursulin
1 sibling, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-08 15:47 UTC (permalink / raw)
To: Chris Wilson, Lionel Landwerlin, igt-dev
On 08/03/2018 14:44, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-03-08 14:31:16)
>> On 08/03/18 11:22, Tvrtko Ursulin wrote:
>>> Interesting question how we want to name this test. We don't have any
>>> i915_ prefix tests, but for instance there is drv_getparams_basic,
>>> suggesting this could be called drv_query_ioctl or something?
>>>
>>> Open for discussion I guess.
>>
>> I've been considering renaming perf.c (that's a pretty confusing one...)
>>
>> One could argue that if IGT tests more than intel devices, we should
>> have a driver prefix (I see vc4 has).
>
> Yes. Move all of the i915.ko specific tests to tests/i915/
>
> For build system simplicity, I'd advocate keeping generic tests
> separate from driver specific tests. So kms_flip would have kms/ and
> i915/ portions, etc.
>
> (And tools/i915; benchmarks/i915 etc)
Okay but pencil in for future work I'd suggest since we are working
against a deadline here. :) At least no need to incorporate larger
renames like that into this work.
Regards,
Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
2018-03-08 15:44 ` Tvrtko Ursulin
@ 2018-03-08 17:51 ` Lionel Landwerlin
0 siblings, 0 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2018-03-08 17:51 UTC (permalink / raw)
To: Tvrtko Ursulin, igt-dev
On 08/03/18 15:44, Tvrtko Ursulin wrote:
>
> On 08/03/2018 14:31, Lionel Landwerlin wrote:
>> On 08/03/18 11:22, Tvrtko Ursulin wrote:
>>>
>>> Commit message is missing.
>>>
>>> On 26/02/2018 17:59, Lionel Landwerlin wrote:
>>>> v2: Complete invalid cases (Chris)
>>>> Some styling (to_user_pointer, etc...) (Chris)
>>>> New error check, through item.length (Chris)
>>>>
>>>> v3: Update for new uAPI iteration (Lionel)
>>>>
>>>> v4: Return errno from a single point (Chris)
>>>> Poising checks (Chris)
>>>>
>>>> v5: Add more debug traces (Lionel)
>>>> Update uAPI (Joonas/Lionel)
>>>> Make sure Haswell is tested (Lionel)
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>> ---
>>>> tests/Makefile.sources | 1 +
>>>> tests/i915_query.c | 314
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>> tests/meson.build | 1 +
>>>> 3 files changed, 316 insertions(+)
>>>> create mode 100644 tests/i915_query.c
>>>>
>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>>> index 23f859be..b4c8a913 100644
>>>> --- a/tests/Makefile.sources
>>>> +++ b/tests/Makefile.sources
>>>> @@ -168,6 +168,7 @@ TESTS_progs = \
>>>> gen3_render_tiledy_blits \
>>>> gen7_forcewake_mt \
>>>> gvt_basic \
>>>> + i915_query \
>>>
>>> Interesting question how we want to name this test. We don't have
>>> any i915_ prefix tests, but for instance there is
>>> drv_getparams_basic, suggesting this could be called drv_query_ioctl
>>> or something?
>>>
>>> Open for discussion I guess.
>>
>> I've been considering renaming perf.c (that's a pretty confusing one...)
>>
>> One could argue that if IGT tests more than intel devices, we should
>> have a driver prefix (I see vc4 has).
>
> Yes, to an extent, but we got prior use on drv_ so until a grander
> scheme is devised I'd stick with it. But no huge complaints towards
> keeping i915 either, it's not really that important.
>
>>>> +
>>>> + memset(items, 0, sizeof(items));
>>>> + i915_query_item_err(fd, items, 1, EINVAL);
>>>> +
>>>> + memset(items, 0, sizeof(items));
>>>> + items[0].query_id = ULONG_MAX;
>>>> + items[1].query_id = ULONG_MAX - 2;
>>>
>>> What is special about ULONG_MAX - 2 versus ULONG_MAX? And not
>>> ULONG_MAX - 1, or some other number?
>>
>> Nothing really... It's just a really bit number that we're unlikely
>> to reach.
>
> I'd maybe first go with one invalid query id, then second test try two
> different invalid ids. So it is at least more obvious what's happening.
>
> Oh another missed test case: invalid query (in different ways)
> followed by a valid one. First must fail, second succeed. And in
> opposite order as well.
>
>>>> + items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>>>> -1, 0);
>>>> + items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>>> + i915_query_item(fd, items_ptr, 1);
>>>> + igt_assert(items_ptr[0].length >= sizeof(struct
>>>> drm_i915_query_topology_info));
>>>> + munmap(items_ptr, 4096);
>>>> + i915_query_item_err(fd, items_ptr, 1, EFAULT);
>>>
>>> Another good test would be passing in a read only mapping and
>>> checking for EFAULT when length writeback fails.
>>
>> Hm... how to do you write something sensible into a read only mapping
>> so that the kernel would at least try to write to it? :)
>
> Make it read-only after populating it. :) man mprotect.
>
>>>
>>>> +
>>>> + len = sizeof(struct drm_i915_query_item) * 10;
>>>> + items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>>>> -1, 0);
>>>> + for (i = 0; i < 10; i++)
>>>> + items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>>> + ret = __i915_query_item(fd, items_ptr,
>>>> + INT_MAX / sizeof(struct drm_i915_query_item) + 1);
>>>> + igt_assert(ret == -EFAULT || ret == -EINVAL);
>>>
>>> What is this testing? Ten queries, all write only so will EFAULT.
>>> But then you pass in nitems = INT_MAX / sizeof.. which is what
>>> exactly? I don't get it.
>>
>> Trying to get the kernel to read into unmapped address space and
>> expecting an EFAULT or if we're unlucky and it ends up into another
>> mapping invalid query items.
>
> You need to set up the mappings explicitly and not count on luck.
> Create exact sequence of mapped r/w, mapped-r, mapped-w and unmapped
> query items. You can use MAP_FIXED, mremap and mprotect to engineer
> exact layouts to test against.
>
> Helper which processes input arrays like:
>
> struct ... {
> query-id;
> enum { unmapped, ro, rw, w, NULL };
> len;
> expected-result;
> expected-lenght;
> } query_items[] = {
> { TOPOLOGY, rw, ... ,0, .. },
> { TOPOLOGY, ro, ..., -EFAULT, ... },
> { INVALID, rw, ..., -EINVAL, ... },
> { TOPOLOGY, w, ..., -EFAULT, ... },
> { TOPOLOGY, NULL, 0, 0, len },
> };
>
> ret = test_items(fd, query_items, ARRAY_SIZE(query_items));
> igt_assert_eq(ret, 0);
>
> Comes to mind. Very roughly though, so see if it makes sense to you.
>
> Then you can have different query item lists defined like that and
> just feed them to test helper. Even 2d array of test items for this
> class of test scenarios would work so you would just iterate it. Maybe
> store the expected return from the drm_i915_query ioctl for each list.
>
>>>> +static void
>>>> +test_query_topology_coherent_slice_mask(int fd)
>>>> +{
>>>> + struct drm_i915_query_item item;
>>>> + uint8_t *_topo_info;
>>>> + struct drm_i915_query_topology_info *topo_info;
>>>> + drm_i915_getparam_t gp;
>>>> + int slice_mask, subslice_mask;
>>>> + int s, topology_slices, topology_subslices_slice0;
>>>
>>> Not in order of use, or width, never mind.
>>>
>>> Some of the types look like should be unsigned or u32?
>>
>> getparam returns int, so I made the rest the same (since we want to
>> compare them).
>
> igt_assert_eq_u32(int, int) below is what caught my eye. Don't know then.
>
>>>
>>>> +
>>>> + gp.param = I915_PARAM_SLICE_MASK;
>>>> + gp.value = &slice_mask;
>>>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>>> +
>>>> + gp.param = I915_PARAM_SUBSLICE_MASK;
>>>> + gp.value = &subslice_mask;
>>>> + igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>>> +
>>>> + /* Slices */
>>>> + memset(&item, 0, sizeof(item));
>>>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>>> + i915_query_item(fd, &item, 1);
>>>> + igt_assert(item.length > 0);
>>>
>>> Is it possible to check for the exact value here? Or at least
>>> minimum amounting to the v1 query minimum?
>>
>> Can't check for an exact value, it completely depends on the GT size.
>> I guess we can do >= 3 (at least one byte for each section (slice,
>> subslice, eus).
>
> Isnt' sizeof(struct drm_i915_query_topology_info) included in the
> length? So it should be at least that big, no?
Indeed, making a define for it.
>
>>>
>>>> +
>>>> + _topo_info = malloc(3 * item.length);
>>>
>>> Assert on malloc.
>>>
>>> What is 3?
>>
>> Some room on both sides, to verify that the kernel doesn't write
>> outside the bounds.
>
> Yeah figured it out below. Put a comment please.
Done.
>
>>>> +
>>>> + free(_topo_info);
>>>> +}
>>>> +
>>>> +static void
>>>> +test_query_topology_matches_eu_total(int fd)
>>>> +{
>>>> + struct drm_i915_query_item item;
>>>> + struct drm_i915_query_topology_info *topo_info;
>>>> + drm_i915_getparam_t gp;
>>>> + int n_eus, n_eus_topology, s;
>>>> +
>>>> + gp.param = I915_PARAM_EU_TOTAL;
>>>> + gp.value = &n_eus;
>>>> + do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>>> + igt_debug("n_eus=%i\n", n_eus);
>>>> +
>>>> + memset(&item, 0, sizeof(item));
>>>> + item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>>> + i915_query_item(fd, &item, 1);
>>>> +
>>>> + topo_info = (struct drm_i915_query_topology_info *) calloc(1,
>>>> item.length);
>>>> +
>>>> + item.data_ptr = to_user_pointer(topo_info);
>>>> + i915_query_item(fd, &item, 1);
>>>> +
>>>> + igt_debug("max_slices=%hu max_subslices=%hu
>>>> max_eus_per_subslice=%hu\n",
>>>> + topo_info->max_slices, topo_info->max_subslices,
>>>> + topo_info->max_eus_per_subslice);
>>>> + igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
>>>> + topo_info->subslice_offset, topo_info->subslice_stride);
>>>> + igt_debug(" eu_offset=%hu eu_stride=%hu\n",
>>>> + topo_info->eu_offset, topo_info->eu_stride);
>>>> +
>>>> + n_eus_topology = 0;
>>>> + for (s = 0; s < topo_info->max_slices; s++) {
>>>> + int ss;
>>>> +
>>>> + igt_debug("slice%i:\n", s);
>>>> +
>>>> + for (ss = 0; ss < topo_info->max_subslices; ss++) {
>>>> + int eu, n_subslice_eus = 0;
>>>> +
>>>> + igt_debug("\tsubslice: %i\n", ss);
>>>> +
>>>> + igt_debug("\t\teu_mask: 0b");
>>>> + for (eu = 0; eu < topo_info->max_eus_per_subslice;
>>>> eu++) {
>>>> + uint8_t val = eu_available(topo_info, s, ss,
>>>> + topo_info->max_eus_per_subslice - 1 - eu);
>>>
>>> Why is the eu parameter reversed on not simply eu?
>>
>> Because of the printf ordering (starting with most significant bit).
>
> For debug.. grumble. Ok. Hm, but are you printing out more than one u8
> here, or will you be, and will bit ordering be correct then?
>>>> + igt_require(query_topology_supported(fd));
>>>
>>> I am actually not sure if this should be igt_assert or igt_require.
>>> Do we actually want to support running new igts on old kernels, or
>>> make the test stronger by failing when it thinks it should work.
>>> Will need to ask for second opinions.
>>>
>>> It would have skipped already from the fixture if query API is not
>>> supported.
>>
>> I was under the impression that we wanted some kind of old kernel
>> support (like 4.15).
>
> But in fixture the whole thing skip already if there is no query
> support in the kernel. And there will be no kernels with query but no
> topology.
You're right, I'll simplify this.
>
>>>
>>>> + test_query_topology_coherent_slice_mask(fd);
>>>> + }
>>>> +
>>>> + igt_subtest("query-topology-matches-eu-total") {
>>>> + igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>>>> + igt_require(query_topology_supported(fd));
>>>> + test_query_topology_matches_eu_total(fd);
>>>> + }
>>>> +
>>>
>>> Is it possible to add some tests which run on platforms where we can
>>> 100% imply the slice/subslice configuration from the devid and then
>>> verify topology query is returning expected data for all slices etc?
>>
>> That's tricky. I don't know what we actually ship (are any big cores
>> ever fused off in the field? Is it just atoms?)
>> I have a production big core BDW laptop with lots of fused stuff, so
>> my guess is no...
>
> Haswell maybe? According to haswell_sseu_info_init in the kernel
> subslice masks are purely pci id based. EU count wouldn need a
> register read.
slice/subslice count doesn't, but EU count does :
https://cgit.freedesktop.org/drm-intel/commit/?id=b8ec759e6f1c6da0418238df066a0f1ef8fd2075
>
> Bottom line is we could also read fuse registers from IGT and verify
> state between the two.
>
> At least Haswell look easy enough for some basic coverage. I won't say
> we need to do all, but at least we need to look at more than just
> subslice 0.
>
> Regards,
>
> Tvrtko
>
>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-03-08 17:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-26 17:59 [igt-dev] [PATCH i-g-t v5 1/3] include: bump drm uAPI headers Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests Lionel Landwerlin
2018-03-08 11:22 ` Tvrtko Ursulin
2018-03-08 14:31 ` Lionel Landwerlin
2018-03-08 14:44 ` Chris Wilson
2018-03-08 14:52 ` Lionel Landwerlin
2018-03-08 15:47 ` Tvrtko Ursulin
2018-03-08 15:44 ` Tvrtko Ursulin
2018-03-08 17:51 ` Lionel Landwerlin
2018-02-26 17:59 ` [igt-dev] [PATCH i-g-t v5 3/3] tests/pm_sseu: adapt debugfs parsing for newer kernels Lionel Landwerlin
2018-03-07 19:14 ` Chris Wilson
2018-02-26 18:23 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,v5,1/3] include: bump drm uAPI headers Patchwork
2018-02-26 22:28 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.