[igt-dev] [PATCH i-g-t] i915/gem_engine_topology: Introduce and use gem_context_clone_with_engines

[Tvrtko Ursulin]

On 23/01/2020 14:16, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-23 14:01:41)
>>
>> On 23/01/2020 13:16, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-01-23 13:08:26)
>>>>
>>>> On 23/01/2020 12:54, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2020-01-23 12:43:06)
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>
>>>>>> In test cases which create new contexts and submit work against them using
>>>>>> the passed in engine index we are sometimes unsure whether this engine
>>>>>> index was potentially created based on a default context with engine map
>>>>>> configured (such as when under the __for_each_physical_engine iterator.
>>>>>>
>>>>>> To simplify test code we add gem_context/queue_clone_with_engines which
>>>>>> is to be used in such scenario instead of the current pattern of
>>>>>> gem_context_create followed by gem_context_set_all_engines (which is also
>>>>>> removed by the patch).
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>> ---
>>>>>>     lib/i915/gem_context.c         | 59 ++++++++++++++++++++++++++++++++++
>>>>>>     lib/i915/gem_context.h         |  4 +++
>>>>>>     lib/i915/gem_engine_topology.c | 11 -------
>>>>>>     lib/i915/gem_engine_topology.h |  2 --
>>>>>>     tests/i915/gem_ctx_clone.c     | 15 +--------
>>>>>>     tests/i915/gem_ctx_switch.c    | 19 ++++-------
>>>>>>     tests/i915/gem_exec_parallel.c |  3 +-
>>>>>>     tests/i915/gem_spin_batch.c    | 11 +++----
>>>>>>     tests/perf_pmu.c               |  3 +-
>>>>>>     9 files changed, 76 insertions(+), 51 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
>>>>>> index 1fae5191f83f..f92d5ff3dfc5 100644
>>>>>> --- a/lib/i915/gem_context.c
>>>>>> +++ b/lib/i915/gem_context.c
>>>>>> @@ -372,6 +372,50 @@ uint32_t gem_context_clone(int i915,
>>>>>>            return ctx;
>>>>>>     }
>>>>>>     
>>>>>> +bool gem_has_context_clone(int i915)
>>>>>> +{
>>>>>> +       struct drm_i915_gem_context_create_ext_clone ext = {
>>>>>> +               { .name = I915_CONTEXT_CREATE_EXT_CLONE },
>>>>>> +               .clone_id = -1,
>>>>>> +       };
>>>>>> +       struct drm_i915_gem_context_create_ext create = {
>>>>>> +               .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
>>>>>> +               .extensions = to_user_pointer(&ext),
>>>>>> +       };
>>>>>> +       int err;
>>>>>> +
>>>>>> +       err = 0;
>>>>>> +       if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &create)) {
>>>>>> +               err = -errno;
>>>>>> +               igt_assume(err);
>>>>>> +       }
>>>>>> +       errno = 0;
>>>>>> +
>>>>>> +       return err == -ENOENT;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * gem_context_clone_with_engines:
>>>>>> + * @i915: open i915 drm file descriptor
>>>>>> + * @src: i915 context id
>>>>>> + *
>>>>>> + * Special purpose wrapper to create a new context by cloning engines from @src.
>>>>>> + *
>>>>>> + * In can be called regardless of whether the kernel supports context cloning.
>>>>>> + *
>>>>>> + * Intended purpose is to use for creating contexts against which work will be
>>>>>> + * submitted and the engine index came from external source, derived from a
>>>>>> + * default context potentially configured with an engine map.
>>>>>> + */
>>>>>> +uint32_t gem_context_clone_with_engines(int i915, uint32_t src)
>>>>>> +{
>>>>>> +       if (!gem_has_context_clone(i915))
>>>>>> +               return gem_context_create(i915);
>>>>>
>>>>> Yes, that should cover us for older kernels and keep the for_each loops
>>>>> happy.
>>>>>
>>>>>> +       else
>>>>>> +               return gem_context_clone(i915, src, 0,
>>>>>> +                                        I915_CONTEXT_CLONE_ENGINES);
>>>>>
>>>>> 0 and CLONE are the wrong way around.
>>>>
>>>> Oopsie.
>>>>
>>>>>
>>>>> I would have done
>>>>>
>>>>> int __gem_context_clone_with_engines(int i915, uint32_t src, uint32_t *out)
>>>>> {
>>>>>         int err;
>>>>>
>>>>>         err = __gem_context_clone(i915, src, CLONE_ENGINES, 0, out);
>>>>>         if (err && !gem_has_context_clone(i915))
>>>>>                 err = __gem_context_create(i915, out);
>>>>>
>>>>>         return err;
>>>>> }
>>>>>
>>>>> uint32_t gem_context_clone_with_engines(int i915, uint32_t src)
>>>>> {
>>>>>         uint32_t ctx;
>>>>>
>>>>>         igt_assert_eq(__gem_context_clone_with_engine(i915, src, &ctx), 0);
>>>>>
>>>>>         return ctx;
>>>>> }
>>>>
>>>> I think I prefer my version as it is a bit more explicit.
>>>
>>> I was hoping to do something more like
>>>        err = __clone()
>>>        if (err == ENOSYS)
>>>                err = __create()
>>>
>>> Either way, I would suggest doing
>>>
>>> int __gem_context_clone_with_engines(int i915, uint32_t src, uint32_t *out);
>>> uint32_t gem_context_clone_with_engines(int i915, uint32_t src);
>>>
>>> as I prefer that style of error message.
>>
>> Error message? What do you mean?
> 
> igt_assert_eq(__gem_context_clone_with_engine(i915, src, &ctx), 0);
> is the nicest assert message without using igt_assert_f and writing an
> information message by hand.

But you will get that, either helper gem_context_clone_with_engines 
calls uses that style.

>>> Nothing else to complain about,
>>> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>>
>> So this is conditional on rewriting it as above or not?
> 
> Mere recommendations. Knowing full well that if at some point we need
> __gem_context_clone_with_engines() the above will happen anyway :)

Ack, leaving it for later. Now need to wait for the results here and 
then coordinate with at least three people who will need to adjust their 
patches.

Regards,

Tvrtko