[igt-dev] [PATCH i-g-t v5 2/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Vanshidhar Konda]

On Mon, Nov 04, 2019 at 06:13:28PM +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 between actually occupied addresses and otherwise
>invalid softpin offsets.  If incorrect GTT alignment will be assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if reported as occupied.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>v4: Silently skip only those offsets which have been explicitly
>    reported as overlapping with shared GTT reserved space, not simply
>    all which raise failures other than -EINVAL (Chris),
>  - as an implementation of moving the probe out of line so it's not
>    easily confused with the central point of the test (Chris), use
>    object validation library helper just introduced.
>
>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 | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index fdd9661d..e46a4df7 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -23,6 +23,7 @@
>
> #include "igt.h"
> #include "igt_dummyload.h"
>+#include "i915/gem_gtt_topology.c"
>
> IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations.");
>
>@@ -520,7 +521,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));
>
>@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = (1ull << (i + 12)) - 4096;
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		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_gtt_validate_object(fd, &obj[n]);

Would it be better to name this function as
gem_gtt_validate_object_offset? From the earlier code is it was easy to
see that the intention of the test was to use gem_execbuf to map the
object to the offset. From the new method name unless I check the code
it's not straight forward that we are just checking the offset.

>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags)
> 		obj[n].offset = 1ull << (i + 12);
> 		obj[n].offset = gen8_canonical_address(obj[n].offset);
> 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>-		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_gtt_validate_object(fd, &obj[n]);
>+		if (err) {
>+			/* Iff using a shared GTT, some of it may be reserved */

Nit-pick: If, not Iff

>+			igt_assert_eq(err, -ENOSPC);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 

Other than the above comments, looks good to me.
Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda at intel.com>

Vanshi

>2.21.0
>