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

[Chris Wilson]

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.

> > +     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?

> > +     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.

> > +     gem_execbuf(i915, &eb);
> > +     igt_assert_eq_u64(batch.offset, 48 << 20);
> > +
> > +     arg.value = gem_vm_create(i915);
> > +     gem_context_set_param(i915, &arg);
> > +     gem_execbuf(i915, &eb);
> > +     igt_assert_eq_u64(batch.offset, 48 << 20);
> 
> Not sure what the offset check proves in this case? It's a new ppgtt, or 
> even if was old one. Seems it would never fail?

Right, this is just an unassuming valid path, checking that we follow
the batch offset around, and explodes if the kernel forgets to actually
update the PD inside the context image before execution.

Not that happened.

> > +     gem_vm_destroy(i915, arg.value);
> > +
> > +     arg.value = gem_vm_create(i915);
> > +     gem_context_set_param(i915, &arg);
> > +     batch.offset = 0;
> > +     gem_execbuf(i915, &eb);
> > +     igt_assert_eq_u64(batch.offset, 0);
> > +     gem_vm_destroy(i915, arg.value);
> > +
> > +     gem_sync(i915, batch.handle);
> > +     gem_close(i915, batch.handle);
> > +}