Re: [PATCH i-g-t] tests/perf_pmu: Fix usage of for_each_engine_class_instance

[Michel Thierry <michel.thierry@xxxxxxxxx>]

On 29/03/18 02:11, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Wrong file descriptor was passed to the iterator. This had currently no
> effect, since it wasn't used in the macro, but needs to be fixed.
>
> At the same time make the macro consistent by checking for engine presence
> like the other iterators do.
>
> Added __for_each_engine_class_instance which does not check for engine
> presence and so is useful for enumerating all possible engines - like for
> instance for subtest enumeration.
>
> And another 'wrong fd used' fixlet in the render node subtests.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Reported-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>

Thanks,

Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
>   lib/igt_gt.h     | 12 +++++++-----
>   tests/perf_pmu.c | 30 ++++++++++--------------------
>   2 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index a517ed7b29a0..d44b7552f3c4 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -100,11 +100,6 @@ extern const struct intel_execution_engine2 {
>   	int instance;
>   } intel_execution_engines2[];
>   
> -#define for_each_engine_class_instance(fd__, e__) \
> -	for ((e__) = intel_execution_engines2;\
> -	     (e__)->name; \
> -	     (e__)++)
> -
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
>   			       enum drm_i915_gem_engine_class class,
> @@ -122,4 +117,11 @@ void gem_require_engine(int gem_fd,
>   	igt_require(gem_has_engine(gem_fd, class, instance));
>   }
>   
> +#define __for_each_engine_class_instance(fd__, e__) \
> +	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
> +
> +#define for_each_engine_class_instance(fd__, e__) \
> +	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++) \
> +		for_if (gem_has_engine((fd__), (e__)->class, (e__)->instance))
> +
>   #endif /* IGT_GT_H */
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index b59af81819c7..2273ddb9e684 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -434,10 +434,8 @@ busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>   
>   	i = 0;
>   	fd[0] = -1;
> -	for_each_engine_class_instance(fd, e_) {
> -		if (!gem_has_engine(gem_fd, e_->class, e_->instance))
> -			continue;
> -		else if (e == e_)
> +	for_each_engine_class_instance(gem_fd, e_) {
> +		if (e == e_)
>   			busy_idx = i;
>   
>   		fd[i++] = open_group(I915_PMU_ENGINE_BUSY(e_->class,
> @@ -499,10 +497,7 @@ most_busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>   	unsigned int idle_idx, i;
>   
>   	i = 0;
> -	for_each_engine_class_instance(fd, e_) {
> -		if (!gem_has_engine(gem_fd, e_->class, e_->instance))
> -			continue;
> -
> +	for_each_engine_class_instance(gem_fd, e_) {
>   		if (e == e_)
>   			idle_idx = i;
>   		else if (spin)
> @@ -559,10 +554,7 @@ all_busy_check_all(int gem_fd, const unsigned int num_engines,
>   	unsigned int i;
>   
>   	i = 0;
> -	for_each_engine_class_instance(fd, e) {
> -		if (!gem_has_engine(gem_fd, e->class, e->instance))
> -			continue;
> -
> +	for_each_engine_class_instance(gem_fd, e) {
>   		if (spin)
>   			__submit_spin_batch(gem_fd, spin, e, 64);
>   		else
> @@ -1677,10 +1669,8 @@ igt_main
>   		igt_require_gem(fd);
>   		igt_require(i915_type_id() > 0);
>   
> -		for_each_engine_class_instance(fd, e) {
> -			if (gem_has_engine(fd, e->class, e->instance))
> -				num_engines++;
> -		}
> +		for_each_engine_class_instance(fd, e)
> +			num_engines++;
>   	}
>   
>   	/**
> @@ -1689,7 +1679,7 @@ igt_main
>   	igt_subtest("invalid-init")
>   		invalid_init();
>   
> -	for_each_engine_class_instance(fd, e) {
> +	__for_each_engine_class_instance(fd, e) {
>   		const unsigned int pct[] = { 2, 50, 98 };
>   
>   		/**
> @@ -1888,7 +1878,7 @@ igt_main
>   			gem_quiescent_gpu(fd);
>   		}
>   
> -		for_each_engine_class_instance(fd, e) {
> +		__for_each_engine_class_instance(render_fd, e) {
>   			igt_subtest_group {
>   				igt_fixture {
>   					gem_require_engine(render_fd,
> @@ -1897,10 +1887,10 @@ igt_main
>   				}
>   
>   				igt_subtest_f("render-node-busy-%s", e->name)
> -					single(fd, e, TEST_BUSY);
> +					single(render_fd, e, TEST_BUSY);
>   				igt_subtest_f("render-node-busy-idle-%s",
>   					      e->name)
> -					single(fd, e,
> +					single(render_fd, e,
>   					       TEST_BUSY | TEST_TRAILING_IDLE);
>   			}
>   		}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx