Re: [igt-dev] [PATCH i-g-t 15/24] i915: Add gem_vm_create

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t 15/24] i915: Add gem_vm_create
Date: Tue, 26 Mar 2019 14:11:46 +0000	[thread overview]
Message-ID: <155360950640.15930.9005696168592044463@skylake-alporthouse-com> (raw)
In-Reply-To: <a1e9d68a-18dc-05fb-7cf4-3a701b79374b@linux.intel.com>

Quoting Tvrtko Ursulin (2019-03-26 11:48:10)
> 
> On 26/03/2019 11:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-26 11:21:13)
> >>
> >> On 22/03/2019 09:21, Chris Wilson wrote:
> >>> +static void invalid_create(int i915)
> >>> +{
> >>> +     struct drm_i915_gem_vm_control ctl = {};
> >>> +     struct i915_user_extension ext = { .name = -1 };
> >>> +
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     gem_vm_destroy(i915, ctl.vm_id);
> >>> +
> >>> +     ctl.vm_id = 0xdeadbeef;
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     gem_vm_destroy(i915, ctl.vm_id);
> >>> +     ctl.vm_id = 0;
> >>
> >> Oh we allow garbage in.. hm.. perhaps disallow that?
> > 
> > It's documented as an out parameter, so meh.
> > 
> >> Otherwise both tests here are not invalid input/behaviour. First is also
> >> covered by has_vm.
> > 
> > First test is with valid input. Second test checks that we fill the
> > vm_id with something recognisable by the kernel.
> > 
> >>> +     ctl.flags = -1;
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), -EINVAL);
> >>> +     ctl.flags = 0;
> >>> +
> >>> +     ctl.extensions = -1;
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), -EFAULT);
> >>> +     ctl.extensions = to_user_pointer(&ext);
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), -EINVAL);
> >>> +     ctl.extensions = 0;
> >>
> >> Could add a loop rejection test as well, even though the underlying
> >> implementation is shared.
> > 
> > Can't loop as there are no valid extensions (yet), so it gets rejected by
> > -EINVAL for unrecognised name.
> > 
> >>> +static void invalid_destroy(int i915)
> >>> +{
> >>> +     struct drm_i915_gem_vm_control ctl = {};
> >>> +
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), -ENOENT);
> >>
> >> Should we have id 0 be -EINVAL?
> > 
> > That would be inconsistent, so no.
> 
> Ok.
> 
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), 0);
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), -ENOENT);
> >>> +
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     ctl.vm_id = ctl.vm_id + 1; /* XXX assume cyclic allocator */
> >>
> >> I think this actually assumes, and correctly so, that the fd is clean
> >> ie. only the created vm_id has been allocated. So comment needs to be
> >> corrected I think.
> > 
> > It was more what I was planning to do. But yes, the assumption is that
> > only one id is valid.
> > 
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), -ENOENT);
> >>> +     ctl.vm_id = ctl.vm_id - 1;
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), 0);
> >>> +
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     ctl.flags = -1;
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), -EINVAL);
> >>> +     ctl.flags = 0;
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), 0);
> >>
> >> Second create-destroy is redundant but okay.
> > 
> > Redundant with what?
> 
> Forget I said it.
> 
> >>> +     igt_assert_eq(__gem_vm_create_local(i915, &ctl), 0);
> >>> +     ctl.extensions = -1;
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), -EINVAL);
> >>> +     ctl.extensions = 0;
> >>> +     igt_assert_eq(__gem_vm_destroy_local(i915, &ctl), 0);
> >>> +}
> >>> +
> >>> +static uint32_t __batch_create(int i915, uint32_t offset)
> >>> +{
> >>> +     const uint32_t bbe = MI_BATCH_BUFFER_END;
> >>> +     uint32_t handle;
> >>> +
> >>> +     handle = gem_create(i915, ALIGN(offset + 4, 4096));
> >>> +     gem_write(i915, handle, offset, &bbe, sizeof(bbe));
> >>> +
> >>> +     return handle;
> >>> +}
> >>> +
> >>> +static uint32_t batch_create(int i915)
> >>> +{
> >>> +     return __batch_create(i915, 0);
> >>> +}
> >>
> >> Oi! :D
> >>
> >>> +
> >>> +static void execbuf(int i915)
> >>> +{
> >>> +     struct drm_i915_gem_exec_object2 batch = {
> >>> +             .handle = batch_create(i915),
> >>> +     };
> >>> +     struct drm_i915_gem_execbuffer2 eb = {
> >>> +             .buffers_ptr = to_user_pointer(&batch),
> >>> +             .buffer_count = 1,
> >>> +     };
> >>> +     struct drm_i915_gem_context_param arg = {
> >>> +             .param = I915_CONTEXT_PARAM_VM,
> >>> +     };
> >>> +
> >>> +     /* First verify that we try to use "softpinning" by default */
> >>> +     batch.offset = 48 << 20;
> >>
> >> Choice of 48 is a bit misleading, but okay.
> > 
> > Misleading? It's a spot has a good chance of being clear on every
> > platform. It's almost as if I just copied this from when I was thinking
> > about GGTT.
> 
> What is special about 48Mib? Apart that reminds me of 48-bit ppgtt? :)

Smallest aperture is 64MiB. Assume around 10MiB get consumed by
framebuffer and ringbuffers; leaving another 10MiB for a second
framebuffer at some pointer causing some fragmentation, so call it
20MiB. So anything in first 30MiB or so could be allocated on an idle
system after, so 48MiB seemed like a reasonable guess to avoid a pinned
framebuffer.

Being lazy is hard work.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx