Re: [igt-dev] [PATCH i-g-t 09/17] benchmarks/gem_wsim: use lib code to query engines

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 9/29/2023 10:11 AM, Tvrtko Ursulin wrote:
> 
> On 28/09/2023 18:45, Marcin Bernatowicz wrote:
>> Use code in lib/i915/gem_engine_topology to query engines.
>>
>> v2:
>> - keep i unsigned, restore igt_assert(count)
>>    in num_engines_in_class (Tvrtko)
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 153 +++++-------------------------------------
>>   1 file changed, 17 insertions(+), 136 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 10ad6cf75..8cd68058d 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -456,146 +456,27 @@ 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_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)
>> +static struct intel_engine_data *query_engines(void)
>>   {
>> -    struct i915_engine_class_instance *engines;
>> -    unsigned int num;
>> -
>> -    if (__engines_queried)
>> -        return;
>> -
>> -    __engines_queried = true;
>> -
>> -    if (!has_engine_query(fd)) {
>> -        unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
>> -        unsigned int i = 0;
>> -
>> -        igt_assert(num_bsd);
>> -
>> -        num = 1 + num_bsd;
>> +    static struct intel_engine_data engines = {};
>> -        if (gem_has_blt(fd))
>> -            num++;
>> +    if (engines.nengines)
>> +        return &engines;
>> -        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;
>> +    engines = intel_engine_list_of_physical(fd);
>> +    igt_assert(engines.nengines);
>> +    return &engines;
>>   }
>>   static unsigned int num_engines_in_class(enum intel_engine_id class)
>>   {
>> +    const struct intel_engine_data *engines = query_engines();
>>       unsigned int i, count = 0;
>>       igt_assert(class == VCS);
>> -    query_engines();
>> -
>> -    for (i = 0; i < __num_engines; i++) {
>> -        if (__engines[i].engine_class == I915_ENGINE_CLASS_VIDEO)
>> +    for (i = 0; i < engines->nengines; i++) {
>> +        if (engines->engines[i].class == I915_ENGINE_CLASS_VIDEO)
>>               count++;
>>       }
>> @@ -607,16 +488,15 @@ static void
>>   fill_engines_id_class(enum intel_engine_id *list,
>>                 enum intel_engine_id class)
>>   {
>> +    const struct intel_engine_data *engines = query_engines();
>>       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();
>> -
>> -    for (i = 0; i < __num_engines; i++) {
>> -        if (__engines[i].engine_class != I915_ENGINE_CLASS_VIDEO)
>> +    for (i = 0; i < engines->nengines; i++) {
>> +        if (engines->engines[i].class != I915_ENGINE_CLASS_VIDEO)
>>               continue;
>>           list[j++] = engine++;
>> @@ -626,17 +506,18 @@ fill_engines_id_class(enum intel_engine_id *list,
>>   static unsigned int
>>   find_physical_instance(enum intel_engine_id class, unsigned int 
>> logical)
>>   {
>> +    const struct intel_engine_data *engines = query_engines();
>>       unsigned int i, j = 0;
>>       igt_assert(class == VCS);
>> -    for (i = 0; i < __num_engines; i++) {
>> -        if (__engines[i].engine_class != I915_ENGINE_CLASS_VIDEO)
>> +    for (i = 0; i < engines->nengines; i++) {
>> +        if (engines->engines[i].class != I915_ENGINE_CLASS_VIDEO)
>>               continue;
>>           /* Map logical to physical instances. */
>>           if (logical == j++)
>> -            return __engines[i].engine_instance;
>> +            return engines->engines[i].instance;
>>       }
>>       igt_assert(0);
> 
> Looks fine - I couldn't spot a problem when I looked at the details a 
> little bit more during the last round of review. I trust you are smoke 
> testing against both drivers at least a little bit.

Yes, I'm checking few workloads: media_nn_1080p_s2.wsim, 
media_load_balance_fhd26u7.wsim and some artificial test_syntax.wsim
on both drivers.
--
marcin
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko