Re: [igt-dev] [PATCH i-g-t v2 11/13] igt: tests: chamelium: Start to unify tests

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> The various tests in kms_chamelium are really variants of one another
> but share little code.
> 
> In addition to the duplication, this gets in the way of the
> introduction of more tests, or to be able to run all the tests on all
> the output, which isn't the case at the moment, with the HDMI and DP
> tests and the VGA tests being different.
> 
> Start by introducing a check parameter to the do_test_display
> function, that will tell which test method we want to use to compare
> the frames.

I think we can go even further and unify the assert_foo_or_dump
functions, which would imply moving enum chamelium_check to
igt_chamelium. It's probably best to do it in a separate patch though
and I might have a go at it in the next revision of my plane testing
series.

What do you think?

> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

> ---
>  tests/kms_chamelium.c | 134 +++++++++++++++++++++++-------------------
>  1 file changed, 73 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 2d848c2f0620..8230cb852cfb 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -527,12 +527,15 @@ static int chamelium_get_pattern_fb(data_t *data, size_t width, size_t height,
>  	return fb_id;
>  }
>  
> -static void do_test_display_crc(data_t *data, struct chamelium_port *port,
> -				igt_output_t *output, drmModeModeInfo *mode,
> -				uint32_t fourcc, int count)
> +enum chamelium_check {
> +	CHAMELIUM_CHECK_CRC,
> +};
> +
> +static void do_test_display(data_t *data, struct chamelium_port *port,
> +			    igt_output_t *output, drmModeModeInfo *mode,
> +			    uint32_t fourcc, enum chamelium_check check,
> +			    int count)
>  {
> -	igt_crc_t *crc;
> -	igt_crc_t *expected_crc;
>  	struct chamelium_fb_crc_async_data *fb_crc;
>  	struct igt_fb frame_fb, fb;
>  	int i, fb_id, captured_frame_count;
> @@ -545,39 +548,46 @@ static void do_test_display_crc(data_t *data, struct chamelium_port *port,
>  	frame_id = igt_fb_convert(&frame_fb, &fb, fourcc);
>  	igt_assert(frame_id > 0);
>  
> -	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> -							&fb);
> +	if (check == CHAMELIUM_CHECK_CRC)
> +		fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> +								&fb);
>  
>  	enable_output(data, port, output, mode, &frame_fb);
>  
> -	/* We want to keep the display running for a little bit, since
> -	 * there's always the potential the driver isn't able to keep
> -	 * the display running properly for very long
> -	 */
> -	chamelium_capture(data->chamelium, port, 0, 0, 0, 0, count);
> -	crc = chamelium_read_captured_crcs(data->chamelium,
> -					   &captured_frame_count);
> +	if (check == CHAMELIUM_CHECK_CRC) {
> +		igt_crc_t *expected_crc;
> +		igt_crc_t *crc;
>  
> -	igt_assert(captured_frame_count == count);
> +		/* We want to keep the display running for a little bit, since
> +		 * there's always the potential the driver isn't able to keep
> +		 * the display running properly for very long
> +		 */
> +		chamelium_capture(data->chamelium, port, 0, 0, 0, 0, count);
> +		crc = chamelium_read_captured_crcs(data->chamelium,
> +						   &captured_frame_count);
>  
> -	igt_debug("Captured %d frames\n", captured_frame_count);
> +		igt_assert(captured_frame_count == count);
>  
> -	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
> +		igt_debug("Captured %d frames\n", captured_frame_count);
>  
> -	for (i = 0; i < captured_frame_count; i++)
> -		chamelium_assert_crc_eq_or_dump(data->chamelium,
> -						expected_crc, &crc[i],
> -						&fb, i);
> +		expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
>  
> -	free(expected_crc);
> -	free(crc);
> +		for (i = 0; i < captured_frame_count; i++)
> +			chamelium_assert_crc_eq_or_dump(data->chamelium,
> +							expected_crc, &crc[i],
> +							&fb, i);
> +
> +		free(expected_crc);
> +		free(crc);
> +	}
>  
>  	igt_remove_fb(data->drm_fd, &frame_fb);
>  	igt_remove_fb(data->drm_fd, &fb);
>  }
>  
> -static void test_display_crc_one_mode(data_t *data, struct chamelium_port *port,
> -				      uint32_t fourcc, int count)
> +static void test_display_one_mode(data_t *data, struct chamelium_port *port,
> +				  uint32_t fourcc, enum chamelium_check check,
> +				  int count)
>  {
>  	igt_output_t *output;
>  	drmModeConnector *connector;
> @@ -590,13 +600,15 @@ static void test_display_crc_one_mode(data_t *data, struct chamelium_port *port,
>  	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>  	igt_assert(primary);
>  
> -	do_test_display_crc(data, port, output, &connector->modes[0], fourcc, count);
> +	do_test_display(data, port, output, &connector->modes[0], fourcc,
> +			check, count);
>  
>  	drmModeFreeConnector(connector);
>  }
>  
> -static void test_display_crc_all_modes(data_t *data, struct chamelium_port *port,
> -				       uint32_t fourcc, int count)
> +static void test_display_all_modes(data_t *data, struct chamelium_port *port,
> +				   uint32_t fourcc, enum chamelium_check check,
> +				   int count)
>  {
>  	igt_output_t *output;
>  	igt_plane_t *primary;
> @@ -613,7 +625,7 @@ static void test_display_crc_all_modes(data_t *data, struct chamelium_port *port
>  	for (i = 0; i < connector->count_modes; i++) {
>  		drmModeModeInfo *mode = &connector->modes[i];
>  
> -		do_test_display_crc(data, port, output, mode, fourcc, count);
> +		do_test_display(data, port, output, mode, fourcc, check, count);
>  	}
>  
>  	drmModeFreeConnector(connector);
> @@ -874,16 +886,16 @@ igt_main
>  							edid_id, alt_edid_id);
>  
>  		connector_subtest("dp-crc-single", DisplayPort)
> -			test_display_crc_all_modes(&data, port,
> -						   DRM_FORMAT_XRGB8888, 1);
> +			test_display_all_modes(&data, port, DRM_FORMAT_XRGB8888,
> +					       CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("dp-crc-fast", DisplayPort)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_XRGB8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_XRGB8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("dp-crc-multiple", DisplayPort)
> -			test_display_crc_all_modes(&data, port,
> -						   DRM_FORMAT_XRGB8888, 3);
> +			test_display_all_modes(&data, port, DRM_FORMAT_XRGB8888,
> +					       CHAMELIUM_CHECK_CRC, 3);
>  
>  		connector_subtest("dp-frame-dump", DisplayPort)
>  			test_display_frame_dump(&data, port);
> @@ -941,56 +953,56 @@ igt_main
>  							edid_id, alt_edid_id);
>  
>  		connector_subtest("hdmi-crc-single", HDMIA)
> -			test_display_crc_all_modes(&data, port,
> -						   DRM_FORMAT_XRGB8888, 1);
> +			test_display_all_modes(&data, port, DRM_FORMAT_XRGB8888,
> +					       CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-fast", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_XRGB8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_XRGB8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-multiple", HDMIA)
> -			test_display_crc_all_modes(&data, port,
> -						   DRM_FORMAT_XRGB8888, 3);
> +			test_display_all_modes(&data, port, DRM_FORMAT_XRGB8888,
> +					       CHAMELIUM_CHECK_CRC, 3);
>  
>  		connector_subtest("hdmi-crc-argb8888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_ARGB8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_ARGB8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-abgr8888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_ABGR8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_ABGR8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-xrgb8888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_XRGB8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_XRGB8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-xbgr8888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_XBGR8888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_XBGR8888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-rgb888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_RGB888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_RGB888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-bgr888", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_BGR888, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_BGR888,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-rgb565", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_RGB565, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_RGB565,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-bgr565", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_BGR565, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_BGR565,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-argb1555", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_ARGB1555, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_ARGB1555,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-crc-xrgb1555", HDMIA)
> -			test_display_crc_one_mode(&data, port,
> -						  DRM_FORMAT_XRGB1555, 1);
> +			test_display_one_mode(&data, port, DRM_FORMAT_XRGB1555,
> +					      CHAMELIUM_CHECK_CRC, 1);
>  
>  		connector_subtest("hdmi-frame-dump", HDMIA)
>  			test_display_frame_dump(&data, port);
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev