Re: [PATCH i-g-t 2/2] Add i915/gem_ctx_persistence

Andi Shyti
2019-10-15T13:05:46Z

Hi Chris,

On Thu, Oct 10, 2019 at 08:32:58AM +0100, Chris Wilson wrote:
> Sanity test existing persistence and new exciting non-persistent context
> behaviour.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> ---
> lib/i915/gem_context.c | 37 +++
> lib/i915/gem_context.h | 8 +
> lib/igt_dummyload.c | 3 +-
> lib/ioctl_wrappers.c | 1 +
> tests/Makefile.sources | 3 +
> tests/i915/gem_ctx_persistence.c | 407 +++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 7 files changed, 459 insertions(+), 1 deletion(-)
> create mode 100644 tests/i915/gem_ctx_persistence.c

I think this patch should be split as ioctl_wrappers,
igt_dummyload have changes that are not related to "Sanity test
existing persistence and new exciting non-persistent context
behaviour"

I think they don't affect the test itself.

Without those changes:

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Still one question.

> +static bool enable_hangcheck(int i915)

how about adding here a boolean and use this function also for
the test_nohangcheck_hostile() case?

> +{
> + int enabled = -1;
> + int dir;
> +
> + dir = igt_sysfs_open_parameters(i915);
> + if (dir < 0) /* no parameters, must be default! */
> + return enabled;
> +
> + /* If i915.hangcheck is removed, assume the default is good */
> + igt_sysfs_set(dir, "enable_hangcheck", "1");
> + igt_sysfs_scanf(dir, "enable_hangcheck", "%d", &enabled);
> +
> + close(dir);
> +
> + return enabled;
> +}
> +
> +static void test_idempotent(int i915)
> +{
> + struct drm_i915_gem_context_param p = {
> + .param = I915_CONTEXT_PARAM_PERSISTENCE,
> + };
> + int expected;
> +
> + /*
> + * Simple test to verify that we are able to read back the same boolean
> + * value as we set.
> + *
> + * Each time we invert the current value so that at the end of the test,
> + * if successful, we leave the context in the original state.
> + */
> +
> + gem_context_get_param(i915, &p);
> + expected = !!p.value;
> +
> + expected = !expected;

mmhhh... looks familiar :)

Andi