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

[Kahola, Mika]

> -----Original Message-----
> From: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> Sent: Thursday, October 29, 2020 8:23 PM
> To: Kahola, Mika <mika.kahola at intel.com>; igt-dev at lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-
> clamping) tests
> 
> On 29.10.2020 14.43, Kahola, Mika wrote:
> >> -----Original Message-----
> >> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> >> Juha- Pekka Heikkila
> >> Sent: Monday, October 26, 2020 3:08 PM
> >> To: igt-dev at 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 at 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?
> 
> You mean there would come more than one modeset? I think many tests will
> start to fail at that moment. Here fb which required modeset is left on screen
> when going for restart, there shouldn't be new modeset. Then if modeset
> itself fail that modesetting commit is not try commit so it will assert on
> failure. I don't think there is need for additional counter unless cdclk
> calculations fail but then everything is failing everywhere anyway I think.

That's what I was thinking that there might be a minor likelihood that more than one modeset would occur. If that happens, something else must have gone wrong.

Anyway, the patch looks ok.

Reviewed-by: Mika Kahola <mika.kahola at intel.com>
> 
> >
> > 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 at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/igt-dev