Re: [igt-dev] [PATCH 2/2] [i-g-t] tests/kms_cursor_crc: Fix test intermittent failures on AMD gpu

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

Hi Hersen,

On 1.10.2023 19.28, Hersen Wu wrote:
> Wait for two more vblanks before reading crc on AMD gpu.
> 
> Without waiting for two vblanks, AMD cursor updates may not
> synchronized to the same frame of pipe, crc generated may
> not be reliable.
> 
> Signed-off-by: Hersen Wu <hersenxs.wu@amd.com>
> ---
>   tests/kms_cursor_crc.c | 24 +++++++++++++++++++++++-
>   1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index ba29ff65d..7450ded9d 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -260,6 +260,15 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
>   							      SUSPEND_TEST_NONE);
>   
>   			igt_pipe_crc_start(pipe_crc);
> +
> +			/* Wait for two more vblanks since cursor updates may not
> +			 * synchronized to the same frame on AMD HW
> +			 */
> +			if (is_amdgpu_device(data->drm_fd))
> +				igt_wait_for_vblank_count(data->drm_fd,
> +					display->pipes[data->pipe].crtc_offset,
> +					data->vblank_wait_count);
> +

Here was not done cursor modifications, test is just coming back from 
suspend or done dpms off/on. If this failed here it would mean there 
probably really was something to look into on driver side. From what 
your commit message says maybe you'll need to look few lines above where 
the crc is taken before suspend/dpms.

>   			igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc_after);
>   			igt_assert_crc_equal(hwcrc, &crc_after);
>   		}
> @@ -276,6 +285,15 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
>   		restore_image(data, swbufidx, &((cursorarea){x, y, data->curw, data->curh}));
>   		igt_plane_set_fb(data->primary, &data->primary_fb[swbufidx]);
>   		igt_display_commit(display);
> +
> +		/* Wait for two more vblanks since cursor updates may not
> +		 * synchronized to the same frame on AMD HW
> +		 */
> +		if (is_amdgpu_device(data->drm_fd))
> +			igt_wait_for_vblank_count(data->drm_fd,
> +				display->pipes[data->pipe].crtc_offset,
> +				data->vblank_wait_count);
> +

Here is normal flip for getting sw generated reference crc, nothing 
about cursor here.

>   		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>   		igt_assert_crc_equal(&crc, hwcrc);
>   	}
> @@ -1079,7 +1097,11 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
>   
>   		igt_require_pipe_crc(data.drm_fd);
>   
> -		data.vblank_wait_count = is_msm_device(data.drm_fd) ? 2 : 1;
> +		/* Wait for two more vblanks since cursor updates may not
> +		 * synchronized to the same frame on AMD HW
> +		 */
> +		data.vblank_wait_count =
> +			(is_msm_device(data.drm_fd) || is_amdgpu_device(data.drm_fd)) ? 2 : 1;

Reflecting your commit message this change should be all that is needed.

>   	}
>   
>   	data.cursor_max_w = cursor_width;