Re: [PATCH i-g-t 1/2] tests/kms_async_flips: subtests to validate async flips on atomic path

[Karthik B S <karthik.b.s@intel.com>]

On 8/27/2024 11:41 AM, Santhosh Reddy Guddati wrote:
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> ---
>   tests/kms_async_flips.c | 154 ++++++++++++++++++++++++++++++++++++----
>   1 file changed, 141 insertions(+), 13 deletions(-)
>
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index 5b70c4033..21cf0d302 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -66,6 +66,25 @@
>    *
>    * SUBTEST: invalid-async-flip
>    * Description: Negative case to verify if changes in fb are rejected from kernel as expected
> + *
> + * SUBTEST: alternate-sync-async-flip-atomic
> + * Description: Verify the async flip functionality and the fps during async flips using atomic path
> + *
> + * SUBTEST: async-flip-with-page-flip-events-atomic
> + * Description: Wait for page flip events in between successive asynchronous flips using atomic path
> + *
> + * SUBTEST: test-time-stamp-atomic
> + * Description: Verify that the async flip timestamp does not coincide with either previous or next vblank
> + * 				when async flip is done using atomic path
> + *
> + * SUBTEST: test-cursor-atomic
> + * Description: Verify that the DRM_IOCTL_MODE_CURSOR passes after async flip with atomic commit
> + *
> + * SUBTEST: invalid-async-flip-atomic
> + * Description: Negative case to verify if changes in fb are rejected from kernel as expected when async flip is done using atomic path
> + *
> + * SUBTEST: crc-atomic
> + * Description: Use CRC to verify async flip scans out the correct framebuffer with atomic commit
>    */
>   
>   #define CURSOR_POS 128
> @@ -101,6 +120,7 @@ typedef struct {
>   	bool alternate_sync_async;
>   	bool allow_fail;
>   	struct buf_ops *bops;
> +	bool atomic_path;
>   } data_t;
>   
>   static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
> @@ -271,9 +291,14 @@ static void test_async_flip(data_t *data)
>   		if (data->alternate_sync_async) {
>   			flags &= ~DRM_MODE_PAGE_FLIP_ASYNC;
>   
> -			ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> +			if (!data->atomic_path) {
> +				ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>   					      data->bufs[frame % NUM_FBS].fb_id,
>   					      flags, data);
> +			} else {
> +				igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
> +				ret = igt_display_try_commit_atomic(&data->display, flags, data);
> +			}

Hi Santhosh,

Could we have this piece of code in a separate function as it is called 
in multiple places?

>   
>   			igt_assert_eq(ret, 0);
>   
> @@ -282,19 +307,30 @@ static void test_async_flip(data_t *data)
>   			flags |= DRM_MODE_PAGE_FLIP_ASYNC;
>   
>   			if (async_flip_needs_extra_frame(data)) {
> -				ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> +				if (!data->atomic_path) {
> +					ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>   						      data->bufs[frame % NUM_FBS].fb_id,
>   						      flags, data);
> -
> +				} else {
> +					igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
> +					ret = igt_display_try_commit_atomic(&data->display, flags, data);
> +				}
>   				igt_assert_eq(ret, 0);
>   
>   				wait_flip_event(data);
>   			}
>   		}
>   
> -		ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> +		if (!data->atomic_path) {
> +			ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>   				      data->bufs[frame % NUM_FBS].fb_id,
>   				      flags, data);
> +		} else {
> +			igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
> +			ret = igt_display_try_commit_atomic(&data->display, flags, data);
> +			igt_skip_on(ret == -EINVAL);
> +		}
> +
>   		if (frame == 1 && data->allow_fail)
>   			igt_skip_on(ret == -EINVAL);
>   		else
> @@ -419,9 +455,14 @@ static void test_cursor(data_t *data)
>   
>   	do_ioctl(data->drm_fd, DRM_IOCTL_MODE_CURSOR, &cur);
>   
> -	ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> -			      data->bufs[0].fb_id,
> -			      flags, data);
> +	if (!data->atomic_path) {
> +		ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> +				      data->bufs[0].fb_id,
> +				      flags, data);
> +	} else {
> +		igt_plane_set_fb(data->plane, &data->bufs[0]);
> +		ret = igt_display_try_commit_atomic(&data->display, flags, data);
> +	}
>   
>   	igt_assert_eq(ret, 0);
>   
> @@ -457,14 +498,24 @@ static void test_invalid(data_t *data)
>   	igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>   
>   	/* first async flip is expected to allow modifier changes */
> -	ret = drmModePageFlip(data->drm_fd, data->crtc_id, fb[1].fb_id,
> -			      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +	if(!data->atomic_path) {
> +		ret = drmModePageFlip(data->drm_fd, data->crtc_id, fb[1].fb_id,
> +				      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +	} else {
> +		igt_plane_set_fb(data->plane, &fb[1]);
> +		ret = igt_display_try_commit_atomic(&data->display, DRM_MODE_PAGE_FLIP_ASYNC, data);
> +	}
>   	igt_assert_eq(ret, 0);
>   	wait_flip_event(data);
>   
>   	/* subsequent async flips should reject modifier changes */
> -	ret = drmModePageFlip(data->drm_fd, data->crtc_id, fb[0].fb_id,
> -			      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +	if(!data->atomic_path) {
> +		ret = drmModePageFlip(data->drm_fd, data->crtc_id, fb[0].fb_id,
> +				      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +	} else {
> +		igt_plane_set_fb(data->plane, &fb[0]);
> +		ret = igt_display_try_commit_atomic(&data->display, DRM_MODE_PAGE_FLIP_ASYNC, data);
> +	}
>   	igt_assert(ret == -EINVAL);
>   
>   	/* TODO: Add verification for changes in stride, pixel format */
> @@ -611,8 +662,26 @@ static void test_crc(data_t *data)
>   		paint_fb(data, &data->bufs[frame], 1, height, 0xff0000ff);
>   
>   		data->flip_pending = true;
> -		ret = drmModePageFlip(data->drm_fd, data->crtc_id, data->bufs[frame].fb_id,
> -				      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +
> +		if (!data->atomic_path) {
> +			ret = drmModePageFlip(data->drm_fd, data->crtc_id, data->bufs[frame].fb_id,
> +					      DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +		} else {
> +			 // TODO: Check if the first frame modeset is required.
> +			 // Atomic commit is failing with invalid parameter when async flip with atomic is done
> +			 // for crc test. So, skipping the first frame modeset.
> +			if (data->frame_count == 0) {
Could we wove this first commit outside of the while loop, as we only 
need this once and doesn't need to be checked everytime.
> +				igt_plane_set_fb(data->plane, &data->bufs[frame]);
> +				igt_display_commit_atomic(&data->display, DRM_MODE_ATOMIC_ALLOW_MODESET , data);
> +				data->frame_count++;
> +				continue;
> +			} else {
> +				igt_plane_set_fb(data->plane, &data->bufs[frame]);
> +				ret = igt_display_try_commit_atomic(&data->display,
> +						DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> +			}
> +		}
> +
>   		igt_assert_eq(ret, 0);
>   
>   		wait_events_crc(data);
> @@ -705,15 +774,38 @@ igt_main
>   				run_test(&data, test_async_flip);
>   		}
>   
> +		igt_describe("Wait for page flip events in between successive asynchronous flips using atomic path");
> +		igt_subtest_with_dynamic("async-flip-with-page-flip-events-atomic") {
> +			data.alternate_sync_async = false;
> +			data.atomic_path = true;
> +			if (is_intel_device(data.drm_fd))
> +				run_test_with_modifiers(&data, test_async_flip);
> +			else
> +				run_test(&data, test_async_flip);
> +		}
> +
>   		igt_describe("Alternate between sync and async flips");
>   		igt_subtest_with_dynamic("alternate-sync-async-flip") {
>   			data.alternate_sync_async = true;
>   			run_test(&data, test_async_flip);
>   		}
>   
> +		igt_describe("Alternate between sync and async flips using atomic path");
> +		igt_subtest_with_dynamic("alternate-sync-async-flip-atomic") {
> +			data.alternate_sync_async = true;
> +			data.atomic_path = true;

Please check if the atomic_path variable needs to be reset for legacy tests?

Thanks,
Karthik.B.S
> +			run_test(&data, test_async_flip);
> +		}
> +
>   		igt_describe("Verify that the async flip timestamp does not coincide with either previous or next vblank");
>   		igt_subtest_with_dynamic("test-time-stamp")
>   			run_test(&data, test_timestamp);
> +
> +		igt_describe("Verify that the async flip timestamp does not coincide with either previous or next vblank with atomic path");
> +		igt_subtest_with_dynamic("test-time-stamp-atomic") {
> +			data.atomic_path = true;
> +			run_test(&data, test_timestamp);
> +		}
>   	}
>   
>   	igt_describe("Verify that the DRM_IOCTL_MODE_CURSOR passes after async flip");
> @@ -730,6 +822,20 @@ igt_main
>   		run_test(&data, test_cursor);
>   	}
>   
> +	igt_describe("Verify that the DRM_IOCTL_MODE_CURSOR passes after async flip with atomic commit");
> +	igt_subtest_with_dynamic("test-cursor-atomic") {
> +		/*
> +		 * Intel's PSR2 selective fetch adds other planes to state when
> +		 * necessary, causing the async flip to fail because async flip is not
> +		 * supported in cursor plane.
> +		 */
> +		igt_skip_on_f(i915_psr2_selective_fetch_check(data.drm_fd, NULL),
> +			      "PSR2 sel fetch causes cursor to be added to primary plane " \
> +			      "pages flips and async flip is not supported in cursor\n");
> +		data.atomic_path = true;
> +		run_test(&data, test_cursor);
> +	}
> +
>   	igt_describe("Negative case to verify if changes in fb are rejected from kernel as expected");
>   	igt_subtest_with_dynamic("invalid-async-flip") {
>   		/* TODO: support more vendors */
> @@ -742,6 +848,19 @@ igt_main
>   		run_test(&data, test_invalid);
>   	}
>   
> +	igt_describe("Negative case to verify if changes in fb are rejected from kernel as expected when async flip is done using atomic path");
> +	igt_subtest_with_dynamic("invalid-async-flip-atomic") {
> +		data.atomic_path = true;
> +		/* TODO: support more vendors */
> +		igt_require(is_intel_device(data.drm_fd));
> +		igt_require(igt_display_has_format_mod(&data.display, DRM_FORMAT_XRGB8888,
> +						       I915_FORMAT_MOD_X_TILED));
> +		igt_require(igt_display_has_format_mod(&data.display, DRM_FORMAT_XRGB8888,
> +						       I915_FORMAT_MOD_Y_TILED));
> +
> +		run_test(&data, test_invalid);
> +	}
> +
>   	igt_describe("Use CRC to verify async flip scans out the correct framebuffer");
>   	igt_subtest_with_dynamic("crc") {
>   		/* Devices without CRC can't run this test */
> @@ -750,6 +869,15 @@ igt_main
>   		run_test(&data, test_crc);
>   	}
>   
> +	igt_describe("Use CRC to verify async flip scans out the correct framebuffer with atomic commit");
> +	igt_subtest_with_dynamic("crc-atomic") {
> +		/* Devices without CRC can't run this test */
> +		igt_require_pipe_crc(data.drm_fd);
> +
> +		data.atomic_path = true;
> +		run_test(&data, test_crc);
> +	}
> +
>   	igt_fixture {
>   		for (i = 0; i < NUM_FBS; i++)
>   			igt_remove_fb(data.drm_fd, &data.bufs[i]);