Re: [PATCH i-g-t] tests/kms_plane: Implement dynamic level execution across multiple planes

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

Hi Pranay,

on quick look things seem ok, just small comments below.

On 21.5.2024 6.22, Pranay Samala wrote:
> The test runs on all pipes on dynamic level to test all the supported
> pixel-formats and modifiers on current plane of that pipe.
> 
> Updating the test to execute on multiple planes on dynamic level.
> 
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>   tests/kms_plane.c | 107 +++++++++++++++++++++++-----------------------
>   1 file changed, 54 insertions(+), 53 deletions(-)
> 
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 406aecc04..f476ed52f 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -275,7 +275,7 @@ test_plane_position_with_output(data_t *data,
>   	drmModeModeInfo *mode;
>   	igt_crc_t crc, crc2;
>   
> -	igt_info("Testing connector %s using pipe %s plane %d\n",
> +	igt_debug("Testing connector %s using pipe %s plane %d\n",
>   		 igt_output_name(output), kmstest_pipe_name(pipe), plane);
>   
>   	igt_output_set_pipe(output, pipe);
> @@ -285,7 +285,7 @@ test_plane_position_with_output(data_t *data,
>   	sprite = igt_output_get_plane(output, plane);
>   
>   	if (primary->drm_plane->plane_id > sprite->drm_plane->plane_id) {
> -		igt_info("primary plane ID (%d) > sprite plane ID (%d), skipping plane %d\n",
> +		igt_debug("primary plane ID (%d) > sprite plane ID (%d), skipping plane %d\n",
>   				primary->drm_plane->plane_id, sprite->drm_plane->plane_id, plane);

While changing these you could also fix the indentation of that 
"primary->dr.." line to match parenthesis.

>   		return;
>   	}
> @@ -352,9 +352,9 @@ test_plane_position(data_t *data, enum pipe pipe)
>   	test_grab_crc(data, output, pipe, &green, data->flags, &reference_crc);
>   
>   	for (int plane = 1; plane < n_planes; plane++)
> -		test_plane_position_with_output(data, pipe, plane,
> -						output, &reference_crc,
> -						data->flags);
> +		igt_dynamic_f("pipe-%s-plane-%d", kmstest_pipe_name(pipe), plane)
> +			test_plane_position_with_output(data, pipe, plane,
> +					output, &reference_crc,data->flags);
                                                                ^
space after comma

>   
>   	test_fini(data);
>   }
> @@ -415,7 +415,7 @@ test_plane_panning_with_output(data_t *data,
>   	mode = igt_output_get_mode(output);
>   	primary = igt_output_get_plane(output, 0);
>   
> -	igt_info("Testing connector %s using pipe %s, mode %s\n",
> +	igt_debug("Testing connector %s using pipe %s, mode %s\n",
>   		 igt_output_name(output), kmstest_pipe_name(pipe), mode->name);
>   
>   	create_fb_for_mode_panning(data, mode, &primary_fb);
> @@ -449,59 +449,60 @@ test_plane_panning_with_output(data_t *data,
>   static void
>   test_plane_panning(data_t *data, enum pipe pipe)
>   {
> -	bool mode_found = false;
> -	uint64_t mem_size = 0;
> -	igt_output_t *output = data->output;
> -	igt_crc_t ref_crc;
> -
> -	igt_info("Using (pipe %s + %s) to run the subtest.\n",
> -		 kmstest_pipe_name(pipe), igt_output_name(output));
> +	igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe)) {

You could put this into small helper which will then call 
test_plane_panning function. This way you would get away from making 
such big change which in the end seems to be about changing indentation.

> +		bool mode_found = false;
> +		uint64_t mem_size = 0;
> +		igt_output_t *output = data->output;
> +		igt_crc_t ref_crc;
>   
> -	test_init(data, pipe);
> +		igt_info("Using (pipe %s + %s) to run the subtest.\n",
> +			 kmstest_pipe_name(pipe), igt_output_name(output));
>   
> -	if (is_i915_device(data->drm_fd)) {
> -		for_each_memory_region(r, data->drm_fd)
> -			if (r->ci.memory_class == I915_MEMORY_CLASS_DEVICE)
> -				mem_size = r->cpu_size;
> +		test_init(data, pipe);
>   
> -	}
> +		if (is_i915_device(data->drm_fd)) {
> +			for_each_memory_region(r, data->drm_fd)
> +				if (r->ci.memory_class == I915_MEMORY_CLASS_DEVICE)
> +					mem_size = r->cpu_size;
> +		}
>   
> -	if (is_xe_device(data->drm_fd)) {
> -		struct drm_xe_mem_region *memregion;
> -		uint64_t memreg = all_memory_regions(data->drm_fd), region;
> +		if (is_xe_device(data->drm_fd)) {
> +			struct drm_xe_mem_region *memregion;
> +			uint64_t memreg = all_memory_regions(data->drm_fd), region;
>   
> -		xe_for_each_mem_region(data->drm_fd, memreg, region) {
> -			memregion = xe_mem_region(data->drm_fd, region);
> +			xe_for_each_mem_region(data->drm_fd, memreg, region) {
> +				memregion = xe_mem_region(data->drm_fd, region);
>   
> -			if (XE_IS_CLASS_VRAM(memregion))
> -				mem_size = memregion->total_size;
> +				if (XE_IS_CLASS_VRAM(memregion))
> +					mem_size = memregion->total_size;
> +			}
>   		}
> -	}
>   
> -	for_each_connector_mode(output) {
> -		drmModeModeInfo *m = &output->config.connector->modes[j__];
> -		uint32_t fb_size = m->hdisplay * m->vdisplay * 4;
> +		for_each_connector_mode(output) {
> +			drmModeModeInfo *m = &output->config.connector->modes[j__];
> +			uint32_t fb_size = m->hdisplay * m->vdisplay * 4;
>   
> -		/* test allocates 2 double-dim fbs, add one more, to be safe */
> -		if (mem_size && 3 * 4 * fb_size > mem_size) {
> -			igt_debug("Skipping mode %s due to low memory\n", m->name);
> -			continue;
> -		}
> +			/* test allocates 2 double-dim fbs, add one more, to be safe */
> +			if (mem_size && 3 * 4 * fb_size > mem_size) {
> +				igt_debug("Skipping mode %s due to low memory\n", m->name);
> +				continue;
> +			}
>   
> -		igt_output_override_mode(output, m);
> -		mode_found = true;
> -		break;
> -	}
> -	igt_require(mode_found);
> +			igt_output_override_mode(output, m);
> +			mode_found = true;
> +			break;
> +		}
> +		igt_require(mode_found);
>   
> -	if (data->flags & TEST_PANNING_TOP_LEFT)
> -		test_grab_crc(data, output, pipe, &red, data->flags, &ref_crc);
> -	else
> -		test_grab_crc(data, output, pipe, &blue, data->flags, &ref_crc);
> +		if (data->flags & TEST_PANNING_TOP_LEFT)
> +			test_grab_crc(data, output, pipe, &red, data->flags, &ref_crc);
> +		else
> +			test_grab_crc(data, output, pipe, &blue, data->flags, &ref_crc);
>   
> -	test_plane_panning_with_output(data, pipe, output, &ref_crc, data->flags);
> +		test_plane_panning_with_output(data, pipe, output, &ref_crc, data->flags);
>   
> -	test_fini(data);
> +		test_fini(data);
> +	}
>   }
>   
>   static const color_t colors_extended[] = {
> @@ -889,7 +890,7 @@ static bool test_format_plane_rgb(data_t *data, enum pipe pipe,
>   				  igt_crc_t ref_crc[],
>   				  struct igt_fb *fb)
>   {
> -	igt_info("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
> +	igt_debug("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
>   		 IGT_FORMAT_ARGS(format), IGT_MODIFIER_ARGS(modifier),
>   		 kmstest_pipe_name(pipe), plane->index);
>   
> @@ -927,7 +928,7 @@ static bool test_format_plane_yuv(data_t *data, enum pipe pipe,
>   						     igt_color_range_to_str(r)))
>   				continue;
>   
> -			igt_info("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " (%s, %s) on %s.%u\n",
> +			igt_debug("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " (%s, %s) on %s.%u\n",
>   				 IGT_FORMAT_ARGS(format), IGT_MODIFIER_ARGS(modifier),
>   				 igt_color_encoding_to_str(e),
>   				 igt_color_range_to_str(r),

Where you change igt_info to igt_debug, these following lines 
indentation also need to match to moved parenthesis.

> @@ -1043,7 +1044,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>   
>   	igt_pipe_crc_start(data->pipe_crc);
>   
> -	igt_info("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
> +	igt_debug("Testing format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
>   		 IGT_FORMAT_ARGS(ref.format), IGT_MODIFIER_ARGS(ref.modifier),
>   		 kmstest_pipe_name(pipe), plane->index);
>   
> @@ -1089,7 +1090,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>   			};
>   
>   			if (igt_vec_index(&tested_formats, &rf) >= 0) {
> -				igt_info("Skipping format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
> +				igt_debug("Skipping format " IGT_FORMAT_FMT " / modifier " IGT_MODIFIER_FMT " on %s.%u\n",
>   					 IGT_FORMAT_ARGS(f.format), IGT_MODIFIER_ARGS(f.modifier),
>   					 kmstest_pipe_name(pipe), plane->index);
>   				continue;
> @@ -1211,7 +1212,8 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>   	for_each_plane_on_pipe(&data->display, pipe, plane) {
>   		if (skip_plane(data, plane))
>   			continue;
> -		result &= test_format_plane(data, pipe, output, plane, &primary_fb);
> +		igt_dynamic_f("pipe-%s-plane-%u", kmstest_pipe_name(pipe), plane->index)
> +			result &= test_format_plane(data, pipe, output, plane, &primary_fb);
>   	}
>   
>   	test_fini(data);
> @@ -1317,8 +1319,7 @@ static void run_test(data_t *data, void (*test)(data_t *, enum pipe))
>   			continue;
>   
>   		igt_output_set_pipe(data->output, PIPE_NONE);
> -		igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe))
> -			test(data, pipe);
> +		test(data, pipe);
>   
>   		if (is_pipe_limit_reached(++count))
>   			break;