[PATCH i-g-t v2 0/2] Kernel selftest plumbing

[PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Petri Latvala]

v2 of the series with miscellaneous fixes.

IGT subtest enumeration must work regardless of what the running
environment is. Kernel selftest launchers want to expose everything
the running kernel can execute.

These two things are mutually exclusive. This series is an attempt for
a best-of-both-worlds approach: After constructing subtests for
selftests the running kernel supports (if any), go through the list of
known kernel selftests and set up a subtest for any such test that
is missing.

This will still violate the IGT rule of list enumeration on tests that
are not present in the selftest headers copied from the kernel
sources, but we can live with that. Generating static testlists for CI
use is now possible, and running new selftests without IGT changes
stays possible.

Petri Latvala (2):
  lib: Add i915 and drm-mm selftest headers from the kernel
  Always expose IGT subtests for known kernel selftests

 lib/Makefile.sources      |  3 +++
 lib/drm_mm_selftests.h    | 24 ++++++++++++++++++++++
 lib/i915_live_selftests.h | 19 +++++++++++++++++
 lib/i915_mock_selftests.h | 20 ++++++++++++++++++
 lib/igt_kmod.c            | 52 ++++++++++++++++++++++++++++++++++-------------
 lib/igt_kmod.h            |  8 +++++++-
 tests/drm_mm.c            | 17 +++++++++++++++-
 tests/drv_selftest.c      | 25 +++++++++++++++++++++--
 8 files changed, 150 insertions(+), 18 deletions(-)
 create mode 100644 lib/drm_mm_selftests.h
 create mode 100644 lib/i915_live_selftests.h
 create mode 100644 lib/i915_mock_selftests.h

-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 1/2] lib: Add i915 and drm-mm selftest headers from the kernel

[Petri Latvala]

Copied as of commit

 commit 496b575e3ccbf6fbe57a674c721af43dc8826361
 Author: Chris Wilson <chris@chris-wilson.co.uk>
 Date:   Mon Feb 13 17:15:58 2017 +0000

    drm/i915: Add initial selftests for hang detection and resets

The headers are used to enumerate available tests when the running
kernel does not support selftests.

v2: Also add them to Makefile.sources

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/Makefile.sources      |  3 +++
 lib/drm_mm_selftests.h    | 24 ++++++++++++++++++++++++
 lib/i915_live_selftests.h | 19 +++++++++++++++++++
 lib/i915_mock_selftests.h | 20 ++++++++++++++++++++
 4 files changed, 66 insertions(+)
 create mode 100644 lib/drm_mm_selftests.h
 create mode 100644 lib/i915_live_selftests.h
 create mode 100644 lib/i915_mock_selftests.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 6348487..151c3d9 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -83,6 +83,9 @@ lib_source_list =	 	\
 	uwildmat/uwildmat.c	\
 	igt_kmod.c		\
 	igt_kmod.h		\
+	drm_mm_selftests.h      \
+	i915_live_selftests.h   \
+	i915_mock_selftests.h   \
 	$(NULL)
 
 if HAVE_CHAMELIUM
diff --git a/lib/drm_mm_selftests.h b/lib/drm_mm_selftests.h
new file mode 100644
index 0000000..37bbdac
--- /dev/null
+++ b/lib/drm_mm_selftests.h
@@ -0,0 +1,24 @@
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_mm
+ */
+selftest(sanitycheck, igt_sanitycheck) /* keep first (selfcheck for igt) */
+selftest(init, igt_init)
+selftest(debug, igt_debug)
+selftest(reserve, igt_reserve)
+selftest(insert, igt_insert)
+selftest(replace, igt_replace)
+selftest(insert_range, igt_insert_range)
+selftest(align, igt_align)
+selftest(align32, igt_align32)
+selftest(align64, igt_align64)
+selftest(evict, igt_evict)
+selftest(evict_range, igt_evict_range)
+selftest(bottomup, igt_bottomup)
+selftest(topdown, igt_topdown)
+selftest(color, igt_color)
+selftest(color_evict, igt_color_evict)
+selftest(color_evict_range, igt_color_evict_range)
diff --git a/lib/i915_live_selftests.h b/lib/i915_live_selftests.h
new file mode 100644
index 0000000..18b174d
--- /dev/null
+++ b/lib/i915_live_selftests.h
@@ -0,0 +1,19 @@
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as subtest__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * The function should be of type int function(void). It may be conditionally
+ * compiled using #if IS_ENABLED(DRM_I915_SELFTEST).
+ *
+ * Tests are executed in order by igt/drv_selftest
+ */
+selftest(sanitycheck, i915_live_sanitycheck) /* keep first (igt selfcheck) */
+selftest(uncore, intel_uncore_live_selftests)
+selftest(requests, i915_gem_request_live_selftests)
+selftest(objects, i915_gem_object_live_selftests)
+selftest(dmabuf, i915_gem_dmabuf_live_selftests)
+selftest(coherency, i915_gem_coherency_live_selftests)
+selftest(gtt, i915_gem_gtt_live_selftests)
+selftest(contexts, i915_gem_context_live_selftests)
+selftest(hangcheck, intel_hangcheck_live_selftests)
diff --git a/lib/i915_mock_selftests.h b/lib/i915_mock_selftests.h
new file mode 100644
index 0000000..be9a9eb
--- /dev/null
+++ b/lib/i915_mock_selftests.h
@@ -0,0 +1,20 @@
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as subtest__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * The function should be of type int function(void). It may be conditionally
+ * compiled using #if IS_ENABLED(DRM_I915_SELFTEST).
+ *
+ * Tests are executed in order by igt/drv_selftest
+ */
+selftest(sanitycheck, i915_mock_sanitycheck) /* keep first (igt selfcheck) */
+selftest(scatterlist, scatterlist_mock_selftests)
+selftest(uncore, intel_uncore_mock_selftests)
+selftest(breadcrumbs, intel_breadcrumbs_mock_selftests)
+selftest(requests, i915_gem_request_mock_selftests)
+selftest(objects, i915_gem_object_mock_selftests)
+selftest(dmabuf, i915_gem_dmabuf_mock_selftests)
+selftest(vma, i915_vma_mock_selftests)
+selftest(evict, i915_gem_evict_mock_selftests)
+selftest(gtt, i915_gem_gtt_mock_selftests)
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 2/2] Always expose IGT subtests for known kernel selftests

[Petri Latvala]

Even when the running kernel does not support selftests, make subtest
enumeration list known kselftests. The list is generated using
selftest listing headers copied from the kernel.

If the running kernel gains new selftest subtests, they are listed
even without copying the headers over and rebuilding IGT.

v2: Use correct names for the testcases

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/igt_kmod.c       | 52 ++++++++++++++++++++++++++++++++++++++--------------
 lib/igt_kmod.h       |  8 +++++++-
 tests/drm_mm.c       | 17 ++++++++++++++++-
 tests/drv_selftest.c | 25 +++++++++++++++++++++++--
 4 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 4b8ea81..86b7896 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -493,30 +493,54 @@ void igt_kselftest_fini(struct igt_kselftest *tst)
 	kmod_module_unref(tst->kmod);
 }
 
+static void mocklist_mark_entry(struct igt_kselftest_mockentry *mocklist,
+				const char *name)
+{
+	struct igt_kselftest_mockentry *me;
+
+	for (me = mocklist; me->name != NULL; ++me) {
+		if (!strcmp(me->name, name)) {
+			me->do_mock = false;
+			return;
+		}
+	}
+}
+
 void igt_kselftests(const char *module_name,
 		    const char *options,
 		    const char *result,
-		    const char *filter)
+		    const char *filter,
+		    struct igt_kselftest_mockentry *mocklist)
 {
 	struct igt_kselftest tst;
 	IGT_LIST(tests);
 	struct igt_kselftest_list *tl, *tn;
+	struct igt_kselftest_mockentry *me;
 
-	igt_require(igt_kselftest_init(&tst, module_name) == 0);
-	igt_fixture
-		igt_require(igt_kselftest_begin(&tst) == 0);
+	if (igt_kselftest_init(&tst, module_name) == 0) {
+		igt_fixture
+			igt_require(igt_kselftest_begin(&tst) == 0);
 
-	igt_kselftest_get_tests(tst.kmod, filter, &tests);
-	igt_list_for_each_safe(tl, tn, &tests, link) {
-		igt_subtest_f("%s", tl->name)
-			igt_kselftest_execute(&tst, tl, options, result);
-		free(tl);
-	}
+		igt_kselftest_get_tests(tst.kmod, filter, &tests);
+		igt_list_for_each_safe(tl, tn, &tests, link) {
+			igt_subtest_f("%s", tl->name)
+				igt_kselftest_execute(&tst, tl, options, result);
+			mocklist_mark_entry(mocklist, tl->name);
+			free(tl);
+		}
 
-	igt_fixture {
-		igt_kselftest_end(&tst);
-		igt_require(!igt_list_empty(&tests));
+		igt_fixture {
+			igt_kselftest_end(&tst);
+			igt_require(!igt_list_empty(&tests));
+		}
+
+		igt_kselftest_fini(&tst);
 	}
 
-	igt_kselftest_fini(&tst);
+	/* Expose subtests for anything the above didn't already generate. */
+	for (me = mocklist; me->name != NULL; ++me) {
+		if (me->do_mock)
+			igt_subtest(me->name)
+				igt_skip("Kernel selftest not present");
+	}
 }
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index 10cd3a2..d49e9d4 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -35,10 +35,16 @@ int igt_kmod_unload(const char *mod_name, unsigned int flags);
 int igt_i915_driver_load(const char *opts);
 int igt_i915_driver_unload(void);
 
+struct igt_kselftest_mockentry {
+	const char *name;
+	bool do_mock;
+};
+
 void igt_kselftests(const char *module_name,
 		    const char *module_options,
 		    const char *result_option,
-		    const char *filter);
+		    const char *filter,
+		    struct igt_kselftest_mockentry* mocklist);
 
 struct igt_kselftest {
 	struct kmod_module *kmod;
diff --git a/tests/drm_mm.c b/tests/drm_mm.c
index 2052b11..2a3212e 100644
--- a/tests/drm_mm.c
+++ b/tests/drm_mm.c
@@ -28,5 +28,20 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range manager (struct drm_mm)"
 
 igt_main
 {
-	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
+	/*
+	 * Set of subtest names that are always exposed by IGT,
+	 * regardless of the running kernel's capabilities. Selftests
+	 * that the kernel has but are not on these lists are also
+	 * exposed. This is a known intentional violation of the
+	 * general rule that subtest enumeration must not change
+	 * depending on the runtime environment.
+	 */
+	struct igt_kselftest_mockentry drm_mm_testlist[] = {
+#define selftest(n, x) { .name = #n, .do_mock = true },
+#include "drm_mm_selftests.h"
+#undef selftest
+		{ NULL, false }
+	};
+
+	igt_kselftests("test-drm_mm", NULL, NULL, NULL, drm_mm_testlist);
 }
diff --git a/tests/drv_selftest.c b/tests/drv_selftest.c
index 96dd8bf..d9b58d5 100644
--- a/tests/drv_selftest.c
+++ b/tests/drv_selftest.c
@@ -28,6 +28,27 @@ IGT_TEST_DESCRIPTION("Basic unit tests for i915.ko");
 
 igt_main
 {
-	igt_kselftests("i915", "mock_selftests=-1", NULL, "mock");
-	igt_kselftests("i915", "live_selftests=-1", "live_selftests", "live");
+	/*
+	 * Set of subtest names that are always exposed by IGT,
+	 * regardless of the running kernel's capabilities. Selftests
+	 * that the kernel has but are not on these lists are also
+	 * exposed. This is a known intentional violation of the
+	 * general rule that subtest enumeration must not change
+	 * depending on the runtime environment.
+	 */
+	struct igt_kselftest_mockentry i915_mock_testlist[] = {
+#define selftest(n, x) { .name = "mock_" #n, .do_mock = true },
+#include "i915_mock_selftests.h"
+#undef selftest
+		{ NULL, false }
+	};
+	struct igt_kselftest_mockentry i915_live_testlist[] = {
+#define selftest(n, x) { .name = "live_" #n, .do_mock = true },
+#include "i915_live_selftests.h"
+#undef selftest
+		{ NULL, false }
+	};
+
+	igt_kselftests("i915", "mock_selftests=-1", NULL, "mock", i915_mock_testlist);
+	igt_kselftests("i915", "live_selftests=-1", "live_selftests", "live", i915_live_testlist);
 }
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Chris Wilson]

On Mon, Mar 13, 2017 at 12:43:56PM +0200, Petri Latvala wrote:
> v2 of the series with miscellaneous fixes.
> 
> IGT subtest enumeration must work regardless of what the running
> environment is. Kernel selftest launchers want to expose everything
> the running kernel can execute.
> 
> These two things are mutually exclusive. This series is an attempt for
> a best-of-both-worlds approach: After constructing subtests for
> selftests the running kernel supports (if any), go through the list of
> known kernel selftests and set up a subtest for any such test that
> is missing.
> 
> This will still violate the IGT rule of list enumeration on tests that
> are not present in the selftest headers copied from the kernel
> sources, but we can live with that. Generating static testlists for CI
> use is now possible, and running new selftests without IGT changes
> stays possible.

Still completely lacking justification. The above is a non sequitur;
static testlist generation can be done just from the source.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Petri Latvala]

On Mon, Mar 13, 2017 at 10:50:04AM +0000, Chris Wilson wrote:
> Still completely lacking justification. The above is a non sequitur;
> static testlist generation can be done just from the source.

Static testlist generation can be done and it breaks when run on a
kernel without selftests enabled.

make distcheck fails when running on a kernel without selftests
enabled. It's frankly surprising that CI has been able to build IGT at
all.

I don't know what you mean by "just from the source"? With this series
the subtest enumeration is done "from the source", which wasn't
possible before. It doesn't change behaviour from before when run with
selftests enabled. It makes other machinery work again, the ones that
expect subtest enumeration to work regardless of the runtime
environment. Changing _that_ rule with the kselftest runners is the
one change that is without justification.


--
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Chris Wilson]

On Mon, Mar 13, 2017 at 01:02:04PM +0200, Petri Latvala wrote:
> On Mon, Mar 13, 2017 at 10:50:04AM +0000, Chris Wilson wrote:
> > Still completely lacking justification. The above is a non sequitur;
> > static testlist generation can be done just from the source.
> 
> Static testlist generation can be done and it breaks when run on a
> kernel without selftests enabled.

That doesn't make sense. static is a static list, as simple as using sed
on the headers.
 
> make distcheck fails when running on a kernel without selftests
> enabled. It's frankly surprising that CI has been able to build IGT at
> all.

Which is just igt having a laugh.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Petri Latvala]

On Mon, Mar 13, 2017 at 02:15:34PM +0000, Chris Wilson wrote:
> On Mon, Mar 13, 2017 at 01:02:04PM +0200, Petri Latvala wrote:
> > On Mon, Mar 13, 2017 at 10:50:04AM +0000, Chris Wilson wrote:
> > > Still completely lacking justification. The above is a non sequitur;
> > > static testlist generation can be done just from the source.
> > 
> > Static testlist generation can be done and it breaks when run on a
> > kernel without selftests enabled.
> 
> That doesn't make sense. static is a static list, as simple as using sed
> on the headers.

Without this series, having the selftests in a .testlist with piglit results in

 KeyError: 'igt@drv_selftest@mock_sanitycheck'

et al.

"But we can fix piglit to ignore that error!"

Sure, and that's on the way, but we're not there yet.

Then when we add testlist checking to IGT to notify developers that
the test they removed/renamed was in a testlist file and the testlist
should be edited, we also hardcode drm_mm, drv_selftest and
$whatever-is-in-future to not be checked?

When generating documentation, we ignore that the docs for drm_mm and
drv_selftest generate different documents depending on where they are
built?

We also hardcode igt_command_line.sh to ignore errors with drm_mm and
drv_selftest?

Or, just maybe, we could follow the convention of having subtest
enumeration always work regardless of environment, by always exposing
certain test names?

Why the rules don't apply to kselftest launchers?



--
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Petri Latvala]

For the record, I pushed this series so I could get the IGT release
out. I'm sure there are various ways this functionality can be
improved, so constructive suggestions are still definitely welcome.



--
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 0/2] Kernel selftest plumbing

[Chris Wilson]

On Mon, Mar 13, 2017 at 04:29:47PM +0200, Petri Latvala wrote:
> On Mon, Mar 13, 2017 at 02:15:34PM +0000, Chris Wilson wrote:
> > On Mon, Mar 13, 2017 at 01:02:04PM +0200, Petri Latvala wrote:
> > > On Mon, Mar 13, 2017 at 10:50:04AM +0000, Chris Wilson wrote:
> > > > Still completely lacking justification. The above is a non sequitur;
> > > > static testlist generation can be done just from the source.
> > > 
> > > Static testlist generation can be done and it breaks when run on a
> > > kernel without selftests enabled.
> > 
> > That doesn't make sense. static is a static list, as simple as using sed
> > on the headers.
> 
> Without this series, having the selftests in a .testlist with piglit results in
> 
>  KeyError: 'igt@drv_selftest@mock_sanitycheck'
> 
> et al.
> 
> "But we can fix piglit to ignore that error!"
> 
> Sure, and that's on the way, but we're not there yet.
> 
> Then when we add testlist checking to IGT to notify developers that
> the test they removed/renamed was in a testlist file and the testlist
> should be edited, we also hardcode drm_mm, drv_selftest and
> $whatever-is-in-future to not be checked?
> 
> When generating documentation, we ignore that the docs for drm_mm and
> drv_selftest generate different documents depending on where they are
> built?
> 
> We also hardcode igt_command_line.sh to ignore errors with drm_mm and
> drv_selftest?

Nope. jst igt_comand_line.sh is enforcing a workaround for piglit that
does not affect igt itself.
 
> Or, just maybe, we could follow the convention of having subtest
> enumeration always work regardless of environment, by always exposing
> certain test names?

Go read the tests and see how broken that concept is.
 
> Why the rules don't apply to kselftest launchers?

What rules? All I see above is piglit is broken. Why should we
workaround bugs in piglit?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx