diff mbox series

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

Message ID 20200123124306.18857-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [i-g-t] i915/gem_engine_topology: Introduce and use gem_context_clone_with_engines | expand

Commit Message

Tvrtko Ursulin Jan. 23, 2020, 12:43 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Suggested-by: Chris Wilson <chris@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(-)

Comments

Chris Wilson Jan. 23, 2020, 12:54 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2020-01-23 12:43:06)
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Suggested-by: Chris Wilson <chris@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.

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

> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index 989a6e26d6ef..43a99e0ff680 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -274,17 +274,6 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>         return 0;
>  }
>  
> -void gem_context_set_all_engines(int fd, uint32_t ctx)
> -{
> -       DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
> -       struct intel_engine_data engine_data = { };
> -
> -       if (!gem_topology_get_param(fd, &param) && !param.size) {
> -               query_engine_list(fd, &engine_data);
> -               ctx_map_engines(fd, &engine_data, &param);
> -       }
> -}

Me likes.
-Chris
Tvrtko Ursulin Jan. 23, 2020, 1:08 p.m. UTC | #2 
On 23/01/2020 12:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-23 12:43:06)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
>> Suggested-by: Chris Wilson <chris@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.

Regards,

Tvrtko

> 
>> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
>> index 989a6e26d6ef..43a99e0ff680 100644
>> --- a/lib/i915/gem_engine_topology.c
>> +++ b/lib/i915/gem_engine_topology.c
>> @@ -274,17 +274,6 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>>          return 0;
>>   }
>>   
>> -void gem_context_set_all_engines(int fd, uint32_t ctx)
>> -{
>> -       DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
>> -       struct intel_engine_data engine_data = { };
>> -
>> -       if (!gem_topology_get_param(fd, &param) && !param.size) {
>> -               query_engine_list(fd, &engine_data);
>> -               ctx_map_engines(fd, &engine_data, &param);
>> -       }
>> -}
> 
> Me likes.
> -Chris
>
Chris Wilson Jan. 23, 2020, 1:16 p.m. UTC | #3 
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@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@intel.com>
> >> Suggested-by: Chris Wilson <chris@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.

Nothing else to complain about,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Tvrtko Ursulin Jan. 23, 2020, 2:01 p.m. UTC | #4 
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@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@intel.com>
>>>> Suggested-by: Chris Wilson <chris@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?

> Nothing else to complain about,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

So this is conditional on rewriting it as above or not?

Regards,

Tvrtko
Chris Wilson Jan. 23, 2020, 2:16 p.m. UTC | #5 
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@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@intel.com>
> >>>> Suggested-by: Chris Wilson <chris@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.

> > Nothing else to complain about,
> > Reviewed-by: Chris Wilson <chris@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 :)
-Chris
Tvrtko Ursulin Jan. 23, 2020, 2:48 p.m. UTC | #6 
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@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@intel.com>
>>>>>> Suggested-by: Chris Wilson <chris@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@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
Chris Wilson Jan. 23, 2020, 2:51 p.m. UTC | #7 
Quoting Tvrtko Ursulin (2020-01-23 14:48:24)
> 
> 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:
> >>> 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.

Yeah, depends on whether you want to see the error for the underlying
ioctl and then have to look at the stack to work out we are in clone, or
if the clone itself is the interesting detail that wants to standout in
the assert message :)
-Chris
diff mbox series

Patch

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);
+	else
+		return gem_context_clone(i915, src, 0,
+					 I915_CONTEXT_CLONE_ENGINES);
+}
+
 uint32_t gem_queue_create(int i915)
 {
 	return gem_context_clone(i915, 0,
@@ -379,6 +423,21 @@  uint32_t gem_queue_create(int i915)
 				 I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE);
 }
 
+/**
+ * gem_queue_clone_with_engines:
+ * @i915: open i915 drm file descriptor
+ * @src: i915 context id
+ *
+ * See gem_context_clone_with_engines.
+ */
+uint32_t gem_queue_clone_with_engines(int i915, uint32_t src)
+{
+	return gem_context_clone(i915, src,
+				 I915_CONTEXT_CLONE_ENGINES |
+				 I915_CONTEXT_CLONE_VM,
+				 I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE);
+}
+
 bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
 {
 	struct drm_i915_gem_exec_object2 exec = {};
diff --git a/lib/i915/gem_context.h b/lib/i915/gem_context.h
index c0d4c9615677..cf2ba33fee8f 100644
--- a/lib/i915/gem_context.h
+++ b/lib/i915/gem_context.h
@@ -41,8 +41,10 @@  int __gem_context_clone(int i915,
 uint32_t gem_context_clone(int i915,
 			   uint32_t src, unsigned int share,
 			   unsigned int flags);
+uint32_t gem_context_clone_with_engines(int i915, uint32_t src);
 
 uint32_t gem_queue_create(int i915);
+uint32_t gem_queue_clone_with_engines(int i915, uint32_t src);
 
 bool gem_contexts_has_shared_gtt(int i915);
 bool gem_has_queues(int i915);
@@ -52,6 +54,8 @@  void gem_require_contexts(int fd);
 void gem_context_require_bannable(int fd);
 void gem_context_require_param(int fd, uint64_t param);
 
+bool gem_has_context_clone(int i915);
+
 void gem_context_get_param(int fd, struct drm_i915_gem_context_param *p);
 void gem_context_set_param(int fd, struct drm_i915_gem_context_param *p);
 int __gem_context_set_param(int fd, struct drm_i915_gem_context_param *p);
diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
index 989a6e26d6ef..43a99e0ff680 100644
--- a/lib/i915/gem_engine_topology.c
+++ b/lib/i915/gem_engine_topology.c
@@ -274,17 +274,6 @@  int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
 	return 0;
 }
 
-void gem_context_set_all_engines(int fd, uint32_t ctx)
-{
-	DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
-	struct intel_engine_data engine_data = { };
-
-	if (!gem_topology_get_param(fd, &param) && !param.size) {
-		query_engine_list(fd, &engine_data);
-		ctx_map_engines(fd, &engine_data, &param);
-	}
-}
-
 bool gem_has_engine_topology(int fd)
 {
 	struct drm_i915_gem_context_param param = {
diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
index 525741cc8a08..e40d7ec8320c 100644
--- a/lib/i915/gem_engine_topology.h
+++ b/lib/i915/gem_engine_topology.h
@@ -51,8 +51,6 @@  void intel_next_engine(struct intel_engine_data *ed);
 int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
 			      struct intel_execution_engine2 *e);
 
-void gem_context_set_all_engines(int fd, uint32_t ctx);
-
 bool gem_context_has_engine_map(int fd, uint32_t ctx);
 
 bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
diff --git a/tests/i915/gem_ctx_clone.c b/tests/i915/gem_ctx_clone.c
index 896b24dce39c..471031d4245b 100644
--- a/tests/i915/gem_ctx_clone.c
+++ b/tests/i915/gem_ctx_clone.c
@@ -40,19 +40,6 @@  static int ctx_create_ioctl(int i915, struct drm_i915_gem_context_create_ext *ar
 	return err;
 }
 
-static bool has_ctx_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),
-	};
-	return ctx_create_ioctl(i915, &create) == -ENOENT;
-}
-
 static void invalid_clone(int i915)
 {
 	struct drm_i915_gem_context_create_ext_clone ext = {
@@ -436,7 +423,7 @@  igt_main
 		igt_require_gem(i915);
 		gem_require_contexts(i915);
 
-		igt_require(has_ctx_clone(i915));
+		igt_require(gem_has_context_clone(i915));
 		igt_fork_hang_detector(i915);
 	}
 
diff --git a/tests/i915/gem_ctx_switch.c b/tests/i915/gem_ctx_switch.c
index 6bbd24972676..a65d1b02f5ee 100644
--- a/tests/i915/gem_ctx_switch.c
+++ b/tests/i915/gem_ctx_switch.c
@@ -63,10 +63,8 @@  static int measure_qlen(int fd,
 	uint32_t ctx[64];
 	int min = INT_MAX, max = 0;
 
-	for (int i = 0; i < ARRAY_SIZE(ctx); i++) {
-		ctx[i] = gem_context_create(fd);
-		gem_context_set_all_engines(fd, ctx[i]);
-	}
+	for (int i = 0; i < ARRAY_SIZE(ctx); i++)
+		ctx[i] = gem_context_clone_with_engines(fd, 0);
 
 	for (unsigned int n = 0; n < engines->nengines; n++) {
 		uint64_t saved = execbuf->flags;
@@ -130,12 +128,9 @@  static void single(int fd, uint32_t handle,
 
 	for (n = 0; n < 64; n++) {
 		if (flags & QUEUE)
-			contexts[n] = gem_queue_create(fd);
+			contexts[n] = gem_queue_clone_with_engines(fd, 0);
 		else
-			contexts[n] = gem_context_create(fd);
-
-		if (gem_context_has_engine_map(fd, 0))
-			gem_context_set_all_engines(fd, contexts[n]);
+			contexts[n] = gem_context_clone_with_engines(fd, 0);
 	}
 
 	memset(&obj, 0, sizeof(obj));
@@ -237,11 +232,9 @@  static void all(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	for (n = 0; n < ARRAY_SIZE(contexts); n++) {
 		if (flags & QUEUE)
-			contexts[n] = gem_queue_create(fd);
+			contexts[n] = gem_queue_clone_with_engines(fd, 0);
 		else
-			contexts[n] = gem_context_create(fd);
-
-		gem_context_set_all_engines(fd, contexts[n]);
+			contexts[n] = gem_context_clone_with_engines(fd, 0);
 	}
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/i915/gem_exec_parallel.c b/tests/i915/gem_exec_parallel.c
index 56b26cf4dd7b..cfbe78070873 100644
--- a/tests/i915/gem_exec_parallel.c
+++ b/tests/i915/gem_exec_parallel.c
@@ -127,8 +127,7 @@  static void *thread(void *data)
 	if (t->gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
 	if (t->flags & CONTEXTS) {
-		execbuf.rsvd1 = gem_context_create(fd);
-		gem_context_set_all_engines(fd, execbuf.rsvd1);
+		execbuf.rsvd1 = gem_context_clone_with_engines(fd, 0);
 	}
 
 	for (i = 0; i < 16; i++) {
diff --git a/tests/i915/gem_spin_batch.c b/tests/i915/gem_spin_batch.c
index c5114ac79b0e..1142a77c7d7f 100644
--- a/tests/i915/gem_spin_batch.c
+++ b/tests/i915/gem_spin_batch.c
@@ -73,9 +73,10 @@  static void spin(int fd, const struct intel_execution_engine2 *e2,
 static void spin_resubmit(int fd, const struct intel_execution_engine2 *e2,
 			  unsigned int flags)
 {
-	const uint32_t ctx0 = gem_context_create(fd);
-	const uint32_t ctx1 = (flags & RESUBMIT_NEW_CTX) ?
-		gem_context_create(fd) : ctx0;
+	const uint32_t ctx0 = gem_context_clone_with_engines(fd, 0);
+	const uint32_t ctx1 =
+		(flags & RESUBMIT_NEW_CTX) ?
+		gem_context_clone_with_engines(fd, 0) : ctx0;
 	igt_spin_t *spin = __igt_spin_new(fd, .ctx = ctx0, .engine = e2->flags);
 	const struct intel_execution_engine2 *other;
 
@@ -89,10 +90,6 @@  static void spin_resubmit(int fd, const struct intel_execution_engine2 *e2,
 		   !(flags & RESUBMIT_ALL_ENGINES));
 
 	if (flags & RESUBMIT_ALL_ENGINES) {
-		gem_context_set_all_engines(fd, ctx0);
-		if (ctx0 != ctx1)
-			gem_context_set_all_engines(fd, ctx1);
-
 		for_each_context_engine(fd, ctx1, other) {
 			if (gem_engine_is_equal(other, e2))
 				continue;
diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
index 3e179daef9d5..e17efe293f96 100644
--- a/tests/perf_pmu.c
+++ b/tests/perf_pmu.c
@@ -361,8 +361,7 @@  busy_double_start(int gem_fd, const struct intel_execution_engine2 *e)
 	uint32_t ctx;
 	int fd;
 
-	ctx = gem_context_create(gem_fd);
-	gem_context_set_all_engines(gem_fd, ctx);
+	ctx = gem_context_clone_with_engines(gem_fd, 0);
 
 	/*
 	 * Defeat the busy stats delayed disable, we need to guarantee we are