diff mbox series

[i-g-t,24/25] gem_wsim: Discover engines

Message ID 20190517112526.6738-25-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Media scalability tooling | expand

Commit Message

Tvrtko Ursulin May 17, 2019, 11:25 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of hardcoding the VCS balancing engines, discover, both with the
new engines query, or with the legacy get_param in the fallback case, so
class based addressing always works.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 180 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 173 insertions(+), 7 deletions(-)

Comments

Andi Shyti May 17, 2019, 11:39 a.m. UTC | #1 
Hi Tvrtko,

> +static int
> +__i915_query(int i915, struct drm_i915_query *q)
> +{
> +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int
> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	struct drm_i915_query q = {
> +		.num_items = n_items,
> +		.items_ptr = to_user_pointer(items),
> +	};
> +	return __i915_query(i915, &q);
> +}
> +
> +static void
> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
> +}
> +
> +static bool has_query(int i915)
> +{
> +	struct drm_i915_query query = {};
> +
> +	return __i915_query(i915, &query) == 0;
> +}
> +
> +static bool has_engine_query(int i915)
> +{
> +	struct drm_i915_query_item item = {
> +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
> +	};
> +
> +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
> +}
> +
> +static void query_engines(void)
> +{

[...]

> +		struct drm_i915_query_engine_info *engine_info;
> +		struct drm_i915_query_item item = {
> +			.query_id = DRM_I915_QUERY_ENGINE_INFO,
> +		};
> +		const unsigned int sz = 4096;
> +		unsigned int i;
> +
> +		engine_info = malloc(sz);
> +		igt_assert(engine_info);
> +		memset(engine_info, 0, sz);
> +
> +		item.data_ptr = to_user_pointer(engine_info);
> +		item.length = sz;
> +
> +		i915_query_items(fd, &item, 1);
> +		igt_assert(item.length > 0);
> +		igt_assert(item.length <= sz);
> +
> +		num = engine_info->num_engines;
> +
> +		engines = calloc(num,
> +				 sizeof(struct i915_engine_class_instance));
> +		igt_assert(engines);
> +
> +		for (i = 0; i < num; i++) {
> +			struct drm_i915_engine_info *engine =
> +				(struct drm_i915_engine_info *)&engine_info->engines[i];
> +
> +			engines[i] = engine->engine;
> +		}
> +	}
> +
> +	__engines = engines;
> +	__num_engines = num;
> +}

would it make sense to make a library out of all the above? e.g.
gem_engine_topology does similar thing (all static functions like
here, though).

Andi
Tvrtko Ursulin May 17, 2019, 11:51 a.m. UTC | #2 
On 17/05/2019 12:39, Andi Shyti wrote:
> Hi Tvrtko,
> 
>> +static int
>> +__i915_query(int i915, struct drm_i915_query *q)
>> +{
>> +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
>> +		return -errno;
>> +	return 0;
>> +}
>> +
>> +static int
>> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
>> +{
>> +	struct drm_i915_query q = {
>> +		.num_items = n_items,
>> +		.items_ptr = to_user_pointer(items),
>> +	};
>> +	return __i915_query(i915, &q);
>> +}
>> +
>> +static void
>> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
>> +{
>> +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
>> +}
>> +
>> +static bool has_query(int i915)
>> +{
>> +	struct drm_i915_query query = {};
>> +
>> +	return __i915_query(i915, &query) == 0;
>> +}
>> +
>> +static bool has_engine_query(int i915)
>> +{
>> +	struct drm_i915_query_item item = {
>> +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
>> +	};
>> +
>> +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
>> +}
>> +
>> +static void query_engines(void)
>> +{
> 
> [...]
> 
>> +		struct drm_i915_query_engine_info *engine_info;
>> +		struct drm_i915_query_item item = {
>> +			.query_id = DRM_I915_QUERY_ENGINE_INFO,
>> +		};
>> +		const unsigned int sz = 4096;
>> +		unsigned int i;
>> +
>> +		engine_info = malloc(sz);
>> +		igt_assert(engine_info);
>> +		memset(engine_info, 0, sz);
>> +
>> +		item.data_ptr = to_user_pointer(engine_info);
>> +		item.length = sz;
>> +
>> +		i915_query_items(fd, &item, 1);
>> +		igt_assert(item.length > 0);
>> +		igt_assert(item.length <= sz);
>> +
>> +		num = engine_info->num_engines;
>> +
>> +		engines = calloc(num,
>> +				 sizeof(struct i915_engine_class_instance));
>> +		igt_assert(engines);
>> +
>> +		for (i = 0; i < num; i++) {
>> +			struct drm_i915_engine_info *engine =
>> +				(struct drm_i915_engine_info *)&engine_info->engines[i];
>> +
>> +			engines[i] = engine->engine;
>> +		}
>> +	}
>> +
>> +	__engines = engines;
>> +	__num_engines = num;
>> +}
> 
> would it make sense to make a library out of all the above? e.g.
> gem_engine_topology does similar thing (all static functions like
> here, though).

Definitely yes, but coordinating all series seems tricky. I think best 
would be to consolidate once everything gets merged?

Regards,

Tvrtko
Andi Shyti May 17, 2019, 11:55 a.m. UTC | #3 
Hi Tvrtko,

> > > +static int
> > > +__i915_query(int i915, struct drm_i915_query *q)
> > > +{
> > > +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
> > > +		return -errno;
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> > > +{
> > > +	struct drm_i915_query q = {
> > > +		.num_items = n_items,
> > > +		.items_ptr = to_user_pointer(items),
> > > +	};
> > > +	return __i915_query(i915, &q);
> > > +}
> > > +
> > > +static void
> > > +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> > > +{
> > > +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
> > > +}
> > > +
> > > +static bool has_query(int i915)
> > > +{
> > > +	struct drm_i915_query query = {};
> > > +
> > > +	return __i915_query(i915, &query) == 0;
> > > +}
> > > +
> > > +static bool has_engine_query(int i915)
> > > +{
> > > +	struct drm_i915_query_item item = {
> > > +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
> > > +	};
> > > +
> > > +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
> > > +}
> > > +
> > > +static void query_engines(void)
> > > +{
> > 
> > [...]
> > 
> > > +		struct drm_i915_query_engine_info *engine_info;
> > > +		struct drm_i915_query_item item = {
> > > +			.query_id = DRM_I915_QUERY_ENGINE_INFO,
> > > +		};
> > > +		const unsigned int sz = 4096;
> > > +		unsigned int i;
> > > +
> > > +		engine_info = malloc(sz);
> > > +		igt_assert(engine_info);
> > > +		memset(engine_info, 0, sz);
> > > +
> > > +		item.data_ptr = to_user_pointer(engine_info);
> > > +		item.length = sz;
> > > +
> > > +		i915_query_items(fd, &item, 1);
> > > +		igt_assert(item.length > 0);
> > > +		igt_assert(item.length <= sz);
> > > +
> > > +		num = engine_info->num_engines;
> > > +
> > > +		engines = calloc(num,
> > > +				 sizeof(struct i915_engine_class_instance));
> > > +		igt_assert(engines);
> > > +
> > > +		for (i = 0; i < num; i++) {
> > > +			struct drm_i915_engine_info *engine =
> > > +				(struct drm_i915_engine_info *)&engine_info->engines[i];
> > > +
> > > +			engines[i] = engine->engine;
> > > +		}
> > > +	}
> > > +
> > > +	__engines = engines;
> > > +	__num_engines = num;
> > > +}
> > 
> > would it make sense to make a library out of all the above? e.g.
> > gem_engine_topology does similar thing (all static functions like
> > here, though).
> 
> Definitely yes, but coordinating all series seems tricky. I think best would
> be to consolidate once everything gets merged?

yes, sure! let's get everything in first :)

Andi
Andi Shyti May 17, 2019, 12:10 p.m. UTC | #4 
On Fri, May 17, 2019 at 12:25:25PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of hardcoding the VCS balancing engines, discover, both with the
> new engines query, or with the legacy get_param in the fallback case, so
> class based addressing always works.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/gem_wsim.c | 180 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 173 insertions(+), 7 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index d43e7c767801..539de243f6e8 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -365,34 +365,198 @@ static int str_to_engine(const char *str)
>  	return -1;
>  }
>  
> +static bool __engines_queried;
> +static unsigned int __num_engines;
> +static struct i915_engine_class_instance *__engines;
> +
> +static int
> +__i915_query(int i915, struct drm_i915_query *q)
> +{
> +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int
> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	struct drm_i915_query q = {
> +		.num_items = n_items,
> +		.items_ptr = to_user_pointer(items),
> +	};
> +	return __i915_query(i915, &q);
> +}
> +
> +static void
> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
> +}
> +
> +static bool has_query(int i915)
> +{
> +	struct drm_i915_query query = {};
> +
> +	return __i915_query(i915, &query) == 0;
> +}
> +
> +static bool has_engine_query(int i915)
> +{
> +	struct drm_i915_query_item item = {
> +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
> +	};
> +
> +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
> +}
> +
> +static void query_engines(void)
> +{
> +	struct i915_engine_class_instance *engines;
> +	unsigned int num;
> +
> +	if (__engines_queried)
> +		return;
> +
> +	__engines_queried = true;
> +
> +	if (!has_query(fd) || !has_engine_query(fd)) {

One question, still. What is the real use of this check and
'has_query' that is used only here.

I mean... here you want to check whether the "ioctl is not
implemented" or "ioctl is not implemented and length  is 0".

Wouldn't in this case just '!has_engine_query()' be enough? or
have I missed any case?

> +		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
> +		unsigned int i = 0;
> +
> +		igt_assert(num);
> +
> +		num = 1 + num_bsd;

did you mean the above two lines swapped?

> +
> +		if (gem_has_blt(fd))
> +			num++;
> +
> +		if (gem_has_vebox(fd))
> +			num++;
> +
> +		engines = calloc(num,
> +				 sizeof(struct i915_engine_class_instance));
> +		igt_assert(engines);
> +
> +		engines[i].engine_class = I915_ENGINE_CLASS_RENDER;
> +		engines[i].engine_instance = 0;
> +		i++;
> +
> +		if (gem_has_blt(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_COPY;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}
> +
> +		if (gem_has_bsd(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}
> +
> +		if (gem_has_bsd2(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
> +			engines[i].engine_instance = 1;
> +			i++;
> +		}
> +
> +		if (gem_has_vebox(fd)) {
> +			engines[i].engine_class =
> +				I915_ENGINE_CLASS_VIDEO_ENHANCE;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}

mmhhh... isn't this the intel_execution_engine2[]? Yet another
way for having engine list... in the long run, updating here (as
well) won't be easy to remember.

Andi
Tvrtko Ursulin May 17, 2019, 12:19 p.m. UTC | #5 
On 17/05/2019 13:10, Andi Shyti wrote:
> On Fri, May 17, 2019 at 12:25:25PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Instead of hardcoding the VCS balancing engines, discover, both with the
>> new engines query, or with the legacy get_param in the fallback case, so
>> class based addressing always works.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 180 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 173 insertions(+), 7 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index d43e7c767801..539de243f6e8 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -365,34 +365,198 @@ static int str_to_engine(const char *str)
>>   	return -1;
>>   }
>>   
>> +static bool __engines_queried;
>> +static unsigned int __num_engines;
>> +static struct i915_engine_class_instance *__engines;
>> +
>> +static int
>> +__i915_query(int i915, struct drm_i915_query *q)
>> +{
>> +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
>> +		return -errno;
>> +	return 0;
>> +}
>> +
>> +static int
>> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
>> +{
>> +	struct drm_i915_query q = {
>> +		.num_items = n_items,
>> +		.items_ptr = to_user_pointer(items),
>> +	};
>> +	return __i915_query(i915, &q);
>> +}
>> +
>> +static void
>> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
>> +{
>> +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
>> +}
>> +
>> +static bool has_query(int i915)
>> +{
>> +	struct drm_i915_query query = {};
>> +
>> +	return __i915_query(i915, &query) == 0;
>> +}
>> +
>> +static bool has_engine_query(int i915)
>> +{
>> +	struct drm_i915_query_item item = {
>> +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
>> +	};
>> +
>> +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
>> +}
>> +
>> +static void query_engines(void)
>> +{
>> +	struct i915_engine_class_instance *engines;
>> +	unsigned int num;
>> +
>> +	if (__engines_queried)
>> +		return;
>> +
>> +	__engines_queried = true;
>> +
>> +	if (!has_query(fd) || !has_engine_query(fd)) {
> 
> One question, still. What is the real use of this check and
> 'has_query' that is used only here.
> 
> I mean... here you want to check whether the "ioctl is not
> implemented" or "ioctl is not implemented and length  is 0".
> 
> Wouldn't in this case just '!has_engine_query()' be enough? or
> have I missed any case?

You haven't missed anything. I have been pointlessly verbose and a bit 
lazy by copy-pasting a lot.

has_engine_query is a superset of has_query for the purpose of ioctl 
detection.

> 
>> +		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
>> +		unsigned int i = 0;
>> +
>> +		igt_assert(num);
>> +
>> +		num = 1 + num_bsd;
> 
> did you mean the above two lines swapped?

No, I want to avoid running on platforms with no vcs engines since no 
one ever tested gem_wsim there.

>> +
>> +		if (gem_has_blt(fd))
>> +			num++;
>> +
>> +		if (gem_has_vebox(fd))
>> +			num++;
>> +
>> +		engines = calloc(num,
>> +				 sizeof(struct i915_engine_class_instance));
>> +		igt_assert(engines);
>> +
>> +		engines[i].engine_class = I915_ENGINE_CLASS_RENDER;
>> +		engines[i].engine_instance = 0;
>> +		i++;
>> +
>> +		if (gem_has_blt(fd)) {
>> +			engines[i].engine_class = I915_ENGINE_CLASS_COPY;
>> +			engines[i].engine_instance = 0;
>> +			i++;
>> +		}
>> +
>> +		if (gem_has_bsd(fd)) {
>> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
>> +			engines[i].engine_instance = 0;
>> +			i++;
>> +		}
>> +
>> +		if (gem_has_bsd2(fd)) {
>> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
>> +			engines[i].engine_instance = 1;
>> +			i++;
>> +		}
>> +
>> +		if (gem_has_vebox(fd)) {
>> +			engines[i].engine_class =
>> +				I915_ENGINE_CLASS_VIDEO_ENHANCE;
>> +			engines[i].engine_instance = 0;
>> +			i++;
>> +		}
> 
> mmhhh... isn't this the intel_execution_engine2[]? Yet another
> way for having engine list... in the long run, updating here (as
> well) won't be easy to remember.

Not here, gem_wsim uses some of the IGT libraries, but should keep it at 
minimum. So I think we don't want to pull in the engine array etc.

Regards,

Tvrtko
Andi Shyti May 17, 2019, 1:02 p.m. UTC | #6 
> > > +static void query_engines(void)
> > > +{
> > > +	struct i915_engine_class_instance *engines;
> > > +	unsigned int num;
> > > +
> > > +	if (__engines_queried)
> > > +		return;
> > > +
> > > +	__engines_queried = true;
> > > +
> > > +	if (!has_query(fd) || !has_engine_query(fd)) {

[...]

> > > +		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
> > > +		unsigned int i = 0;
> > > +
> > > +		igt_assert(num);
> > > +
> > > +		num = 1 + num_bsd;
> > 
> > did you mean the above two lines swapped?
> 
> No, I want to avoid running on platforms with no vcs engines since no one
> ever tested gem_wsim there.

but you are asserting on 'num' while num is not initialized.

so that I guess it should first be

	num = 1 + num_bsd;

and then

	igt_assert(num);

right?

Andi
Tvrtko Ursulin May 17, 2019, 1:05 p.m. UTC | #7 
On 17/05/2019 14:02, Andi Shyti wrote:
>>>> +static void query_engines(void)
>>>> +{
>>>> +	struct i915_engine_class_instance *engines;
>>>> +	unsigned int num;
>>>> +
>>>> +	if (__engines_queried)
>>>> +		return;
>>>> +
>>>> +	__engines_queried = true;
>>>> +
>>>> +	if (!has_query(fd) || !has_engine_query(fd)) {
> 
> [...]
> 
>>>> +		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
>>>> +		unsigned int i = 0;
>>>> +
>>>> +		igt_assert(num);
>>>> +
>>>> +		num = 1 + num_bsd;
>>>
>>> did you mean the above two lines swapped?
>>
>> No, I want to avoid running on platforms with no vcs engines since no one
>> ever tested gem_wsim there.
> 
> but you are asserting on 'num' while num is not initialized.

True, my bad. Where are compiler warnings when you need them.

> so that I guess it should first be
> 
> 	num = 1 + num_bsd;
> 
> and then
> 
> 	igt_assert(num);
> 
> right?

I just want igt_assert(num_bsd).

Regards,

Tvrtko
Chris Wilson May 17, 2019, 7:50 p.m. UTC | #8 
Quoting Tvrtko Ursulin (2019-05-17 12:51:06)
> 
> On 17/05/2019 12:39, Andi Shyti wrote:
> > Hi Tvrtko,
> > 
> >> +static int
> >> +__i915_query(int i915, struct drm_i915_query *q)
> >> +{
> >> +    if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
> >> +            return -errno;
> >> +    return 0;
> >> +}
> >> +
> >> +static int
> >> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> >> +{
> >> +    struct drm_i915_query q = {
> >> +            .num_items = n_items,
> >> +            .items_ptr = to_user_pointer(items),
> >> +    };
> >> +    return __i915_query(i915, &q);
> >> +}
> >> +
> >> +static void
> >> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> >> +{
> >> +    igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
> >> +}
> >> +
> >> +static bool has_query(int i915)
> >> +{
> >> +    struct drm_i915_query query = {};
> >> +
> >> +    return __i915_query(i915, &query) == 0;
> >> +}
> >> +
> >> +static bool has_engine_query(int i915)
> >> +{
> >> +    struct drm_i915_query_item item = {
> >> +            .query_id = DRM_I915_QUERY_ENGINE_INFO,
> >> +    };
> >> +
> >> +    return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
> >> +}
> >> +
> >> +static void query_engines(void)
> >> +{
> > 
> > [...]
> > 
> >> +            struct drm_i915_query_engine_info *engine_info;
> >> +            struct drm_i915_query_item item = {
> >> +                    .query_id = DRM_I915_QUERY_ENGINE_INFO,
> >> +            };
> >> +            const unsigned int sz = 4096;
> >> +            unsigned int i;
> >> +
> >> +            engine_info = malloc(sz);
> >> +            igt_assert(engine_info);
> >> +            memset(engine_info, 0, sz);
> >> +
> >> +            item.data_ptr = to_user_pointer(engine_info);
> >> +            item.length = sz;
> >> +
> >> +            i915_query_items(fd, &item, 1);
> >> +            igt_assert(item.length > 0);
> >> +            igt_assert(item.length <= sz);
> >> +
> >> +            num = engine_info->num_engines;
> >> +
> >> +            engines = calloc(num,
> >> +                             sizeof(struct i915_engine_class_instance));
> >> +            igt_assert(engines);
> >> +
> >> +            for (i = 0; i < num; i++) {
> >> +                    struct drm_i915_engine_info *engine =
> >> +                            (struct drm_i915_engine_info *)&engine_info->engines[i];
> >> +
> >> +                    engines[i] = engine->engine;
> >> +            }
> >> +    }
> >> +
> >> +    __engines = engines;
> >> +    __num_engines = num;
> >> +}
> > 
> > would it make sense to make a library out of all the above? e.g.
> > gem_engine_topology does similar thing (all static functions like
> > here, though).
> 
> Definitely yes, but coordinating all series seems tricky. I think best 
> would be to consolidate once everything gets merged?

The challenge is carving out the core into a separate library that
doesn't pull libigt.la in. (Tvrtko has already committed the cardinal sin
of using libigt outside of tests/.) At which point, you have just a
bunch of ioctl wrappers, and fwiw some of us may wish gem_wsim itself
was a scripting engine...
-Chris
diff mbox series

Patch

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index d43e7c767801..539de243f6e8 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -365,34 +365,198 @@  static int str_to_engine(const char *str)
 	return -1;
 }
 
+static bool __engines_queried;
+static unsigned int __num_engines;
+static struct i915_engine_class_instance *__engines;
+
+static int
+__i915_query(int i915, struct drm_i915_query *q)
+{
+	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
+		return -errno;
+	return 0;
+}
+
+static int
+__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
+{
+	struct drm_i915_query q = {
+		.num_items = n_items,
+		.items_ptr = to_user_pointer(items),
+	};
+	return __i915_query(i915, &q);
+}
+
+static void
+i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
+{
+	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
+}
+
+static bool has_query(int i915)
+{
+	struct drm_i915_query query = {};
+
+	return __i915_query(i915, &query) == 0;
+}
+
+static bool has_engine_query(int i915)
+{
+	struct drm_i915_query_item item = {
+		.query_id = DRM_I915_QUERY_ENGINE_INFO,
+	};
+
+	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
+}
+
+static void query_engines(void)
+{
+	struct i915_engine_class_instance *engines;
+	unsigned int num;
+
+	if (__engines_queried)
+		return;
+
+	__engines_queried = true;
+
+	if (!has_query(fd) || !has_engine_query(fd)) {
+		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
+		unsigned int i = 0;
+
+		igt_assert(num);
+
+		num = 1 + num_bsd;
+
+		if (gem_has_blt(fd))
+			num++;
+
+		if (gem_has_vebox(fd))
+			num++;
+
+		engines = calloc(num,
+				 sizeof(struct i915_engine_class_instance));
+		igt_assert(engines);
+
+		engines[i].engine_class = I915_ENGINE_CLASS_RENDER;
+		engines[i].engine_instance = 0;
+		i++;
+
+		if (gem_has_blt(fd)) {
+			engines[i].engine_class = I915_ENGINE_CLASS_COPY;
+			engines[i].engine_instance = 0;
+			i++;
+		}
+
+		if (gem_has_bsd(fd)) {
+			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
+			engines[i].engine_instance = 0;
+			i++;
+		}
+
+		if (gem_has_bsd2(fd)) {
+			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
+			engines[i].engine_instance = 1;
+			i++;
+		}
+
+		if (gem_has_vebox(fd)) {
+			engines[i].engine_class =
+				I915_ENGINE_CLASS_VIDEO_ENHANCE;
+			engines[i].engine_instance = 0;
+			i++;
+		}
+	} else {
+		struct drm_i915_query_engine_info *engine_info;
+		struct drm_i915_query_item item = {
+			.query_id = DRM_I915_QUERY_ENGINE_INFO,
+		};
+		const unsigned int sz = 4096;
+		unsigned int i;
+
+		engine_info = malloc(sz);
+		igt_assert(engine_info);
+		memset(engine_info, 0, sz);
+
+		item.data_ptr = to_user_pointer(engine_info);
+		item.length = sz;
+
+		i915_query_items(fd, &item, 1);
+		igt_assert(item.length > 0);
+		igt_assert(item.length <= sz);
+
+		num = engine_info->num_engines;
+
+		engines = calloc(num,
+				 sizeof(struct i915_engine_class_instance));
+		igt_assert(engines);
+
+		for (i = 0; i < num; i++) {
+			struct drm_i915_engine_info *engine =
+				(struct drm_i915_engine_info *)&engine_info->engines[i];
+
+			engines[i] = engine->engine;
+		}
+	}
+
+	__engines = engines;
+	__num_engines = num;
+}
+
 static unsigned int num_engines_in_class(enum intel_engine_id class)
 {
+	unsigned int i, count = 0;
+
 	igt_assert(class == VCS);
 
-	return 2;
+	query_engines();
+
+	for (i = 0; i < __num_engines; i++) {
+		if (__engines[i].engine_class == I915_ENGINE_CLASS_VIDEO)
+			count++;
+	}
+
+	igt_assert(count);
+	return count;
 }
 
 static void
 fill_engines_class(struct i915_engine_class_instance *ci,
 		   enum intel_engine_id class)
 {
+	unsigned int i, j = 0;
+
 	igt_assert(class == VCS);
 
-	ci[0].engine_class = I915_ENGINE_CLASS_VIDEO;
-	ci[0].engine_instance = 0;
+	query_engines();
 
-	ci[1].engine_class = I915_ENGINE_CLASS_VIDEO;
-	ci[1].engine_instance = 1;
+	for (i = 0; i < __num_engines; i++) {
+		if (__engines[i].engine_class != I915_ENGINE_CLASS_VIDEO)
+			continue;
+
+		ci[j].engine_class = __engines[i].engine_class;
+		ci[j].engine_instance = __engines[i].engine_instance;
+		j++;
+	}
 }
 
 static void
 fill_engines_id_class(enum intel_engine_id *list,
 		      enum intel_engine_id class)
 {
+	enum intel_engine_id engine = VCS1;
+	unsigned int i, j = 0;
+
 	igt_assert(class == VCS);
+	igt_assert(num_engines_in_class(VCS) <= 2);
+
+	query_engines();
 
-	list[0] = VCS1;
-	list[1] = VCS2;
+	for (i = 0; i < __num_engines; i++) {
+		if (__engines[i].engine_class != I915_ENGINE_CLASS_VIDEO)
+			continue;
+
+		list[j++] = engine++;
+	}
 }
 
 static struct i915_engine_class_instance
@@ -400,6 +564,8 @@  get_engine(enum intel_engine_id engine)
 {
 	struct i915_engine_class_instance ci;
 
+	query_engines();
+
 	switch (engine) {
 	case RCS:
 		ci.engine_class = I915_ENGINE_CLASS_RENDER;