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