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@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> #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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
