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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> - 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
