Re: [PATCH igt] igt/gem_ctx_isolation: Check isolation of registers between contexts

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

On Fri, 2017-12-01 at 11:13 +0000, Chris Wilson wrote:
> A new context assumes that all of its registers are in the default state
> when it is created. What may happen is that a register written by one
> context may leak into the second, causing mass confusion.
> 
> v2: Extend back to Sandybridge (etc)
> v3: Check context preserves registers across suspend/hibernate and resets.
> v4: Complete the remapping onto the new class:instance
> v5: Not like that, like this, try again to use class:instance
> v6: Prepare for retrospective gen4 contexts!
> v7: Repaint register set name to nonpriv, as this is what bspec calls the
> registers that are writable by userspace.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

<SNIP>

> +static uint32_t read_regs(int fd,
> +			  uint32_t ctx,
> +			  const struct intel_execution_engine2 *e,
> +			  unsigned int flags)
> +{
> +	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> +	const unsigned int gen_bit = 1 << gen;

You did define BIT() in the beginning...

<SNIP>

> +	n = 0;
> +	for (const struct named_register *r = nonpriv_registers; r->name; r++) {
> +		if (!(r->engine_mask & engine_bit))
> +			continue;
> +		if (!(r->gen_mask & gen_bit))
> +			continue;
> +
> +		for (unsigned count = r->count ?: 1, offset = r->offset;
> +		     count--; offset += 4) {
> +			*b++ = 0x24 << 23 | (1 + r64b); /* SRM */
> +			*b++ = offset;
> +			reloc[n].target_handle = obj[0].handle;

I still kind of loathe the obj[0] vs obj[1] when it's not going to be
about comparing the two. obj_regs and obj_batch is just requires so
much fewer scroll-ups to see what was 0 and what was 1...

> +			reloc[n].presumed_offset = 0;
> +			reloc[n].offset = (b - batch) * sizeof(*b);
> +			reloc[n].delta = offset;

This might confuse somebody. At least call the variable r_offset (or
reg_offset).

> +			reloc[n].read_domains = I915_GEM_DOMAIN_RENDER;
> +			reloc[n].write_domain = I915_GEM_DOMAIN_RENDER;
> +			*b++ = offset;
> +			if (r64b)
> +				*b++ = 0;
> +			n++;

I can't see why this is not either inside the for () or bound closer to
filling the relocation, here it's bit of a "what?".

<SNIP>

> +static void restore_regs(int fd,
> +			 uint32_t ctx,
> +			 const struct intel_execution_engine2 *e,
> +			 unsigned int flags,
> +			 uint32_t regs)
> +{
> +	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> +	const unsigned int gen_bit = 1 << gen;
> +	const unsigned int engine_bit = ENGINE(e->class, e->instance);
> +	const bool r64b = gen >= 8;
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_relocation_entry *reloc;
> +	unsigned int batch_size, n;
> +	uint32_t *batch, *b;
> +
> +	if (gen < 7) /* no LRM */
> +		return;

Considering this restriction, would not it be possible to just CPU
mmap and have this as a bunch of LRIs?

<SNIP>

> +static void preservation(int fd,
> +			 const struct intel_execution_engine2 *e,
> +			 unsigned int flags)
> +{
> +	static const uint32_t values[] = {
> +		0x0,
> +		0xffffffff,
> +		0xcccccccc,
> +		0x33333333,
> +		0x55555555,
> +		0xaaaaaaaa,
> +		0xdeadbeef
> +	};
> +	const unsigned int num_values = ARRAY_SIZE(values);
> +	unsigned int engine =
> +		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	uint32_t ctx[num_values +1 ];

"+ 1"

<SNIP>

> +static unsigned int __has_context_isolation(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int value = 0;
> +
> +	memset(&gp, 0, sizeof(gp));
> +	gp.param = 50; /* I915_PARAM_HAS_CONTEXT_ISOLATION */
> +	gp.value = &value;
> +
> +	igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	errno = 0;
> +
> +	return value;
> +}

<SNIP>

> +++ b/tests/gem_exec_fence.c
> @@ -698,7 +698,7 @@ static bool has_submit_fence(int fd)
>  	int value = 0;
>  
>  	memset(&gp, 0, sizeof(gp));
> -	gp.param = 50; /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
> +	gp.param = 0xdeadbeef ^ 51; /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
>  	gp.value = &value;
>  
>  	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));

Probably wan't to sort the param stuff out :)

With the params corrected, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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