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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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? :)

Regards,

Tvrtko

>>> +     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);
>>> +}
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev