[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 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).

> +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.

> +	bo = gem_create(fd, 4096);
> +	name = gem_flink(fd, bo);
> +
> +	fd2 = drm_open_any();
> +	batch = gem_create(fd2, 4096);
> +
> +	flinked_bo = gem_open(fd2, name);
> +	offset = exec_and_get_offset(fd2, batch, flinked_bo);
> +	gem_close(fd2, flinked_bo);
> +	gem_sync(fd2, batch);
> +
> +	/* the flinked bo VMA should have been cleared now, so a new bo of the
> +	 * same size should get the same offset
> +	 */
> +	new_bo = gem_create(fd2, 4096);
> +	offset_new = exec_and_get_offset(fd2, batch, new_bo);
> +	gem_close(fd2, new_bo);
> +
> +	igt_assert(offset == offset_new);

igt_assert_eq

Nice test.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre