[Intel-gfx] [PATCH v2] [i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 11:14:12AM +0100, Ceraolo Spurio, Daniele wrote:
> On 4/23/2015 10:43 AM, Chris Wilson wrote:
> >On Thu, Apr 23, 2015 at 10:30:01AM +0100, daniele.ceraolospurio at intel.com wrote:
> >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >>
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>Using imported objects should not leak i915 vmas (and vms).
> >>
> >>In practice this simulates Xorg importing fbcon and leaking (or not) one vma
> >>per Xorg startup cycle.
> >>
> >>v2: use low-level ioctl wrappers and bo offset to check the leak (Chris)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> (v2)
> >>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>---
> >>  tests/gem_ppgtt.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 99 insertions(+)
> >>
> >>diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
> >>index 5bf773c..b865af3 100644
> >>--- a/tests/gem_ppgtt.c
> >>+++ b/tests/gem_ppgtt.c
> >>@@ -48,6 +48,22 @@
> >>  #define HEIGHT 512
> >>  #define SIZE (HEIGHT*STRIDE)
> >>
> >>+static bool uses_full_ppgtt(int fd)
> >>+{
> >>+	struct drm_i915_getparam gp;
> >>+	int val = 0;
> >>+
> >>+	memset(&gp, 0, sizeof(gp));
> >>+	gp.param = 18; /* HAS_ALIASING_PPGTT */
> >>+	gp.value = &val;
> >>+
> >>+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> >>+		return 0;
> >>+
> >>+	errno = 0;
> >>+	return val > 1;
> >>+}
> >>+
> >>  static drm_intel_bo *create_bo(drm_intel_bufmgr *bufmgr,
> >>  			       uint32_t pixel)
> >>  {
> >>@@ -200,6 +216,86 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
> >>  	}
> >>  }
> >>
> >>+
> >>+static uint64_t exec_and_get_offset(int fd, uint32_t batch, uint32_t bo)
> >>+{
> >>+	struct drm_i915_gem_execbuffer2 execbuf;
> >>+	struct drm_i915_gem_exec_object2 exec[2];
> >>+	struct drm_i915_gem_relocation_entry reloc[1];
> >>+	uint32_t buf[6], i = 0;
> >>+
> >>+	/* use a simple MI_STORE_DWORD_IMM to write something on the bo.
> >>+	 * We just want to get a VMA
> >>+	 */
> >>+	buf[i++] = MI_STORE_DWORD_IMM | 2;
> >>+	buf[i++] = 0;
> >>+	buf[i++] = 0;
> >>+	buf[i++] = 0xdeadbeef;
> >>+
> >>+	reloc->offset = 1 * sizeof(uint32_t);
> >>+	reloc->delta = 0;
> >>+	reloc->target_handle = bo;
> >>+	reloc->read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> >>+	reloc->write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> >>+	reloc->presumed_offset = 0;
> >
> >A relocation (and the STORE) is not required. The kernel will do all
> >reservations (of exec_object[]) before doing relocations. Not relocating
> >makes life more predictable (fewer error conditions may strike).

Actually thought of another potential issue. The kernel will reserve
space in the GTT for an execbuffer in an arbitrary order. So kill the
separate batch entirely, and just use flinked objects (as the batch
buffer).

> >>+static void flink_and_close(void)
> >>+{
> >>+	uint32_t fd, fd2;
> >>+	uint32_t batch, bo, flinked_bo, new_bo, name;
> >>+	uint64_t offset, offset_new;
> >>+
> >>+	fd = drm_open_any();
> >>+	igt_require(uses_full_ppgtt(fd));
> >
> >The test equally applies to !full-ppgtt. The bug we saw isn't possible,
> >but the interface expectations are the same.
> 
> I've tried the test with aliasing ppgtt, but the flinked buffer gets
> the same offset as the original one, so the new_bo will get a
> different offset indipendently from the vma leak and the assert will
> always fail.
> Are there any other ways to check the vma leak in !full-ppgtt mode?

Oh right. Because we lazy-unbind and the object still exists within the
aliasing vm so the eviction is not forced.

I give in, I can't think of a way to test this on !full-ppgtt without
explicitly unbinding the vma, thereby breaking the neat test.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre