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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_ppgtt.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
index 5bf773c..9b5b3ee 100644
--- a/tests/gem_ppgtt.c
+++ b/tests/gem_ppgtt.c
@@ -200,6 +200,103 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
 	}
 }
 
+static void flink_and_contexts(void)
+{
+	drm_intel_bufmgr *bufmgr;
+	int fd, fd2;
+	dri_bo *bo;
+	uint32_t flink_name;
+	unsigned int max_line = 0, num_lines = 0, cnt = 0;
+	int ret;
+
+	/*
+	 * Export a bo via flink and access it from a child process via a
+	 * different ppggtt context. Make sure when child exists that the vma
+	 * (and hence the vm), associated with its ppgtt context, is torn down.
+	 */
+
+	fd = drm_open_any();
+
+	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+	igt_assert(bufmgr);
+
+	bo = create_bo(bufmgr, 0);
+	igt_assert(bo);
+
+	ret = drm_intel_bo_flink(bo, &flink_name);
+	igt_assert(ret == 0);
+
+	igt_fork(child, 20) {
+		int devid;
+		drm_intel_bufmgr *bufmgr;
+		int fd;
+		dri_bo *bo, *import;
+		struct intel_batchbuffer *batch;
+
+		fd = drm_open_any();
+		devid = intel_get_drm_devid(fd);
+
+		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+		igt_assert(bufmgr);
+
+		bo = create_bo(bufmgr, 0);
+		import = drm_intel_bo_gem_create_from_name(bufmgr,
+							   "flink_and_contexts",
+							   flink_name);
+		igt_assert(bo && import);
+
+		batch = intel_batchbuffer_alloc(bufmgr, devid);
+		igt_assert(batch);
+
+		intel_copy_bo(batch, bo, import, SIZE);
+
+		intel_batchbuffer_free(batch);
+
+		dri_bo_unreference(import);
+		dri_bo_unreference(bo);
+
+		drm_intel_bufmgr_destroy(bufmgr);
+		close(fd);
+
+		exit(0);
+	}
+
+	igt_waitchildren();
+
+	/*
+	 * Count the longest line in the file which lists the vmas per object.
+	 * This might be a bit fragile so maybe there is a better way.
+	 */
+	fd2 = open("/sys/kernel/debug/dri/0/i915_gem_gtt", O_RDONLY);
+	igt_assert(fd2 >= 0);
+
+	do {
+		char ch;
+
+		ret = read(fd2, &ch, 1);
+		if (ret == 0)
+			break;
+		igt_assert(ret == 1);
+		if (ch == '\n') {
+			if (cnt > max_line)
+				max_line = cnt;
+			num_lines++;
+			cnt = 0;
+		} else {
+			cnt++;
+		}
+	} while (true);
+
+	close(fd2);
+
+	dri_bo_unreference(bo);
+	drm_intel_bufmgr_destroy(bufmgr);
+	close(fd);
+
+	igt_assert(num_lines >= 1);
+	igt_assert(max_line < 1000);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +326,8 @@ int main(int argc, char **argv)
 		surfaces_check(rcs, N_CHILD, 0x8000 / N_CHILD);
 	}
 
+	igt_subtest("flink-vs-ctx-vm-leak")
+		flink_and_contexts();
+
 	igt_exit();
 }
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Mon, Apr 20, 2015 at 01:14:14PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/gem_ppgtt.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
> index 5bf773c..9b5b3ee 100644
> --- a/tests/gem_ppgtt.c
> +++ b/tests/gem_ppgtt.c
> @@ -200,6 +200,103 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
>  	}
>  }
>  
> +static void flink_and_contexts(void)
> +{
> +	drm_intel_bufmgr *bufmgr;
> +	int fd, fd2;
> +	dri_bo *bo;
> +	uint32_t flink_name;
> +	unsigned int max_line = 0, num_lines = 0, cnt = 0;
> +	int ret;
> +
> +	/*
> +	 * Export a bo via flink and access it from a child process via a
> +	 * different ppggtt context. Make sure when child exists that the vma
> +	 * (and hence the vm), associated with its ppgtt context, is torn down.
> +	 */
> +
> +	fd = drm_open_any();
> +
> +	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> +	igt_assert(bufmgr);
> +
> +	bo = create_bo(bufmgr, 0);
> +	igt_assert(bo);
> +
> +	ret = drm_intel_bo_flink(bo, &flink_name);
> +	igt_assert(ret == 0);
> +
> +	igt_fork(child, 20) {
> +		int devid;
> +		drm_intel_bufmgr *bufmgr;
> +		int fd;
> +		dri_bo *bo, *import;
> +		struct intel_batchbuffer *batch;
> +
> +		fd = drm_open_any();
> +		devid = intel_get_drm_devid(fd);
> +
> +		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> +		igt_assert(bufmgr);
> +
> +		bo = create_bo(bufmgr, 0);
> +		import = drm_intel_bo_gem_create_from_name(bufmgr,
> +							   "flink_and_contexts",
> +							   flink_name);
> +		igt_assert(bo && import);
> +
> +		batch = intel_batchbuffer_alloc(bufmgr, devid);
> +		igt_assert(batch);
> +
> +		intel_copy_bo(batch, bo, import, SIZE);

Do we care about any differentiation between the default per-file
context and explicitly created contexts?

> +		intel_batchbuffer_free(batch);
> +
> +		dri_bo_unreference(import);
> +		dri_bo_unreference(bo);
> +
> +		drm_intel_bufmgr_destroy(bufmgr);
> +		close(fd);
> +
> +		exit(0);
> +	}
> +
> +	igt_waitchildren();
> +
> +	/*
> +	 * Count the longest line in the file which lists the vmas per object.
> +	 * This might be a bit fragile so maybe there is a better way.
> +	 */

So what I was thinking was something along the lines of:

/* record the return value from exec[0].offset */
intel_copy_bo(&leaked_vma_offset);

gem_close(exec[0].handle);

/* flush execbuffer and the vma should be gone */
gem_sync();

intel_copy_bo(&new_vma_offset);
igt_assert(new_vma_offset == leaked_vma_offset);

Will take a bit of judicious planning (like doing a self-copy on the dst
first to make sure it bound ahead of the leak and not reallocating the
batch handle)

Or alternatively, we know the name of the flinked object, so we can do
an explicit search! Ok, that seems more robust, but you do some sync
point in there (either gem_sync() every child before exit or
gem_quiescent_gpu()).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 04/20/2015 01:50 PM, Chris Wilson wrote:
> On Mon, Apr 20, 2015 at 01:14:14PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   tests/gem_ppgtt.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>
>> diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
>> index 5bf773c..9b5b3ee 100644
>> --- a/tests/gem_ppgtt.c
>> +++ b/tests/gem_ppgtt.c
>> @@ -200,6 +200,103 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
>>   	}
>>   }
>>
>> +static void flink_and_contexts(void)
>> +{
>> +	drm_intel_bufmgr *bufmgr;
>> +	int fd, fd2;
>> +	dri_bo *bo;
>> +	uint32_t flink_name;
>> +	unsigned int max_line = 0, num_lines = 0, cnt = 0;
>> +	int ret;
>> +
>> +	/*
>> +	 * Export a bo via flink and access it from a child process via a
>> +	 * different ppggtt context. Make sure when child exists that the vma
>> +	 * (and hence the vm), associated with its ppgtt context, is torn down.
>> +	 */
>> +
>> +	fd = drm_open_any();
>> +
>> +	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
>> +	igt_assert(bufmgr);
>> +
>> +	bo = create_bo(bufmgr, 0);
>> +	igt_assert(bo);
>> +
>> +	ret = drm_intel_bo_flink(bo, &flink_name);
>> +	igt_assert(ret == 0);
>> +
>> +	igt_fork(child, 20) {
>> +		int devid;
>> +		drm_intel_bufmgr *bufmgr;
>> +		int fd;
>> +		dri_bo *bo, *import;
>> +		struct intel_batchbuffer *batch;
>> +
>> +		fd = drm_open_any();
>> +		devid = intel_get_drm_devid(fd);
>> +
>> +		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
>> +		igt_assert(bufmgr);
>> +
>> +		bo = create_bo(bufmgr, 0);
>> +		import = drm_intel_bo_gem_create_from_name(bufmgr,
>> +							   "flink_and_contexts",
>> +							   flink_name);
>> +		igt_assert(bo && import);
>> +
>> +		batch = intel_batchbuffer_alloc(bufmgr, devid);
>> +		igt_assert(batch);
>> +
>> +		intel_copy_bo(batch, bo, import, SIZE);
>
> Do we care about any differentiation between the default per-file
> context and explicitly created contexts?

Don't know - would expect so - but I am new in this area.

>> +		intel_batchbuffer_free(batch);
>> +
>> +		dri_bo_unreference(import);
>> +		dri_bo_unreference(bo);
>> +
>> +		drm_intel_bufmgr_destroy(bufmgr);
>> +		close(fd);
>> +
>> +		exit(0);
>> +	}
>> +
>> +	igt_waitchildren();
>> +
>> +	/*
>> +	 * Count the longest line in the file which lists the vmas per object.
>> +	 * This might be a bit fragile so maybe there is a better way.
>> +	 */
>
> So what I was thinking was something along the lines of:
>
> /* record the return value from exec[0].offset */
> intel_copy_bo(&leaked_vma_offset);
>
> gem_close(exec[0].handle);
>
> /* flush execbuffer and the vma should be gone */
> gem_sync();
>
> intel_copy_bo(&new_vma_offset);
> igt_assert(new_vma_offset == leaked_vma_offset);
>
> Will take a bit of judicious planning (like doing a self-copy on the dst
> first to make sure it bound ahead of the leak and not reallocating the
> batch handle)

Hm.. not bad!

> Or alternatively, we know the name of the flinked object, so we can do
> an explicit search! Ok, that seems more robust, but you do some sync
> point in there (either gem_sync() every child before exit or
> gem_quiescent_gpu()).

Anything to avoid debugfs parsing I think. Well not anything, if 
exec[].offset is not retrievable from intel_batchbuffer helpers I'll 
reconsider it. Definitely need to at least count ppgtt sections since 
the IGT already failed (to fail) on one older kernel with the poor 
character counting approach.

Thanks for the ideas!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Mon, Apr 20, 2015 at 03:56:09PM +0100, Tvrtko Ursulin wrote:
> >Will take a bit of judicious planning (like doing a self-copy on the dst
> >first to make sure it bound ahead of the leak and not reallocating the
> >batch handle)
> 
> Hm.. not bad!
> 
> >Or alternatively, we know the name of the flinked object, so we can do
> >an explicit search! Ok, that seems more robust, but you do some sync
> >point in there (either gem_sync() every child before exit or
> >gem_quiescent_gpu()).
> 
> Anything to avoid debugfs parsing I think. Well not anything, if
> exec[].offset is not retrievable from intel_batchbuffer helpers I'll
> reconsider it. Definitely need to at least count ppgtt sections
> since the IGT already failed (to fail) on one older kernel with the
> poor character counting approach.

With the libdrm interface, drm_intel_bo->offset will be kept up to date
with the post-execbuffer value. I'd suggest just using the rawer ioctl
wrappers for fine control, especially when allocating objects and
offsets.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Apr 20, 2015 at 01:50:48PM +0100, Chris Wilson wrote:
> On Mon, Apr 20, 2015 at 01:14:14PM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@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.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/gem_ppgtt.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 100 insertions(+)
> > 
> > diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
> > index 5bf773c..9b5b3ee 100644
> > --- a/tests/gem_ppgtt.c
> > +++ b/tests/gem_ppgtt.c
> > @@ -200,6 +200,103 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
> >  	}
> >  }
> >  
> > +static void flink_and_contexts(void)
> > +{
> > +	drm_intel_bufmgr *bufmgr;
> > +	int fd, fd2;
> > +	dri_bo *bo;
> > +	uint32_t flink_name;
> > +	unsigned int max_line = 0, num_lines = 0, cnt = 0;
> > +	int ret;
> > +
> > +	/*
> > +	 * Export a bo via flink and access it from a child process via a
> > +	 * different ppggtt context. Make sure when child exists that the vma
> > +	 * (and hence the vm), associated with its ppgtt context, is torn down.
> > +	 */
> > +
> > +	fd = drm_open_any();
> > +
> > +	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> > +	igt_assert(bufmgr);
> > +
> > +	bo = create_bo(bufmgr, 0);
> > +	igt_assert(bo);
> > +
> > +	ret = drm_intel_bo_flink(bo, &flink_name);
> > +	igt_assert(ret == 0);
> > +
> > +	igt_fork(child, 20) {
> > +		int devid;
> > +		drm_intel_bufmgr *bufmgr;
> > +		int fd;
> > +		dri_bo *bo, *import;
> > +		struct intel_batchbuffer *batch;
> > +
> > +		fd = drm_open_any();
> > +		devid = intel_get_drm_devid(fd);
> > +
> > +		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> > +		igt_assert(bufmgr);
> > +
> > +		bo = create_bo(bufmgr, 0);
> > +		import = drm_intel_bo_gem_create_from_name(bufmgr,
> > +							   "flink_and_contexts",
> > +							   flink_name);
> > +		igt_assert(bo && import);
> > +
> > +		batch = intel_batchbuffer_alloc(bufmgr, devid);
> > +		igt_assert(batch);
> > +
> > +		intel_copy_bo(batch, bo, import, SIZE);
> 
> Do we care about any differentiation between the default per-file
> context and explicitly created contexts?
> 
> > +		intel_batchbuffer_free(batch);
> > +
> > +		dri_bo_unreference(import);
> > +		dri_bo_unreference(bo);
> > +
> > +		drm_intel_bufmgr_destroy(bufmgr);
> > +		close(fd);
> > +
> > +		exit(0);
> > +	}
> > +
> > +	igt_waitchildren();
> > +
> > +	/*
> > +	 * Count the longest line in the file which lists the vmas per object.
> > +	 * This might be a bit fragile so maybe there is a better way.
> > +	 */
> 
> So what I was thinking was something along the lines of:
> 
> /* record the return value from exec[0].offset */
> intel_copy_bo(&leaked_vma_offset);
> 
> gem_close(exec[0].handle);
> 
> /* flush execbuffer and the vma should be gone */
> gem_sync();
> 
> intel_copy_bo(&new_vma_offset);
> igt_assert(new_vma_offset == leaked_vma_offset);
> 
> Will take a bit of judicious planning (like doing a self-copy on the dst
> first to make sure it bound ahead of the leak and not reallocating the
> batch handle)
> 
> Or alternatively, we know the name of the flinked object, so we can do
> an explicit search! Ok, that seems more robust, but you do some sync
> point in there (either gem_sync() every child before exit or
> gem_quiescent_gpu()).

Another option for making the leak check more robust is to allocate a few
test objects and double-check that the count we deduce
incremented/decremented by the correct amount. But checking the offsets
the kernel hands out from execbuf should work well too, we have a few
other tests relying upon that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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;
+
+	buf[i++] = 0;
+	buf[i++] = MI_BATCH_BUFFER_END;
+
+	gem_write(fd, batch, 0, buf, i * sizeof(uint32_t));
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = bo;
+	exec[1].handle = batch;
+	exec[1].relocation_count = 1;
+	exec[1].relocs_ptr = (uintptr_t)reloc;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 2;
+	execbuf.batch_len = i * sizeof(uint32_t);
+	execbuf.flags = 0;
+
+	gem_execbuf(fd, &execbuf);
+
+	return exec[0].offset;
+}
+
+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));
+
+	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);
+
+	gem_close(fd2, batch);
+	gem_close(fd, bo);
+	close(fd);
+	close(fd2);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +325,8 @@ int main(int argc, char **argv)
 		surfaces_check(rcs, N_CHILD, 0x8000 / N_CHILD);
 	}
 
+	igt_subtest("flink-and-close-vma-leak")
+		flink_and_close();
+
 	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ceraolo Spurio, Daniele]

On 4/23/2015 10:43 AM, Chris Wilson wrote:
> On Thu, Apr 23, 2015 at 10:30:01AM +0100, daniele.ceraolospurio@intel.com wrote:
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.

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?

Thanks,
Daniele

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@intel.com wrote:
> >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>
> >>From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> >>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2)
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[daniele.ceraolospurio]

From: Tvrtko Ursulin <tvrtko.ursulin@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)
v3: use the flinked bo as batch (Chris)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2+)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_ppgtt.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
index 5bf773c..882a884 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,63 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
 	}
 }
 
+static uint64_t exec_and_get_offset(int fd, uint32_t batch)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec[1];
+	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };
+
+	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = batch;
+	exec[0].relocation_count = 0;
+	exec[0].relocs_ptr = 0;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 1;
+	execbuf.batch_len = sizeof(batch_data);
+	execbuf.flags = 0;
+
+	gem_execbuf(fd, &execbuf);
+
+	return exec[0].offset;
+}
+
+static void flink_and_close(void)
+{
+	uint32_t fd, fd2;
+	uint32_t bo, flinked_bo, new_bo, name;
+	uint64_t offset, offset_new;
+
+	fd = drm_open_any();
+	igt_require(uses_full_ppgtt(fd));
+
+	bo = gem_create(fd, 4096);
+	name = gem_flink(fd, bo);
+
+	fd2 = drm_open_any();
+
+	flinked_bo = gem_open(fd2, name);
+	offset = exec_and_get_offset(fd2, flinked_bo);
+	gem_sync(fd2, flinked_bo);
+	gem_close(fd2, flinked_bo);
+
+	/* 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, new_bo);
+	gem_close(fd2, new_bo);
+
+	igt_assert_eq(offset, offset_new);
+
+	gem_close(fd, bo);
+	close(fd);
+	close(fd2);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +302,8 @@ int main(int argc, char **argv)
 		surfaces_check(rcs, N_CHILD, 0x8000 / N_CHILD);
 	}
 
+	igt_subtest("flink-and-close-vma-leak")
+		flink_and_close();
+
 	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] [i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 12:23:17PM +0100, daniele.ceraolospurio@intel.com wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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)
> v3: use the flinked bo as batch (Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2+)
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Bikeshedding for fun aside,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> +static uint64_t exec_and_get_offset(int fd, uint32_t batch)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec[1];
> +	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };

uint32_t buf[2] = { MI_BATCH_BUFFER_END };

> +
> +	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
> +
> +	memset(exec, 0, sizeof(exec));
> +	exec[0].handle = batch;
> +	exec[0].relocation_count = 0;
> +	exec[0].relocs_ptr = 0;

We just memset(0) these two, so we don't need to clear them again.

> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = (uintptr_t)exec;
> +	execbuf.buffer_count = 1;
> +	execbuf.batch_len = sizeof(batch_data);
> +	execbuf.flags = 0;
These two can also happily disappear.

> +	gem_execbuf(fd, &execbuf);

igt_assert_neq(exec[0].offset, -1)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] [i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

[Ceraolo Spurio, Daniele]

On 4/23/2015 12:36 PM, Chris Wilson wrote:
> On Thu, Apr 23, 2015 at 12:23:17PM +0100, daniele.ceraolospurio@intel.com wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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)
>> v3: use the flinked bo as batch (Chris)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2+)
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Bikeshedding for fun aside,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
>> +static uint64_t exec_and_get_offset(int fd, uint32_t batch)
>> +{
>> +	struct drm_i915_gem_execbuffer2 execbuf;
>> +	struct drm_i915_gem_exec_object2 exec[1];
>> +	uint32_t batch_data[2] = { MI_NOOP, MI_BATCH_BUFFER_END };
>
> uint32_t buf[2] = { MI_BATCH_BUFFER_END };
>
>> +
>> +	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
>> +
>> +	memset(exec, 0, sizeof(exec));
>> +	exec[0].handle = batch;
>> +	exec[0].relocation_count = 0;
>> +	exec[0].relocs_ptr = 0;
>
> We just memset(0) these two, so we don't need to clear them again.
>
>> +	memset(&execbuf, 0, sizeof(execbuf));
>> +	execbuf.buffers_ptr = (uintptr_t)exec;
>> +	execbuf.buffer_count = 1;
>> +	execbuf.batch_len = sizeof(batch_data);
>> +	execbuf.flags = 0;
> These two can also happily disappear.

Ok for the execbuf.flags and for the other fixes, but why remove also 
execbuf.batch_len?

Thanks,
Daniele

>
>> +	gem_execbuf(fd, &execbuf);
>
> igt_assert_neq(exec[0].offset, -1)
> -Chris
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] [i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 02:01:43PM +0100, Ceraolo Spurio, Daniele wrote:
> On 4/23/2015 12:36 PM, Chris Wilson wrote:
> >>+	memset(&execbuf, 0, sizeof(execbuf));
> >>+	execbuf.buffers_ptr = (uintptr_t)exec;
> >>+	execbuf.buffer_count = 1;
> >>+	execbuf.batch_len = sizeof(batch_data);
> >>+	execbuf.flags = 0;
> >These two can also happily disappear.
> 
> Ok for the execbuf.flags and for the other fixes, but why remove
> also execbuf.batch_len?

Then we get flagged as a no-op batch. It's just a piece of
sleight-of-hand.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] [i-g-t] tests/gem_ppgtt: Check for vm leaks with flink and ppgtt

[Tvrtko Ursulin]

On 04/23/2015 12:23 PM, daniele.ceraolospurio@intel.com wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I have least to do with the current version so best upgrade yourself as 
author now!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@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)
v3: use the flinked bo as batch (Chris)
v4: add check on offset, remove unneeded  assignments (Chris)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> (v2+)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/gem_ppgtt.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/tests/gem_ppgtt.c b/tests/gem_ppgtt.c
index 5bf773c..d1e484a 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,60 @@ static void surfaces_check(dri_bo **bo, int count, uint32_t expected)
 	}
 }
 
+static uint64_t exec_and_get_offset(int fd, uint32_t batch)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec[1];
+	uint32_t batch_data[2] = { MI_BATCH_BUFFER_END };
+
+	gem_write(fd, batch, 0, batch_data, sizeof(batch_data));
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = batch;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 1;
+
+	gem_execbuf(fd, &execbuf);
+	igt_assert_neq(exec[0].offset, -1);
+
+	return exec[0].offset;
+}
+
+static void flink_and_close(void)
+{
+	uint32_t fd, fd2;
+	uint32_t bo, flinked_bo, new_bo, name;
+	uint64_t offset, offset_new;
+
+	fd = drm_open_any();
+	igt_require(uses_full_ppgtt(fd));
+
+	bo = gem_create(fd, 4096);
+	name = gem_flink(fd, bo);
+
+	fd2 = drm_open_any();
+
+	flinked_bo = gem_open(fd2, name);
+	offset = exec_and_get_offset(fd2, flinked_bo);
+	gem_sync(fd2, flinked_bo);
+	gem_close(fd2, flinked_bo);
+
+	/* 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, new_bo);
+	gem_close(fd2, new_bo);
+
+	igt_assert_eq(offset, offset_new);
+
+	gem_close(fd, bo);
+	close(fd);
+	close(fd2);
+}
+
 #define N_CHILD 8
 int main(int argc, char **argv)
 {
@@ -229,5 +299,8 @@ int main(int argc, char **argv)
 		surfaces_check(rcs, N_CHILD, 0x8000 / N_CHILD);
 	}
 
+	igt_subtest("flink-and-close-vma-leak")
+		flink_and_close();
+
 	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx