[Intel-gfx] [igt-dev] [PATCH i-g-t 13/24] i915/gem_ctx_param: Test set/get (copy) VM

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-26 10:33:04)
> 
> On 26/03/2019 10:22, Tvrtko Ursulin wrote:
> > 
> > On 22/03/2019 09:21, Chris Wilson wrote:
> >> +static void test_vm(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,
> >> +    };
> >> +    uint32_t parent, child;
> >> +
> >> +    arg.value = -1ull;
> >> +    igt_require(__gem_context_set_param(i915, &arg) == -ENOENT);
> >> +
> >> +    parent = gem_context_create(i915);
> >> +    child = gem_context_create(i915);
> >> +
> >> +    eb.rsvd1 = parent;
> >> +    batch.offset = 48 << 20;
> >> +    gem_execbuf(i915, &eb);
> >> +    igt_assert_eq_u64(batch.offset, 48 << 20);
> >> +
> >> +    eb.rsvd1 = child;
> >> +    batch.offset = 0;
> >> +    gem_execbuf(i915, &eb);
> >> +    igt_assert_eq_u64(batch.offset, 0);
> >> +
> >> +    eb.rsvd1 = parent;
> >> +    gem_execbuf(i915, &eb);
> >> +    igt_assert_eq_u64(batch.offset, 48 << 20);
> > 
> > Please drop a comment at the start of these execbuf operations to 
> > explain what and why. We don't need softpin to guarantee they will get 
> > pinned to where we want them to?

Using explict softpinning would make the test so much harder, since it
will end up exactly where you told it to be. If we kill the implicit
offset, I will shed a tear but hopefully the world moves on and makes
execbuf2 defunct before then.

> >> +    arg.ctx_id = parent;
> >> +    gem_context_get_param(i915, &arg);
> >> +
> >> +    arg.ctx_id = child;
> >> +    gem_context_set_param(i915, &arg);
> > 
> > Another get param to assert child vm id is the same as the parent?

Seem fair, you didn't like my proof by checking the vma offset :)

> > Also, try self-assign? I mean set the same vm id as already have?

Yup.
 
> And a test to check vm id space is per fd - that same id can be obtained 
> in two fds, if not too fragile/white-box wrt idr allocator.

That's meh. I don't want to encode any information about the id
themselves as part of the uABI, with the exception that 0 is the invalid
value.
 
> And also that different id from one fd cannot be passed to set_vm in 
> another. This one should be robust.

That's just an ENOENT check for set_param. Combining the two, checking
that if we obtain the same id from a second fd, that those VM are
distinct does should a useful challenge.

> >> +    eb.rsvd1 = child;
> >> +    gem_execbuf(i915, &eb);
> >> +    igt_assert_eq_u64(batch.offset, 48 << 20);
> > 
> > Interesting, for me at least. Please put a comment here.
> > 
> >> +
> >> +    gem_context_destroy(i915, child);
> >> +    gem_context_destroy(i915, parent);
> >> +
> >> +    gem_sync(i915, batch.handle);
> >> +    gem_close(i915, batch.handle);
> >> +}
> >> +
> >>   igt_main
> >>   {
> >>       struct drm_i915_gem_context_param arg;
> >> @@ -253,6 +309,9 @@ igt_main
> >>           gem_context_set_param(fd, &arg);
> >>       }
> >> +    igt_subtest("vm")
> >> +        test_vm(fd);
> >> +
> >>       arg.param = I915_CONTEXT_PARAM_PRIORITY;
> >>       igt_subtest("set-priority-not-supported") {
> >>
> > 
> > Add to basic test list? Or call basic-vm? Honestly don't remember how we 
> > do it these days..

This can live on the shards for exercising a corner of the API. I think
the second batch of vm tests should make more interesting BAT with a
bit (lot) more work to develop those into better HW coverage.
-Chris