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

Ceraolo Spurio, Daniele
2015-04-23T10:14:14Z

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