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

Tvrtko Ursulin
2019-03-26T11:48:10Z

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);
>>> +}