Re: [igt-dev] [PATCH i-g-t 4/6] Add i915/gem_ctx_persistence

Andi Shyti
2019-09-03T17:50:34Z

Hi Chris,

just a few quick question from a first look,

> +/**
> + * __gem_context_set_persistence:
> + * @i915: open i915 drm file descriptor
> + * @ctx: i915 context id
> + * @state: desired persistence
> + *
> + * Like __gem_context_set_persistence(), except we assert on failure.
> + */
> +void gem_context_set_persistence(int i915, uint32_t ctx, bool state)
> +{
> + igt_assert_eq(__gem_context_set_persistence(i915, ctx, state), 0);
> +}

Is this really required? We know what comes out of this, it's the
same as calling igt_assert_eq(1, 2); right?

> @@ -58,6 +63,10 @@ int __gem_context_get_param(int fd, struct drm_i915_gem_context_param *p);
> int __gem_context_set_priority(int fd, uint32_t ctx, int prio);
> void gem_context_set_priority(int fd, uint32_t ctx, int prio);
>
> +#define I915_CONTEXT_PARAM_PERSISTENCE 0xb

what if we want to add more parameters in the driver? We would
need to remember to update this as well.

> + gem_context_get_param(i915, &p);
> + expected = !!p.value;
> +
> + expected = !expected;

"expected = !p.value" ?

> + p.value = expected;
> + gem_context_set_param(i915, &p);
> + gem_context_get_param(i915, &p);
> + igt_assert_eq(p.value, expected);

Andi