Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-clamping) tests

["Kahola, Mika" <mika.kahola@intel.com>]

> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Juha-
> Pekka Heikkila
> Sent: Monday, October 26, 2020 3:08 PM
> To: igt-dev@lists.freedesktop.org
> Subject: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-clamping)
> tests
> 
> cdclk caused modesets mid test will mess up crc sequence where two
> consecutive frames will get same crc. If there come modeset restart round
> for given set as modeset will anyway happen only on first color.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  tests/kms_plane.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c index
> e42c71cd9..4ce859fba 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -560,9 +560,11 @@ static void capture_format_crcs(data_t *data, enum
> pipe pipe,
>  	struct drm_event_vblank ev;
>  	int i;
> 
> +restart_round:
>  	for (i = 0; i < data->num_colors; i++) {
>  		const color_t *c = &data->colors[i];
>  		struct igt_fb old_fb = *fb;
> +		int ret;
> 
>  		prepare_format_color(data, pipe, plane, format, modifier,
>  				     width, height, encoding, range, c, fb); @@
> -593,10 +595,22 @@ static void capture_format_crcs(data_t *data, enum
> pipe pipe,
>  			 * to be prepared in parallel while the current fb
>  			 * awaits to be latched.
>  			 */
> -			igt_display_commit_atomic(&data->display,
> -
> DRM_MODE_ATOMIC_ALLOW_MODESET |
> -
> DRM_MODE_ATOMIC_NONBLOCK |
> -
> DRM_MODE_PAGE_FLIP_EVENT, NULL);
> +			ret = igt_display_try_commit_atomic(&data->display,
> +
> DRM_MODE_ATOMIC_NONBLOCK |
> +
> DRM_MODE_PAGE_FLIP_EVENT, NULL);
> +			if (ret) {
> +				/*
> +				 * there was needed modeset for pixel
> format.
> +				 * modeset here happen only on first color of
> +				 * given set so restart round as modeset will
> +				 * mess up crc frame sequence.
> +				 */
> +				igt_display_commit_atomic(&data->display,
> +
> DRM_MODE_ATOMIC_ALLOW_MODESET,
> +							  NULL);
> +				igt_remove_fb(data->drm_fd, &old_fb);
> +				goto restart_round;
> +			}

I tried to figure out the patch and it seems that we need to do modeset only once, right? However, there is a theoretical chance (probability maybe next to nothing) that we keep looping on restart_found. Therefore, I was thinking that we should limit the restarts to 1 or to some finite number?

Cheers,
Mika
>  		} else {
>  			/*
>  			 * Last moment to grab the previous crc @@ -815,8
> +829,10 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> 
>  		igt_plane_set_fb(plane, &test_fb);
> 
> -		ret = igt_display_try_commit_atomic(&data->display,
> DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> -
> +		ret = igt_display_try_commit_atomic(&data->display,
> +
> DRM_MODE_ATOMIC_TEST_ONLY |
> +
> DRM_MODE_ATOMIC_ALLOW_MODESET,
> +						    NULL);
>  		if (!ret) {
>  			width = test_fb.width;
>  			height = test_fb.height;
> --
> 2.28.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev