| Message ID | 1429788197-10817-1-git-send-email-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 >
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
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(); }