Re: [igt-dev] [PATCH i-g-t 05/16] i915/gem_ctx_create: Basic checks for constructor properties

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

On 22/05/2019 11:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-16 09:38:15)
>>
>> On 15/05/2019 20:05, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-05-14 11:15:12)
>>>>
>>>> On 08/05/2019 11:09, Chris Wilson wrote:
>>>>> Check that the extended create interface accepts setparam.
>>>>>
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> ---
>>>>>     tests/i915/gem_ctx_create.c | 225 ++++++++++++++++++++++++++++++++++--
>>>>>     1 file changed, 213 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tests/i915/gem_ctx_create.c b/tests/i915/gem_ctx_create.c
>>>>> index a664070db..9b4fddbe7 100644
>>>>> --- a/tests/i915/gem_ctx_create.c
>>>>> +++ b/tests/i915/gem_ctx_create.c
>>>>> @@ -33,6 +33,7 @@
>>>>>     #include <time.h>
>>>>>     
>>>>>     #include "igt_rand.h"
>>>>> +#include "sw_sync.h"
>>>>>     
>>>>>     #define LOCAL_I915_EXEC_BSD_SHIFT      (13)
>>>>>     #define LOCAL_I915_EXEC_BSD_MASK       (3 << LOCAL_I915_EXEC_BSD_SHIFT)
>>>>> @@ -45,12 +46,33 @@ static unsigned all_nengine;
>>>>>     static unsigned ppgtt_engines[16];
>>>>>     static unsigned ppgtt_nengine;
>>>>>     
>>>>> -static int __gem_context_create_local(int fd, struct drm_i915_gem_context_create *arg)
>>>>> +static int create_ioctl(int fd, struct drm_i915_gem_context_create *arg)
>>>>>     {
>>>>> -     int ret = 0;
>>>>> -     if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, arg))
>>>>> -             ret = -errno;
>>>>> -     return ret;
>>>>> +     int err;
>>>>> +
>>>>> +     err = 0;
>>>>> +     if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, arg)) {
>>>>> +             err = -errno;
>>>>> +             igt_assert(err);
>>>>> +     }
>>>>> +
>>>>> +     errno = 0;
>>>>> +     return err;
>>>>> +}
>>>>> +
>>>>> +static int create_ext_ioctl(int i915,
>>>>> +                         struct drm_i915_gem_context_create_ext *arg)
>>>>> +{
>>>>> +     int err;
>>>>> +
>>>>> +     err = 0;
>>>>> +     if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, arg)) {
>>>>> +             err = -errno;
>>>>> +             igt_assume(err);
>>>>> +     }
>>>>> +
>>>>> +     errno = 0;
>>>>> +     return err;
>>>>>     }
>>>>>     
>>>>>     static double elapsed(const struct timespec *start,
>>>>> @@ -308,6 +330,187 @@ static void maximum(int fd, int ncpus, unsigned mode)
>>>>>         free(contexts);
>>>>>     }
>>>>>     
>>>>> +static void basic_ext_param(int i915)
>>>>> +{
>>>>> +     struct drm_i915_gem_context_create_ext_setparam ext = {
>>>>> +             { .name = I915_CONTEXT_CREATE_EXT_SETPARAM },
>>>>> +     };
>>>>> +     struct drm_i915_gem_context_create_ext create = {
>>>>> +             .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS
>>>>> +     };
>>>>> +     struct drm_i915_gem_context_param get;
>>>>> +
>>>>> +     igt_require(create_ext_ioctl(i915, &create) == 0);
>>>>> +     gem_context_destroy(i915, create.ctx_id);
>>>>> +
>>>>> +     create.extensions = -1ull;
>>>>> +     igt_assert_eq(create_ext_ioctl(i915, &create), -EFAULT);
>>>>> +
>>>>> +     create.extensions = to_user_pointer(&ext);
>>>>> +     igt_assert_eq(create_ext_ioctl(i915, &create), -EINVAL);
>>>>> +
>>>>> +     ext.param.param = I915_CONTEXT_PARAM_PRIORITY;
>>>>> +     if (create_ext_ioctl(i915, &create) != -ENODEV) {
>>>>> +             gem_context_destroy(i915, create.ctx_id);
>>>>> +
>>>>> +             ext.base.next_extension = -1ull;
>>>>> +             igt_assert_eq(create_ext_ioctl(i915, &create), -EFAULT);
>>>>> +             ext.base.next_extension = to_user_pointer(&ext);
>>>>> +             igt_assert_eq(create_ext_ioctl(i915, &create), -E2BIG);
>>>>> +             ext.base.next_extension = 0;
>>>>> +
>>>>> +             ext.param.value = 32;
>>>>> +             igt_assert_eq(create_ext_ioctl(i915, &create), 0);
>>>>> +
>>>>> +             memset(&get, 0, sizeof(get));
>>>>> +             get.ctx_id = create.ctx_id;
>>>>> +             get.param = I915_CONTEXT_PARAM_PRIORITY;
>>>>> +             gem_context_get_param(i915, &get);
>>>>> +             igt_assert_eq(get.value, ext.param.value);
>>>>> +
>>>>> +             gem_context_destroy(i915, create.ctx_id);
>>>>> +     }
>>>>> +}
>>>>> +
>>>>> +static void check_single_timeline(int i915, uint32_t ctx, int num_engines)
>>>>> +{
>>>>> +#define RCS_TIMESTAMP (0x2000 + 0x358)
>>>>> +     const int gen = intel_gen(intel_get_drm_devid(i915));
>>>>> +     const int has_64bit_reloc = gen >= 8;
>>>>> +     struct drm_i915_gem_exec_object2 results = { .handle = gem_create(i915, 4096) };
>>>>> +     const uint32_t bbe = MI_BATCH_BUFFER_END;
>>>>> +     int timeline = sw_sync_timeline_create();
>>>>> +     uint32_t last, *map;
>>>>> +
>>>>> +     {
>>>>> +             struct drm_i915_gem_execbuffer2 execbuf = {
>>>>> +                     .buffers_ptr = to_user_pointer(&results),
>>>>> +                     .buffer_count = 1,
>>>>> +                     .rsvd1 = ctx,
>>>>> +             };
>>>>> +             gem_write(i915, results.handle, 0, &bbe, sizeof(bbe));
>>>>> +             gem_execbuf(i915, &execbuf);
>>>>> +             results.flags = EXEC_OBJECT_PINNED;
>>>>> +     }
>>>>> +
>>>>> +     for (int i = 0; i < num_engines; i++) {
>>>>> +             struct drm_i915_gem_exec_object2 obj[2] = {
>>>>> +                     results, /* write hazard lies! */
>>>>> +                     { .handle = gem_create(i915, 4096) },
>>>>> +             };
>>>>> +             struct drm_i915_gem_execbuffer2 execbuf = {
>>>>> +                     .buffers_ptr = to_user_pointer(obj),
>>>>> +                     .buffer_count = 2,
>>>>> +                     .rsvd1 = ctx,
>>>>> +                     .rsvd2 = sw_sync_timeline_create_fence(timeline, num_engines - i),
>>>>> +                     .flags = i | I915_EXEC_FENCE_IN,
>>>>> +             };
>>>>> +             uint64_t offset = results.offset + 4 * i;
>>>>> +             uint32_t *cs;
>>>>> +             int j = 0;
>>>>> +
>>>>> +             cs = gem_mmap__cpu(i915, obj[1].handle, 0, 4096, PROT_WRITE);
>>>>> +
>>>>> +             cs[j] = 0x24 << 23 | 1; /* SRM */
>>>>> +             if (has_64bit_reloc)
>>>>> +                     cs[j]++;
>>>>> +             j++;
>>>>> +             cs[j++] = RCS_TIMESTAMP;
>>>>> +             cs[j++] = offset;
>>>>> +             if (has_64bit_reloc)
>>>>> +                     cs[j++] = offset >> 32;
>>>>> +             cs[j++] = MI_BATCH_BUFFER_END;
>>>>> +
>>>>> +             munmap(cs, 4096);
>>>>> +
>>>>> +             gem_execbuf(i915, &execbuf);
>>>>> +             gem_close(i915, obj[1].handle);
>>>>> +             close(execbuf.rsvd2);
>>>>> +     }
>>>>> +     close(timeline);
>>>>> +     gem_sync(i915, results.handle);
>>>>> +
>>>>> +     map = gem_mmap__cpu(i915, results.handle, 0, 4096, PROT_READ);
>>>>> +     gem_set_domain(i915, results.handle, I915_GEM_DOMAIN_CPU, 0);
>>>>> +     gem_close(i915, results.handle);
>>>>> +
>>>>> +     last = map[0];
>>>>> +     for (int i = 1; i < num_engines; i++) {
>>>>> +             igt_assert_f((map[i] - last) > 0,
>>>>> +                          "Engine instance [%d] executed too early: this:%x, last:%x\n",
>>>>> +                          i, map[i], last);
>>>>> +             last = map[i];
>>>>> +     }
>>>>
>>>> Hm.. aren't two sw fences (two seqnos) just a needless complication -
>>>> since the execution order in the single timeline is controlled by
>>>> submission order. The statement is true only when compounded with the
>>>> fact that you signal both fences at the same time. I am thinking that if
>>>> it wasn't a single timeline context what would happen. Fences would be
>>>> signaled in order, but execution does not have to happen in order. That
>>>> it does is a property of single timeline and not fence ordering. So two
>>>> input fences with two seqnos is misleading. Single plug would do I think
>>>
>>> But that would not detect the case when it was multiple timelines...
>>>    
>>>> Or you are thinking to nudge the driver to do the right thing? But in
>>>> that case I think you'd need to manually advance the first seqno (2nd
>>>> batch) first and wait a bit to check it hasn't been execute. Then signal
>>>> the second seqno (first batch) and run the above check to see they have
>>>> been executed in order.
>>>
>>> The challenge is that we detect if the driver uses 2 timelines instead
>>> of one. So that is what we setup to detect.
>>
>> With a single seqno advance what determines the order of signal delivery
>> on blocked fences? Is it defined in the dma-fence contract it happens in
>> order? If it is, then is it defined that would map to in order
>> submission in i915 (if the contexts/timelines were separate)? (Might not
>> I am thinking, scheduler can decide whatever it wants.)
> 
> We don't emit signals in order. Mostly we do, but not strictly.
>   
>> So I don't see a problem with being more explicit in this test and doing
>> a step by step timeline advance so it is completely under test's control
>> what's happening. And it would AFAICS detect the two timelines because
>> it would expect first timeline advance must not result in request execution.
> 
> I don't see the problem with the test, it does what I need it to.

Now my memory on how exactly this test works has slightly faded.

Let me put it this way - if you just removed the single timeline flag 
form the test and left the rest as is - would it 100% reliably fail? If 
it would pass 100%, then is it because of undefined implementation 
details rather than the ABI contract?

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev