Re: [PATCH v2 02/38] drm/i915: Provide a hook for selftests

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2017-01-19 at 11:41 +0000, Chris Wilson wrote:
> Some pieces of code are independent of hardware but are very tricky to
> exercise through the normal userspace ABI or via debugfs hooks. Being
> able to create mock unit tests and execute them through CI is vital.
> Start by adding a central point where we can execute unit tests and
> a parameter to enable them. This is disabled by default as the
> expectation is that these tests will occasionally explode.
> 
> To facilitate integration with igt, any parameter beginning with
> i915.igt__ is interpreted as a subtest executable independently via
> igt/drv_selftest.
> 
> Two classes of selftests are recognised: mock unit tests and integration
> tests. Mock unit tests are run as soon as the module is loaded, before
> the device is probed. At that point there is no driver instantiated and
> all hw interactions must be "mocked". This is very useful for writing
> universal tests to exercise code not typically run on a broad range of
> architectures. Alternatively, you can hook into the live selftests and
> run when the device has been instantiated - hw interactions are real.
> 
> v2: Add a macro for compiling conditional code for mock objects inside
> real objects.
> v3: Differentiate between mock unit tests and late integration test.
> v4: List the tests in natural order, use igt to sort after modparam.
> v5: s/late/live/
> v6: s/unsigned long/unsigned int/
> v7: Use igt_ prefixes for long helpers.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1

<SNIP>

> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -3,6 +3,7 @@
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) := -Werror
> +subdir-ccflags-$(CONFIG_DRM_I915_SELFTEST) += -I$(src) -I$(src)/selftests

Similar to drm, add selftests/Makefile, to get rid of this.

> @@ -116,6 +117,9 @@ i915-y += dvo_ch7017.o \
>  
>  # Post-mortem debug and GPU hang state capture
>  i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
> +i915-$(CONFIG_DRM_I915_SELFTEST) += \
> +	selftests/i915_random.o \
> +	selftests/i915_selftest.o
> 

Ditto.

> @@ -499,7 +501,17 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (vga_switcheroo_client_probe_defer(pdev))
>  		return -EPROBE_DEFER;
>  
> -	return i915_driver_load(pdev, ent);
> +	err = i915_driver_load(pdev, ent);
> +	if (err)
> +		return err;
> +
> +	err = i915_live_selftests(pdev);
> +	if (err) {
> +		i915_driver_unload(pci_get_drvdata(pdev));
> +		return err > 0 ? -ENOTTY : err;

What's up with this?
 
>  static void i915_pci_remove(struct pci_dev *pdev)
> @@ -521,6 +533,11 @@ static struct pci_driver i915_pci_driver = {
>  static int __init i915_init(void)
>  {
>  	bool use_kms = true;
> +	int err;
> +
> +	err = i915_mock_selftests();
> +	if (err)
> +		return err > 0 ? 0 : err;

Especially this, is this for skipping the device init completely?

> +int i915_subtests(const char *caller,
> +		  const struct i915_subtest *st,
> +		  unsigned int count,
> +		  void *data);
> +#define i915_subtests(T, data) \
> +	(i915_subtests)(__func__, T, ARRAY_SIZE(T), data)

Argh, why not __i915_subtests like good people do?

> +/* Using the i915_selftest_ prefix becomes a little unwieldy with the helpers.
> + * Instead we use the igt_ shorthand, in reference to the intel-gpu-tools
> + * suite of uabi test cases (which includes a test runner for our selftests).
> + */

I'd ask for an ack from Daniel/Jani on this.

> +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);
> +}

Upstream material. Also I remember this stuff is in DRM too, so I
assume you cleanly copy pasted, and skip this randomization code.

> +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c

<SNIP>

> +/* Embed the line number into the parameter name so that we can order tests */
> +#define selftest(n, func) selftest_0(n, func, param(n))
> +#define param(n) __PASTE(igt__, __PASTE(__PASTE(__LINE__, __), mock_##n))

Hmm, you could reduce one __PASTE by making the ending __mock_##n?

> +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();

You know that in theory this might take an eternity. I'm not sure why
zero is not OK after this point?

> +
> +	i915_selftest.timeout_jiffies =
> +		i915_selftest.timeout_ms ?
> +		msecs_to_jiffies_timeout(i915_selftest.timeout_ms) :
> +		MAX_SCHEDULE_TIMEOUT;

You had a default value for the variable too, I guess that's not needed
now, and gets some bytes off .data.

> +
> +	set_default_test_all(st, count);
> +
> +	pr_info("i915: 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("i915: Running %s\n", st->name);

I guess we shouldn't be hardcoding "i915" in strings.

> +		if (data)
> +			err = st->live(data);
> +		else
> +			err = st->mock();

I'd newline here.

> +		if (err == -EINTR && !signal_pending(current))
> +			err = 0;
> +		if (err)
> +			break;
> +	}
> +
> +	if (WARN(err > 0 || err == -ENOTTY,
> +		 "%s returned %d, conflicting with selftest's magic values!\n",
> +		 st->name, err))
> +		err = -1;
> +
> +	rcu_barrier();

Our tests themselves use no RCU, so at least drop a comment here,
internal driver implementation seems to bleed here.

> +	return err;
> +}
> +
> +#define run_selftests(x, data) \
> +	(run_selftests)(#x, x##_selftests, ARRAY_SIZE(x##_selftests), data)

Nooooo....

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx