| Message ID | 1429532054-29948-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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(); }