Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_plane: Pipeline crc capture and flips

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 21, 2020 at 08:26:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Eliminate some dead time used to wait for crcs by pipelining
> the flips and crc capture.
> 
> On KBL:
> time kms_plane --r pixel-format-pipe-A-planes
> - real	0m22,592s
> + real	0m14,527s
> 
> time kms_plane --r pixel-format-pipe-A-planes --extended
> - real	1m8,090s
> + real	0m38,738s
> 
> To keep the code somewhat sane we only pipeline while
> capturing the colors for a single pixel format. That does
> still leave a few non-pipelined frames when switching from
> one pixel format to another. Which explains why the extended
> test gets closer to the -50% mark as it tests more colors and
> thus has a larger proportion of optimally pipelined flips.
> 
> Not sure how horrible the code would become if we tried to
> maintain the pipelining throughout the test. For now let's
> just accept the small victory and move on.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tests/kms_plane.c | 70 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 9ef3a7f3c1b0..9ad366824c0d 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -500,8 +500,9 @@ static void test_format_plane_color(data_t *data, enum pipe pipe,
>  		igt_plane_set_size(plane, width, height);
>  	}
>  
> +	if (crc)
> +		igt_pipe_crc_get_previous(data->drm_fd, data->pipe_crc, crc);

Actually need to do this before painting the new fb as that can be
slow. If it takes more than a frame then we'd read the crc for frame N-1
instead of N-2 as we intended. But that's still a bit fragile perhaps.
I guess what I really need to do is sample the frame counter when I do
the flip so that I know exactly which crc I need.

>  	igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_UNIVERSAL);
> -	igt_pipe_crc_get_current(data->display.drm_fd, data->pipe_crc, crc);
>  
>  	igt_remove_fb(data->drm_fd, &old_fb);
>  }
> @@ -525,6 +526,42 @@ static int num_unique_crcs(const igt_crc_t crc[], int num_crc)
>  	return num_unique_crc;
>  }
>  
> +static void capture_format_crcs(data_t *data, enum pipe pipe,
> +				igt_plane_t *plane,
> +				uint32_t format, uint64_t modifier,
> +				int width, int height,
> +				enum igt_color_encoding encoding,
> +				enum igt_color_range range,
> +				igt_crc_t crc[], struct igt_fb *fb)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->num_colors; i++) {
> +		const color_t *c = &data->colors[i];
> +
> +		/*
> +		 * The flip issued during frame N will latch
> +		 * at the start of frame N+1, and its CRC will
> +		 * be ready at the start of frame N+2. So the
> +		 * CRC captured here before the flip is issued
> +		 * is for frame N-2.
> +		 */
> +		test_format_plane_color(data, pipe, plane, format, modifier,
> +					width, height, encoding, range, c,
> +					i >= 2 ? &crc[i - 2] : NULL, fb);
> +	}
> +
> +	/*
> +	 * Get the remaining two crcs
> +	 *
> +	 * TODO: avoid the extra wait by maintaining the pipeline
> +	 * between different pixel formats as well? Could get messy.
> +	 */
> +	igt_pipe_crc_get_previous(data->drm_fd, data->pipe_crc, &crc[i - 2]);
> +	i++;
> +	igt_pipe_crc_get_current(data->drm_fd, data->pipe_crc, &crc[i - 2]);
> +}
> +
>  static bool test_format_plane_colors(data_t *data, enum pipe pipe,
>  				     igt_plane_t *plane,
>  				     uint32_t format, uint64_t modifier,
> @@ -534,21 +571,17 @@ static bool test_format_plane_colors(data_t *data, enum pipe pipe,
>  				     igt_crc_t ref_crc[],
>  				     struct igt_fb *fb)
>  {
> -	int crc_mismatch_count = 0;
> +	igt_crc_t crc[ARRAY_SIZE(colors_extended)];
>  	unsigned int crc_mismatch_mask = 0;
> +	int crc_mismatch_count = 0;
>  	bool result = true;
> +	int i;
>  
> -	for (int i = 0; i < data->num_colors; i++) {
> -		const color_t *c = &data->colors[i];
> -		igt_crc_t crc;
> -
> -		test_format_plane_color(data, pipe, plane,
> -					format, modifier,
> -					width, height,
> -					encoding, range,
> -					c, &crc, fb);
> +	capture_format_crcs(data, pipe, plane, format, modifier,
> +			    width, height, encoding, range, crc, fb);
>  
> -		if (!igt_check_crc_equal(&crc, &ref_crc[i])) {
> +	for (i = 0; i < data->num_colors; i++) {
> +		if (!igt_check_crc_equal(&crc[i], &ref_crc[i])) {
>  			crc_mismatch_count++;
>  			crc_mismatch_mask |= (1 << i);
>  			result = false;
> @@ -700,16 +733,9 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>  		igt_remove_fb(data->drm_fd, &test_fb);
>  	}
>  
> -	for (int i = 0; i < data->num_colors; i++) {
> -		const color_t *c = &data->colors[i];
> -
> -		test_format_plane_color(data, pipe, plane,
> -					format, modifier,
> -					width, height,
> -					IGT_COLOR_YCBCR_BT709,
> -					IGT_COLOR_YCBCR_LIMITED_RANGE,
> -					c, &ref_crc[i], &fb);
> -	}
> +	capture_format_crcs(data, pipe, plane, format, modifier,
> +			    width, height, IGT_COLOR_YCBCR_BT709,
> +			    IGT_COLOR_YCBCR_LIMITED_RANGE, ref_crc, &fb);
>  
>  	/*
>  	 * Make sure we have some difference between the colors. This
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev