[Intel-gfx] [igt-dev] [RFC PATCH i-g-t v3] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

On Wednesday, October 30, 2019 10:19:43 PM CET Vanshidhar Konda wrote:
> On Wed, Oct 30, 2019 at 06:15:35PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish actually occupied addresses from otherwise invalid
> >softpin offsets.  For example, on a future hardware backing store with
> >a page size larger than 4 kB incorrect object alignment is assumed and
> >the test results are distorted as it happily skips over incorrectly
> >aligned objects instead of reporting the problem.
> >
> >Filter out failing addresses only if not reported as invalid.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> >Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index fdd9661d..1d0c791e 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> 
> The addresses to which the object is being pinned is generated as part
> of the test. The code is just assuming that the address needs to be 4K
> aligned instead of figuring out what the alignment requirement for the
> device is.
> 
> Shouldn't the test be updated to generate virtual addresses per the
> alignment requirements of the test device instead of just assuming 4K
> increments are good?

You're perfectly right, and that's what I've been trying to achieve in my 
series https://lists.freedesktop.org/archives/igt-dev/2019-October/017081.html.  
As suggested by Chris (https://lists.freedesktop.org/archives/igt-dev/2019-October/016936.html), 
I've been trying to add a library function that detects the alignment
requirement of a device.  We haven't agreed yet on necessity of my approach to 
distinguish failures caused by incorrect offset alignment from those which are 
simply coming from addresses being occupied by other users, and how to do 
that distinction.  In my current approach, I'm retrying at different offsets 
to conclude possible failure reasons, but maybe error codes can be used for 
that.

Back to this patch, skipping over invalid offsets, calculated from incorrectly 
assumed or detected alignment requirements still seems wrong to me, anyway.  
That's the reason for this patch.

Thanks,
Janusz


> 
> Vanshi
> 
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
> >@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
>