Re: [Intel-gfx] [PATCH igt] lib: Fix MI_BATCH_BUFFER_START for hang injection

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 02, 2018 at 04:13:46PM +0000, Chris Wilson wrote:
> A couple of bugs inside the hang injector, the worst being that the
> presumed_offset of the reloc didn't match the batch; so if the reloc was
> skipped (as the presumed_offset matched the reloc offset), the batch
> wasn't updated and so we may not have generated a hanging batch at all!
> Secondly, the MI_BATCH_BUFFER_START was not correct for all gen.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  lib/igt_gt.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index e630550b..799ca1ae 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -276,6 +276,7 @@ igt_hang_t igt_hang_ctx(int fd,
>  	uint32_t b[16];
>  	unsigned ban;
>  	unsigned len;
> +	int gen;
>  
>  	igt_require_hang_ring(fd, ring);
>  
> @@ -310,12 +311,26 @@ igt_hang_t igt_hang_ctx(int fd,
>  
>  	memset(b, 0xc5, sizeof(b));
>  
> -	len = 2;
> -	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> +	len = 0;
> +	gen = intel_gen(intel_get_drm_devid(fd));
> +	if (gen >= 8) {
> +		b[len++] = MI_BATCH_BUFFER_START | 1 << 8 | 1;
> +		b[len++] = 0;
> +		b[len++] = 0;
> +	} else if (gen >= 6) {
> +		b[len++] = MI_BATCH_BUFFER_START | 1 << 8;
> +		b[len++] = 0;

ppgtt for gen6+

> +	} else {
> +		b[len++] = MI_BATCH_BUFFER_START | 2 << 6;
> +		b[len] = 0;
> +		if (gen < 4) {
> +			b[len] |= 1;
> +			reloc.delta = 1;
> +		}
>  		len++;

gtt secure for gen4/5
gtt non-secure for gen2/3

How does the security thing work on ctg/ilk for chained batches? The spec
is a wee bit unclear. It says the security bit for chained batches is
ignored, but then it also says non-secure batches can't access the gtt.
That was the case for MI_STORE_DWORD if I recall your earlier patch
correctly. So if we don't execute the first batch as secure the chained
MI_BB_START gets nopped out maybe?

Hmm. Now I wonder how the earlier MI_STORE_DWORD thing worked on pre-ctg
with a non-secure batch? Wasn't the hardware supposed to nop those out
entirely? /me confused

Anyways the new code looks at least more correct than the old one so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -	b[0] = MI_BATCH_BUFFER_START | (len - 2);
> -	b[len] = MI_BATCH_BUFFER_END;
> -	b[len+1] = MI_NOOP;
> +	}
> +	b[len++] = MI_BATCH_BUFFER_END;
> +	b[len] = MI_NOOP;
>  	gem_write(fd, exec.handle, 0, b, sizeof(b));
>  
>  	reloc.offset = sizeof(uint32_t);
> @@ -364,8 +379,7 @@ void igt_post_hang_ring(int fd, igt_hang_t arg)
>  	if (arg.handle == 0)
>  		return;
>  
> -	gem_set_domain(fd, arg.handle,
> -		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +	gem_sync(fd, arg.handle);
>  	gem_close(fd, arg.handle);
>  
>  	context_set_ban(fd, arg.ctx, arg.ban);
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx