Re: [igt-dev] [PATCH i-g-t 13/14] benchmarks/gem_wsim: extract prepare working sets code to new function

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

On 26/09/2023 09:44, Marcin Bernatowicz wrote:
> No functional changes.
> Extracted prepare_working_sets function from prepare_workload.
> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>   benchmarks/gem_wsim.c | 106 +++++++++++++++++++++++-------------------
>   1 file changed, 58 insertions(+), 48 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index 55f8d9b1b..7703ca822 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -1964,10 +1964,66 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>   	return 0;
>   }
>   
> -static int prepare_workload(unsigned int id, struct workload *wrk)
> +static int prepare_working_sets(unsigned int id, struct workload *wrk)

Can return void? Come to think of it, previous patch does not use the 
return value either.

Otherwise code movement looks 1:1.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>   {
>   	struct working_set **sets;
>   	unsigned long total = 0;
> +	struct w_step *w;
> +	int i;
> +
> +	/*
> +	 * Allocate working sets.
> +	 */
> +	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> +		if (w->type == WORKINGSET && !w->working_set.shared)
> +			total += allocate_working_set(wrk, &w->working_set);
> +	}
> +
> +	if (verbose > 2)
> +		printf("%u: %lu bytes in working sets.\n", wrk->id, total);
> +
> +	/*
> +	 * Map of working set ids.
> +	 */
> +	wrk->max_working_set_id = -1;
> +	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> +		if (w->type == WORKINGSET &&
> +		    w->working_set.id > wrk->max_working_set_id)
> +			wrk->max_working_set_id = w->working_set.id;
> +	}
> +
> +	sets = wrk->working_sets;
> +	wrk->working_sets = calloc(wrk->max_working_set_id + 1,
> +				   sizeof(*wrk->working_sets));
> +	igt_assert(wrk->working_sets);
> +
> +	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> +		struct working_set *set;
> +
> +		if (w->type != WORKINGSET)
> +			continue;
> +
> +		if (!w->working_set.shared) {
> +			set = &w->working_set;
> +		} else {
> +			igt_assert(sets);
> +
> +			set = sets[w->working_set.id];
> +			igt_assert(set->shared);
> +			igt_assert(set->sizes);
> +		}
> +
> +		wrk->working_sets[w->working_set.id] = set;
> +	}
> +
> +	if (sets)
> +		free(sets);
> +
> +	return 0;
> +}
> +
> +static int prepare_workload(unsigned int id, struct workload *wrk)
> +{
>   	struct w_step *w;
>   	int i, j;
>   
> @@ -2019,53 +2075,7 @@ static int prepare_workload(unsigned int id, struct workload *wrk)
>   		}
>   	}
>   
> -	/*
> -	 * Allocate working sets.
> -	 */
> -	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> -		if (w->type == WORKINGSET && !w->working_set.shared)
> -			total += allocate_working_set(wrk, &w->working_set);
> -	}
> -
> -	if (verbose > 2)
> -		printf("%u: %lu bytes in working sets.\n", wrk->id, total);
> -
> -	/*
> -	 * Map of working set ids.
> -	 */
> -	wrk->max_working_set_id = -1;
> -	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> -		if (w->type == WORKINGSET &&
> -		    w->working_set.id > wrk->max_working_set_id)
> -			wrk->max_working_set_id = w->working_set.id;
> -	}
> -
> -	sets = wrk->working_sets;
> -	wrk->working_sets = calloc(wrk->max_working_set_id + 1,
> -				   sizeof(*wrk->working_sets));
> -	igt_assert(wrk->working_sets);
> -
> -	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> -		struct working_set *set;
> -
> -		if (w->type != WORKINGSET)
> -			continue;
> -
> -		if (!w->working_set.shared) {
> -			set = &w->working_set;
> -		} else {
> -			igt_assert(sets);
> -
> -			set = sets[w->working_set.id];
> -			igt_assert(set->shared);
> -			igt_assert(set->sizes);
> -		}
> -
> -		wrk->working_sets[w->working_set.id] = set;
> -	}
> -
> -	if (sets)
> -		free(sets);
> +	prepare_working_sets(id, wrk);
>   
>   	/*
>   	 * Allocate batch buffers.