[igt-dev] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Vanshi,

On Thursday, October 31, 2019 5:58:31 PM CET Vanshidhar Konda wrote:
> On Thu, Oct 31, 2019 at 04:28:55PM +0100, Janusz Krzysztofik wrote:
> >Some tests assume 4kB offset alignment while using softpin.  That
> >assumption may be wrong on future GEM backends with possibly larger
> >minimum page sizes.  As a result, those tests may either fail on
> >softpin at offsets which are incorrectly aligned, may silently skip
> >such incorrectly aligned addresses assuming them occupied by other
> >users if incorrect detection method is used, or may always succeed
> >when examining invalid use patterns.
> >
> >Provide a helper function that detects minimum GTT alignment.  Tests
> >may use it to calculate softpin offsets valid for actually used backing
> >store.
> >
> >v2: Rename the helper, use 'minimum GTT alignment' term across the
> >    change (Chris),
> >  - use error numbers to distinguish between invalid offsets and
> >    addresses occupied by other users, then
> >  - simplify the code (Chris).
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> >Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >Cc: Stuart Summers <stuart.summers at intel.com>
> >---
> > lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > lib/ioctl_wrappers.h |  2 ++
> > 2 files changed, 48 insertions(+)
> >
> >diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> >index 628f8b83..f0ef8b2e 100644
> >--- a/lib/ioctl_wrappers.c
> >+++ b/lib/ioctl_wrappers.c
> >@@ -54,6 +54,7 @@
> > #include "intel_io.h"
> > #include "igt_debugfs.h"
> > #include "igt_sysfs.h"
> >+#include "igt_x86.h"
> > #include "config.h"
> >
> > #ifdef HAVE_VALGRIND
> >@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > 	return has_softpin;
> > }
> >
> >+/**
> >+ * gem_gtt_min_alignment_order:
> >+ * @fd: open i915 drm file descriptor
> >+ *
> >+ * This function detects the minimum possible alignment of a soft-pinned gem
> >+ * object allocated from a default backing store.  It is useful for calculating
> >+ * correctly aligned softpin offsets.
> >+ * Since size order to size conversion (size = 1 << order) is less trivial
> >+ * than the opposite, the function returns the alignment order as more handy.
> >+ *
> >+ * Returns:
> >+ * Size order of the minimum GTT alignment
> >+ */
> >+int gem_gtt_min_alignment_order(int fd)
> >+{
> >+	struct drm_i915_gem_exec_object2 obj;
> >+	struct drm_i915_gem_execbuffer2 eb;
> >+	const uint32_t bbe = MI_BATCH_BUFFER_END;
> >+	int order;
> >+
> >+	/* no softpin => 4kB page size */
> >+	if (!gem_has_softpin(fd))
> >+		return 12;
> >+
> >+	memset(&obj, 0, sizeof(obj));
> >+	memset(&eb, 0, sizeof(eb));
> >+
> >+	obj.handle = gem_create(fd, 4096);
> >+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >+	eb.buffers_ptr = to_user_pointer(&obj);
> >+	eb.buffer_count = 1;
> >+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> 
> I think it will be safer to create a new context to execute this
> execbuffer. For a new context the address space should be empty reducing
> the chance that there is another object mapped by the caller of the
> helper function at the address we start testing.

AFAICU, that shouldn't matter.  Object attributes are validated and possibly 
-EINVAL is returned already before an eviction is possibly attempted.  The 
algorithm assumes an offset is valid if any error code other than -EINVAL 
(including no error) is returned.

Thanks,
Janusz

> 
> Otherwise it looks good to me.
> 
> Vanshi
> 
> >+
> >+	for (order = 12; order < 64; order++) {
> >+		obj.offset = 1ull << order;
> >+		if (__gem_execbuf(fd, &eb) != -EINVAL)
> >+			break;
> >+	}
> >+	igt_assert(obj.offset < gem_aperture_size(fd));
> >+
> >+	gem_close(fd, obj.handle);
> >+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
> >+	return order;
> >+}
> >+
> > /**
> >  * gem_has_exec_fence:
> >  * @fd: open i915 drm file descriptor
> >diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> >index 03211c97..c8d57a7c 100644
> >--- a/lib/ioctl_wrappers.h
> >+++ b/lib/ioctl_wrappers.h
> >@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
> > uint64_t gem_global_aperture_size(int fd);
> > uint64_t gem_mappable_aperture_size(void);
> > bool gem_has_softpin(int fd);
> >+int gem_gtt_min_alignment_order(int fd);
> >+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
> > bool gem_has_exec_fence(int fd);
> >
> > /* check functions which auto-skip tests by calling igt_skip() */
>