Re: [PATCH] tests/gem_reloc_overflow: Add gen8+ specifc tests

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 06, 2013 at 07:52:23PM +0000, Barbalho, Rafael wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Wednesday, November 06, 2013 6:52 PM
> > To: Barbalho, Rafael
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] tests/gem_reloc_overflow: Add gen8+
> > specifc tests
> > 
> 
> * snip *
> 
> > > @@ -185,6 +208,36 @@ static void reloc_tests(void)
> > >  		igt_assert(errno == EINVAL);
> > >  	}
> > >
> > > +	igt_fixture {
> > > +		execobjs[0].handle = batch_handle;
> > > +		execobjs[0].relocation_count = 0;
> > > +		execobjs[0].relocs_ptr = 0;
> > > +
> > > +		execbuf.buffer_count = 1;
> > > +	}
> > > +
> > > +	igt_subtest("batch-start-unaligned") {
> > > +		execbuf.batch_start_offset = 1;
> > > +		execbuf.batch_len = 8;
> > > +
> > > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> > &execbuf) != 0);
> > > +		igt_assert(errno == EINVAL);
> > > +	}
> > > +
> > > +	igt_subtest("batch-end-unaligned") {
> > > +		execbuf.batch_start_offset = 0;
> > > +		execbuf.batch_len = 7;
> > > +
> > > +		igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2,
> > &execbuf) != 0);
> > > +		igt_assert(errno == EINVAL);
> > > +	}
> > > +
> > > +	igt_fixture {
> > > +		/* Undo damage for next tests. */
> > > +		execbuf.batch_start_offset = 0;
> > > +		execbuf.batch_len = 8;
> > > +	}
> > 
> > Moving these tests around is tricky - they should be carefully constructed so
> > that the wrong batch start/end is the only thing which is wrong with the
> > metadata, and that with correct start/len (0, 8) it would execute perfectly.
> > 
> > This is to make sure that we really exercise this cornercase and don't get
> > caught in some other check (that originally was done later but then might
> > have moved around).
> > 
> > tldr; Have you check that this is still true?
> 
> When I first moved the test I did try to make sure to move the igt_fixture 
> that set up the state for the tests did the correct thing. Just to make sure that I
> didn't screw things up I've re-run tests with an annotated kernel and it is 
> behaving as expected with the driver returning false from 
> i915_gem_check_execbuffer due to buffer misalignment.
> 
> tldr; Yes.

Cool, thanks for testing. Since I'm paranoid about this I've added an
assert into the relevant fixture block to make sure the batch would really
work safe for the condition we're testing. That way we're maximally robust
against kernel changes that move the tests around.

Also fixed some compile warnings in a follow-up.

Thanks for your patch, applied to i-g-t.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch