[Intel-gfx] [PATCH 05/46] drm/i915: Provide a hook for selftests

[Chris Wilson]

On Fri, Feb 10, 2017 at 10:19:01AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2017 09:08, Chris Wilson wrote:
> >+static inline u32 i915_prandom_u32_max_state(u32 ep_ro, struct rnd_state *state)
> >+{
> >+	return upper_32_bits((u64)prandom_u32_state(state) * ep_ro);
> 
> What is ep_ro?

"right open interval endpoint"

And now I wish I didn't.

> >+void i915_random_reorder(unsigned int *order, unsigned int count,
> >+			 struct rnd_state *state)
> >+{
> >+	unsigned int i, j;
> >+
> >+	for (i = 0; i < count; ++i) {
> >+		BUILD_BUG_ON(sizeof(unsigned int) > sizeof(u32));
> 
> ? :)

Future proofing? A statement of the assumption that the prng can return
a value in the right range.

> >+static int __run_selftests(const char *name,
> >+			   struct selftest *st,
> >+			   unsigned int count,
> >+			   void *data)
> >+{
> >+	int err = 0;
> >+
> >+	while (!i915_selftest.random_seed)
> >+		i915_selftest.random_seed = get_random_int();
> >+
> >+	i915_selftest.timeout_jiffies =
> >+		i915_selftest.timeout_ms ?
> >+		msecs_to_jiffies_timeout(i915_selftest.timeout_ms) :
> >+		MAX_SCHEDULE_TIMEOUT;
> >+
> >+	set_default_test_all(st, count);
> >+
> >+	pr_info(DRIVER_NAME ": Performing %s selftests with st_random_seed=0x%x st_timeout=%u\n",
> >+		name, i915_selftest.random_seed, i915_selftest.timeout_ms);
> >+
> >+	/* Tests are listed in order in i915_*_selftests.h */
> >+	for (; count--; st++) {
> >+		if (!st->enabled)
> >+			continue;
> >+
> >+		cond_resched();
> >+		if (signal_pending(current))
> >+			return -EINTR;
> >+
> >+		pr_debug(DRIVER_NAME ": Running %s\n", st->name);
> >+		if (data)
> >+			err = st->live(data);
> >+		else
> >+			err = st->mock();
> >+		if (err == -EINTR && !signal_pending(current))
> 
> What is the expected use for this?

ctrl-c escaping of tests is self-explanatory. However, I adopted the
convention of using EINTR to mean both escape due to timeout, and due to
user interruption. The convenience of only having to check one errno
inside the tests, but here at the user interface, I did want to
distinguish between testing running to timeout as being normal, and
responding back to the user interruption with EINTR.

> >+			err = 0;
> >+		if (err)
> >+			break;
> >+	}
> >+module_param_named(st_random_seed, i915_selftest.random_seed, uint, 0400);
> >+module_param_named(st_timeout, i915_selftest.timeout_ms, uint, 0400);
> 
> I think selftest_ prefix would be better to match the ones below and
> just in general.
> 
> >+
> >+module_param_named_unsafe(mock_selftests, i915_selftest.mock, int, 0400);
> >+MODULE_PARM_DESC(mock_selftests, "Run selftests before loading, using mock hardware (0:disabled [default], 1:run tests then load driver, -1:run tests then exit module)");
> >+
> >+module_param_named_unsafe(live_selftests, i915_selftest.live, int, 0400);
> >+MODULE_PARM_DESC(live_selftests, "Run selftests after driver initialisation on the live system (0:disabled [default], 1:run tests then continue, -1:run tests then exit module)");
> >diff --git a/tools/testing/selftests/drivers/gpu/i915.sh b/tools/testing/selftests/drivers/gpu/i915.sh
> >index d407f0fa1e3a..c06d6e8a8dcc 100755
> >--- a/tools/testing/selftests/drivers/gpu/i915.sh
> >+++ b/tools/testing/selftests/drivers/gpu/i915.sh
> >@@ -7,6 +7,7 @@ if ! /sbin/modprobe -q -r i915; then
> > fi
> >
> > if /sbin/modprobe -q i915 mock_selftests=-1; then
> >+	/sbin/modprobe -q -r i915
> > 	echo "drivers/gpu/i915: ok"
> > else
> > 	echo "drivers/gpu/i915: [FAIL]"
> >
> 
> No serious complaints. Bikeshed or not (but preferred for modparams):

Hmm. They get very long, very quickly. I print them out on each run (so
that we can just copy'n'paste if we need to replay an exact set of
parameters) - they just look ugly. :|

I'll throw it to the bikeshed committee, if people are in favour I'll
switch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre