[PATCH v5 4/4] tests/kms_async_flips: Create subtest for overlay planes
Melissa Wen
mwen at igalia.com
Thu Apr 3 15:34:48 UTC 2025
On 04/01, André Almeida wrote:
> amdgpu can perform async flips in overlay planes as well, so create a
> test for that.
>
> Signed-off-by: André Almeida <andrealmeid at igalia.com>
> ---
> tests/kms_async_flips.c | 73 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index 6986278d7..98000fc92 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -85,6 +85,9 @@
> *
> * SUBTEST: async-flip-suspend-resume
> * Description: Verify the async flip functionality with suspend and resume cycle
> + *
> + * SUBTEST: overlay-atomic
> + * Description: Verify overlay planes with async flips in atomic API
> */
>
> #define CURSOR_POS 128
> @@ -105,12 +108,14 @@ typedef struct {
> uint32_t crtc_id;
> uint32_t refresh_rate;
> struct igt_fb bufs[NUM_FBS];
> + struct igt_fb bufs_overlay[NUM_FBS];
> igt_display_t display;
> igt_output_t *output;
> unsigned long flip_timestamp_us;
> double flip_interval;
> uint64_t modifier;
> igt_plane_t *plane;
> + igt_plane_t *plane_overlay;
> igt_pipe_crc_t *pipe_crc;
> igt_crc_t ref_crc;
> int flip_count;
> @@ -122,6 +127,7 @@ typedef struct {
> bool allow_fail;
> struct buf_ops *bops;
> bool atomic_path;
> + bool overlay_path;
> } data_t;
>
> static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
> @@ -213,6 +219,25 @@ static void require_atomic_async_cap(data_t *data)
> "Atomic flip async flip not supported\n");
> }
>
> +static void require_overlay_flip_support(data_t *data)
> +{
> + struct igt_fb *bufs = data->bufs_overlay;
> + igt_plane_t *plane = data->plane_overlay;
> + int flags = DRM_MODE_PAGE_FLIP_EVENT;
> +
> + igt_plane_set_fb(plane, &bufs[0]);
> +
> + igt_require_f(!igt_display_try_commit_atomic(&data->display, flags, data),
> + "Overlay planes not supported\n");
> +
> + flags |= DRM_MODE_PAGE_FLIP_ASYNC;
> +
> + igt_plane_set_fb(plane, &bufs[1]);
> +
> + igt_require_f(!igt_display_try_commit_atomic(&data->display, flags, data),
> + "Async flip for overlay planes not supported\n");
> +}
> +
> static void test_init(data_t *data)
> {
> drmModeModeInfo *mode;
> @@ -228,12 +253,15 @@ static void test_init(data_t *data)
> igt_output_set_pipe(data->output, data->pipe);
>
> data->plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
> + if (data->overlay_path)
> + data->plane_overlay = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_OVERLAY);
> }
>
> static void test_init_ops(data_t *data)
> {
> data->alternate_sync_async = false;
> data->atomic_path = false;
> + data->overlay_path = false;
> }
>
> static void test_init_fbs(data_t *data)
> @@ -242,6 +270,7 @@ static void test_init_fbs(data_t *data)
> uint32_t width, height;
> static uint32_t prev_output_id;
> static uint64_t prev_modifier;
> + static bool prev_overlay;
> drmModeModeInfo *mode;
>
> mode = igt_output_get_mode(data->output);
> @@ -249,21 +278,35 @@ static void test_init_fbs(data_t *data)
> height = mode->vdisplay;
>
> if (prev_output_id != data->output->id ||
> - prev_modifier != data->modifier) {
> + prev_modifier != data->modifier ||
> + prev_overlay != data->overlay_path) {
> prev_output_id = data->output->id;
> prev_modifier = data->modifier;
> + prev_overlay = data->overlay_path;
>
> if (data->bufs[0].fb_id) {
> - for (i = 0; i < NUM_FBS; i++)
> + for (i = 0; i < NUM_FBS; i++) {
> igt_remove_fb(data->drm_fd, &data->bufs[i]);
> + if (data->overlay_path)
> + igt_remove_fb(data->drm_fd, &data->bufs_overlay[i]);
> + }
> }
>
> - for (i = 0; i < NUM_FBS; i++)
> + for (i = 0; i < NUM_FBS; i++) {
> make_fb(data, &data->bufs[i], width, height, i);
> + if (data->overlay_path) {
> + make_fb(data, &data->bufs_overlay[i], width, height, i);
> + }
> + }
I don't think this logic is correct.
In the way it is implemented, if we are running a subtest just after the
overlay-atomic, but with `data->overlay_path = false`, bufs_overlays
won't be removed.
In addition, if we are running the overlay-atomic subtest, where
`prev_overlay != data->overlay_path`, data->bufs will be recreated
unnecessarily (because both `prev_overlay != data->overlay_path` and
`data->bufs[0].fb_id` conditions are satisfied).
I understand that what you intended to do would be something like this:
if (prev_output_id != data->output->id ||
prev_modifier != data->modifier) {
prev_output_id = data->output->id;
prev_modifier = data->modifier;
if (data->bufs[0].fb_id) {
for (i = 0; i < NUM_FBS; i++)
igt_remove_fb(data->drm_fd, &data->bufs[i]);
}
for (i = 0; i < NUM_FBS; i++)
make_fb(data, &data->bufs[i], width, height, i);
}
if (prev_overlay != data->overlay_path) {
prev_overlay = data->overlay_path;
if (data->bufs_overlay[0].fb_id) {
for (i = 0; i < NUM_FBS; i++)
igt_remove_fb(data->drm_fd, &data->bufs_overlay[i]);
}
for (i = 0; i < NUM_FBS; i++) {
if (data->overlay_path)
make_fb(data, &data->bufs_overlay[i], width, height, i);
}
}
Then I undertand we will only create bufs_overlays if we are running
overlay-atomic for the first time and remove those fbs in the first
execution of the next subtest that doesn't need it. And
`data->overlay_path` doesn't affect primary fbs if not needed.
> }
>
> igt_plane_set_fb(data->plane, &data->bufs[0]);
> igt_plane_set_size(data->plane, width, height);
> +
> + if (data->overlay_path) {
> + igt_plane_set_fb(data->plane_overlay, &data->bufs_overlay[0]);
> + igt_plane_set_size(data->plane_overlay, width, height);
> + }
> }
>
> static bool async_flip_needs_extra_frame(data_t *data)
> @@ -291,12 +334,17 @@ static bool async_flip_needs_extra_frame(data_t *data)
> static int perform_flip(data_t *data, int frame, int flags)
> {
> int ret;
> + igt_plane_t *plane;
> + struct igt_fb *bufs;
> +
> + plane = data->overlay_path ? data->plane_overlay : data->plane;
> + bufs = data->overlay_path ? data->bufs_overlay : data->bufs;
>
> if (!data->atomic_path) {
> ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> - data->bufs[frame % NUM_FBS].fb_id, flags, data);
> + bufs[frame % NUM_FBS].fb_id, flags, data);
> } else {
> - igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
> + igt_plane_set_fb(plane, &bufs[frame % NUM_FBS]);
> ret = igt_display_try_commit_atomic(&data->display, flags, data);
> }
>
> @@ -312,6 +360,9 @@ static void test_async_flip(data_t *data)
>
> igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>
> + if (data->overlay_path)
> + require_overlay_flip_support(data);
> +
> gettimeofday(&start, NULL);
> frame = 1;
> do {
> @@ -817,6 +868,14 @@ igt_main
> run_test(&data, test_async_flip);
> }
>
> + igt_describe("Verify overlay planes with async flips in atomic API");
> + igt_subtest_with_dynamic("overlay-atomic") {
Missing `test_init_ops()` here.
Melissa
> + igt_require(is_amdgpu_device(data.drm_fd));
> + data.atomic_path = true;
> + data.overlay_path = true;
> + 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") {
> @@ -920,8 +979,10 @@ igt_main
> }
>
> igt_fixture {
> - for (i = 0; i < NUM_FBS; i++)
> + for (i = 0; i < NUM_FBS; i++) {
> igt_remove_fb(data.drm_fd, &data.bufs[i]);
> + igt_remove_fb(data.drm_fd, &data.bufs_overlay[i]);
> + }
>
> if (is_intel_device(data.drm_fd))
> buf_ops_destroy(data.bops);
> --
> 2.49.0
>
More information about the igt-dev
mailing list