[i-g-t] benchmarks: Add VKMS benchmark

Commit Message

Arthur Grillo Feb. 7, 2024, 8:17 p.m. UTC

Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
frames flipping framebuffers in each plane.

Link: https://lore.kernel.org/all/[e-mail]/
Suggested-by: Pekka Paalanen <[e-mail]>
Signed-off-by: Arthur Grillo <[e-mail]>
---
This benchmark was suggested by Pekka Paalanen on [1] to better analyse
possible performance regression on the Virtual Kernel Modesetting(VKMS)
driver.

With this benchmark I was able to determine two performance regression:

- 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
- cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")

[1]: https://lore.kernel.org/all/[e-mail]/
---
 benchmarks/meson.build   |   1 +
 benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 204 insertions(+)


---
base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc
change-id: 20240207-bench-393789eaba47

Best regards,

Comments

Pekka Paalanen Feb. 8, 2024, 9:50 a.m. UTC | #1

On Wed, 07 Feb 2024 17:17:15 -0300
Arthur Grillo <[e-mail]> wrote:

> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> frames flipping framebuffers in each plane.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> Suggested-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: Arthur Grillo <[e-mail]>
> ---
> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> driver.
> 
> With this benchmark I was able to determine two performance regression:
> 
> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> ---
>  benchmarks/meson.build   |   1 +
>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 204 insertions(+)
> 
> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> index c451268bc44f..3aa66d6dffe2 100644
> --- a/benchmarks/meson.build
> +++ b/benchmarks/meson.build
> @@ -20,6 +20,7 @@ benchmark_progs = [
>  	'kms_vblank',
>  	'prime_lookup',
>  	'vgem_mmap',
> +	'vkms_stress',
>  ]
>  
>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> new file mode 100644
> index 000000000000..b9128c208861
> --- /dev/null
> +++ b/benchmarks/vkms_stress.c
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright © 2024 Arthur Grillo
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Arthur Grillo <[e-mail]>
> + *
> + */
> +
> +#include "igt.h"
> +
> +#define FRAME_COUNT 100
> +
> +struct rect_t {
> +	int x, y;
> +	int width, height;
> +};
> +
> +struct plane_t {
> +	igt_plane_t *base;
> +	struct rect_t rect;
> +	uint32_t format;
> +	struct igt_fb fbs[2];
> +};
> +
> +struct kms_t {
> +	struct plane_t primary;
> +	struct plane_t overlay_a;
> +	struct plane_t overlay_b;
> +	struct plane_t writeback;
> +};
> +
> +struct data_t {
> +	int fd;
> +	igt_display_t display;
> +	igt_output_t *wb_output;
> +	drmModeModeInfo *mode;
> +	struct kms_t kms;
> +};
> +
> +static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
> +{
> +	igt_create_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
> +				   double b)
> +{
> +	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			r, g, b,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_setup(struct plane_t *plane, int index)
> +{
> +	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
> +	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
> +	igt_plane_set_fb(plane->base, &plane->fbs[index]);
> +}
> +
> +static void gen_fbs(struct data_t *data)
> +{
> +	struct kms_t *kms = &data->kms;
> +	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
> +
> +	for (int i = 0; i < 2; i++) {
> +		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
> +
> +		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
> +
> +		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
> +
> +		kms->writeback.rect.width = mode->hdisplay;
> +		kms->writeback.rect.height = mode->vdisplay;
> +		plane_create_fb(&kms->writeback, data->fd, i);
> +	}
> +}
> +
> +static igt_output_t *find_wb_output(struct data_t *data)
> +{
> +	for (int i = 0; i < data->display.n_outputs; i++) {
> +		igt_output_t *output = &data->display.outputs[i];
> +
> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +			continue;
> +
> +		return output;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct kms_t default_kms = {
> +	.primary = {
> +		.rect = {
> +			.x = 101, .y = 0,
> +			.width = 3639, .height = 2161,
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +	.overlay_a = {
> +		.rect = {
> +			.x = 201, .y = 199,
> +			.width = 3033, .height = 1777,
> +		},
> +		.format = DRM_FORMAT_XRGB16161616,
> +	},
> +	.overlay_b = {
> +		.rect = {
> +			.x = 1800, .y = 250,
> +			.width = 1507, .height = 1400,
> +		},
> +		.format = DRM_FORMAT_ARGB8888,
> +	},
> +	.writeback = {
> +		.rect = {
> +			.x = 0, .y = 0,
> +			// Size is to be determined at runtime
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +};
> +
> +
> +igt_simple_main
> +{
> +	struct data_t data;
> +	enum pipe pipe = PIPE_NONE;
> +
> +	data.kms = default_kms;
> +

Hi Arthur,

all the above looks really good!

Some things below look strange to me, but I don't know the igt API.

> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> +
> +	igt_display_require(&data.display, data.fd);
> +
> +	kmstest_set_vt_graphics_mode();
> +
> +	igt_display_require(&data.display, data.fd);

Are you supposed to call igt_display_require twice?

> +	igt_require(data.display.is_atomic);
> +
> +	igt_display_require_output(&data.display);
> +
> +	igt_require(data.wb_output);
> +	igt_display_reset(&data.display);
> +
> +	data.wb_output = find_wb_output(&data);

Should igt_require(data.wb_output) be after find_wb_output?

> +
> +	for_each_pipe(&data.display, pipe) {
> +		igt_debug("Selecting pipe %s to %s\n",
> +			  kmstest_pipe_name(pipe),
> +			  igt_output_name(data.wb_output));
> +		igt_output_set_pipe(data.wb_output, pipe);

Isn't this strange if there are multiple pipes?

> +	}
> +
> +	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);

What's this commit needed for?

> +
> +	gen_fbs(&data);
> +
> +	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
> +	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 0);
> +	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 1);
> +
> +	for (int i = 0; i < FRAME_COUNT; i++) {
> +		int fb_index = i % 2;
> +
> +		plane_setup(&data.kms.primary, fb_index);
> +
> +		plane_setup(&data.kms.overlay_a, fb_index);
> +
> +		plane_setup(&data.kms.overlay_b, fb_index);
> +
> +		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
> +
> +		igt_display_commit2(&data.display, COMMIT_ATOMIC);
> +	}
> +
> +	igt_display_fini(&data.display);
> +	drm_close_driver(data.fd);
> +}

Aside those questions, I'm already happy giving a

Acked-by: Pekka Paalanen <[e-mail]>


Thanks,
pq

Kamil Konieczny Feb. 8, 2024, 11:25 a.m. UTC | #2

Hi Arthur,
On 2024-02-07 at 17:17:15 -0300, Arthur Grillo wrote:
> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> frames flipping framebuffers in each plane.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> Suggested-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: Arthur Grillo <[e-mail]>
> ---
> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> driver.
> 
> With this benchmark I was able to determine two performance regression:
> 
> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")

Could you move those explanations into commit description?

> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> ---
>  benchmarks/meson.build   |   1 +
>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 204 insertions(+)
> 
> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> index c451268bc44f..3aa66d6dffe2 100644
> --- a/benchmarks/meson.build
> +++ b/benchmarks/meson.build
> @@ -20,6 +20,7 @@ benchmark_progs = [
>  	'kms_vblank',
>  	'prime_lookup',
>  	'vgem_mmap',
> +	'vkms_stress',
>  ]
>  
>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> new file mode 100644
> index 000000000000..b9128c208861
> --- /dev/null
> +++ b/benchmarks/vkms_stress.c
> @@ -0,0 +1,203 @@

Please add here at begin:
// SPDX-License-Identifier: MIT

> +/*
> + * Copyright © 2024 Arthur Grillo
> + *

Keep above copyright and delete below text, it is replaced by SPDX.

Regards,
Kamil

> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Arthur Grillo <[e-mail]>
> + *
> + */
> +
> +#include "igt.h"
> +
> +#define FRAME_COUNT 100
> +
> +struct rect_t {
> +	int x, y;
> +	int width, height;
> +};
> +
> +struct plane_t {
> +	igt_plane_t *base;
> +	struct rect_t rect;
> +	uint32_t format;
> +	struct igt_fb fbs[2];
> +};
> +
> +struct kms_t {
> +	struct plane_t primary;
> +	struct plane_t overlay_a;
> +	struct plane_t overlay_b;
> +	struct plane_t writeback;
> +};
> +
> +struct data_t {
> +	int fd;
> +	igt_display_t display;
> +	igt_output_t *wb_output;
> +	drmModeModeInfo *mode;
> +	struct kms_t kms;
> +};
> +
> +static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
> +{
> +	igt_create_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
> +				   double b)
> +{
> +	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			r, g, b,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_setup(struct plane_t *plane, int index)
> +{
> +	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
> +	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
> +	igt_plane_set_fb(plane->base, &plane->fbs[index]);
> +}
> +
> +static void gen_fbs(struct data_t *data)
> +{
> +	struct kms_t *kms = &data->kms;
> +	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
> +
> +	for (int i = 0; i < 2; i++) {
> +		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
> +
> +		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
> +
> +		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
> +
> +		kms->writeback.rect.width = mode->hdisplay;
> +		kms->writeback.rect.height = mode->vdisplay;
> +		plane_create_fb(&kms->writeback, data->fd, i);
> +	}
> +}
> +
> +static igt_output_t *find_wb_output(struct data_t *data)
> +{
> +	for (int i = 0; i < data->display.n_outputs; i++) {
> +		igt_output_t *output = &data->display.outputs[i];
> +
> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +			continue;
> +
> +		return output;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct kms_t default_kms = {
> +	.primary = {
> +		.rect = {
> +			.x = 101, .y = 0,
> +			.width = 3639, .height = 2161,
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +	.overlay_a = {
> +		.rect = {
> +			.x = 201, .y = 199,
> +			.width = 3033, .height = 1777,
> +		},
> +		.format = DRM_FORMAT_XRGB16161616,
> +	},
> +	.overlay_b = {
> +		.rect = {
> +			.x = 1800, .y = 250,
> +			.width = 1507, .height = 1400,
> +		},
> +		.format = DRM_FORMAT_ARGB8888,
> +	},
> +	.writeback = {
> +		.rect = {
> +			.x = 0, .y = 0,
> +			// Size is to be determined at runtime
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +};
> +
> +
> +igt_simple_main
> +{
> +	struct data_t data;
> +	enum pipe pipe = PIPE_NONE;
> +
> +	data.kms = default_kms;
> +
> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> +
> +	igt_display_require(&data.display, data.fd);
> +
> +	kmstest_set_vt_graphics_mode();
> +
> +	igt_display_require(&data.display, data.fd);
> +	igt_require(data.display.is_atomic);
> +
> +	igt_display_require_output(&data.display);
> +
> +	igt_require(data.wb_output);
> +	igt_display_reset(&data.display);
> +
> +	data.wb_output = find_wb_output(&data);
> +
> +	for_each_pipe(&data.display, pipe) {
> +		igt_debug("Selecting pipe %s to %s\n",
> +			  kmstest_pipe_name(pipe),
> +			  igt_output_name(data.wb_output));
> +		igt_output_set_pipe(data.wb_output, pipe);
> +	}
> +
> +	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +
> +	gen_fbs(&data);
> +
> +	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
> +	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 0);
> +	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 1);
> +
> +	for (int i = 0; i < FRAME_COUNT; i++) {
> +		int fb_index = i % 2;
> +
> +		plane_setup(&data.kms.primary, fb_index);
> +
> +		plane_setup(&data.kms.overlay_a, fb_index);
> +
> +		plane_setup(&data.kms.overlay_b, fb_index);
> +
> +		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
> +
> +		igt_display_commit2(&data.display, COMMIT_ATOMIC);
> +	}
> +
> +	igt_display_fini(&data.display);
> +	drm_close_driver(data.fd);
> +}
> 
> ---
> base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc
> change-id: 20240207-bench-393789eaba47
> 
> Best regards,
> -- 
> Arthur Grillo <[e-mail]>
>

Juha-Pekka Heikkila Feb. 8, 2024, 12:51 p.m. UTC | #3

Hi Arthur,

I was taking brief look. Generally things look ok. Few comments below.

On 7.2.2024 22.17, Arthur Grillo wrote:
> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> frames flipping framebuffers in each plane.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> Suggested-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: Arthur Grillo <[e-mail]>
> ---
> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> driver.
> 
> With this benchmark I was able to determine two performance regression:
> 
> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> ---
>   benchmarks/meson.build   |   1 +
>   benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 204 insertions(+)
> 
> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> index c451268bc44f..3aa66d6dffe2 100644
> --- a/benchmarks/meson.build
> +++ b/benchmarks/meson.build
> @@ -20,6 +20,7 @@ benchmark_progs = [
>   	'kms_vblank',
>   	'prime_lookup',
>   	'vgem_mmap',
> +	'vkms_stress',
>   ]
>   
>   benchmarksdir = join_paths(libexecdir, 'benchmarks')
> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> new file mode 100644
> index 000000000000..b9128c208861
> --- /dev/null
> +++ b/benchmarks/vkms_stress.c
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright © 2024 Arthur Grillo
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Arthur Grillo <[e-mail]>
> + *
> + */
> +
> +#include "igt.h"
> +
> +#define FRAME_COUNT 100
> +
> +struct rect_t {
> +	int x, y;
> +	int width, height;
> +};
> +
> +struct plane_t {
> +	igt_plane_t *base;
> +	struct rect_t rect;
> +	uint32_t format;
> +	struct igt_fb fbs[2];
> +};
> +
> +struct kms_t {
> +	struct plane_t primary;
> +	struct plane_t overlay_a;
> +	struct plane_t overlay_b;
> +	struct plane_t writeback;
> +};
> +
> +struct data_t {
> +	int fd;
> +	igt_display_t display;
> +	igt_output_t *wb_output;
> +	drmModeModeInfo *mode;
> +	struct kms_t kms;
> +};
> +
> +static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
> +{
> +	igt_create_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
> +				   double b)
> +{
> +	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			r, g, b,
> +			&plane->fbs[index]);
> +}

These two above functions, why not just use the igt_create_* function 
instead of wrapping them with different name?

> +
> +static void plane_setup(struct plane_t *plane, int index)
> +{
> +	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
> +	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
> +	igt_plane_set_fb(plane->base, &plane->fbs[index]);
> +}
> +
> +static void gen_fbs(struct data_t *data)
> +{
> +	struct kms_t *kms = &data->kms;
> +	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
> +
> +	for (int i = 0; i < 2; i++) {

Maybe something along the lines of ARRAY_SIZE(kms->primary.fbs) instead 
of '2' ?

> +		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
> +
> +		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
> +
> +		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
> +
> +		kms->writeback.rect.width = mode->hdisplay;
> +		kms->writeback.rect.height = mode->vdisplay;
> +		plane_create_fb(&kms->writeback, data->fd, i);
> +	}
> +}
> +
> +static igt_output_t *find_wb_output(struct data_t *data)
> +{
> +	for (int i = 0; i < data->display.n_outputs; i++) {
> +		igt_output_t *output = &data->display.outputs[i];
> +
> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +			continue;
> +
> +		return output;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct kms_t default_kms = {
> +	.primary = {
> +		.rect = {
> +			.x = 101, .y = 0,
> +			.width = 3639, .height = 2161,
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +	.overlay_a = {
> +		.rect = {
> +			.x = 201, .y = 199,
> +			.width = 3033, .height = 1777,
> +		},
> +		.format = DRM_FORMAT_XRGB16161616,
> +	},
> +	.overlay_b = {
> +		.rect = {
> +			.x = 1800, .y = 250,
> +			.width = 1507, .height = 1400,
> +		},
> +		.format = DRM_FORMAT_ARGB8888,
> +	},
> +	.writeback = {
> +		.rect = {
> +			.x = 0, .y = 0,
> +			// Size is to be determined at runtime
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +};
> +
> +
> +igt_simple_main
> +{
> +	struct data_t data;
> +	enum pipe pipe = PIPE_NONE;
> +
> +	data.kms = default_kms;
> +
> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> +
> +	igt_display_require(&data.display, data.fd);
> +
> +	kmstest_set_vt_graphics_mode();
> +
> +	igt_display_require(&data.display, data.fd);
> +	igt_require(data.display.is_atomic);
> +
> +	igt_display_require_output(&data.display);
> +
> +	igt_require(data.wb_output);
> +	igt_display_reset(&data.display);
> +
> +	data.wb_output = find_wb_output(&data);

igt_require(data.wb_output)


I noticed Pekka had already commented on this and those couple of things 
below.


> +
> +	for_each_pipe(&data.display, pipe) {
> +		igt_debug("Selecting pipe %s to %s\n",
> +			  kmstest_pipe_name(pipe),
> +			  igt_output_name(data.wb_output));
> +		igt_output_set_pipe(data.wb_output, pipe);
> +	}
> +
> +	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +
> +	gen_fbs(&data);
> +
> +	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
> +	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 0);
> +	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 1);
> +
> +	for (int i = 0; i < FRAME_COUNT; i++) {
> +		int fb_index = i % 2;

I think this 2 could a be defined in the beginning giving it a name and 
used everywhere

> +
> +		plane_setup(&data.kms.primary, fb_index);
> +
> +		plane_setup(&data.kms.overlay_a, fb_index);
> +
> +		plane_setup(&data.kms.overlay_b, fb_index);
> +
> +		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
> +
> +		igt_display_commit2(&data.display, COMMIT_ATOMIC);
> +	}
> +
> +	igt_display_fini(&data.display);
> +	drm_close_driver(data.fd);
> +}
> 
> ---
> base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc
> change-id: 20240207-bench-393789eaba47
> 
> Best regards,

Kamil Konieczny Feb. 8, 2024, 2:05 p.m. UTC | #4

Hi Arthur,
On 2024-02-07 at 17:17:15 -0300, Arthur Grillo wrote:
> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> frames flipping framebuffers in each plane.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> Suggested-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: Arthur Grillo <[e-mail]>
> ---
> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> driver.
> 
> With this benchmark I was able to determine two performance regression:
> 
> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> ---
>  benchmarks/meson.build   |   1 +
>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
------------- ^^^^
Why did you named it vkms? Below at drm_open you use DRM_DRIVER_ANY
so rename it to something else? For example:

kms_fb_stress

Regards,
Kamil

>  2 files changed, 204 insertions(+)
> 
> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> index c451268bc44f..3aa66d6dffe2 100644
> --- a/benchmarks/meson.build
> +++ b/benchmarks/meson.build
> @@ -20,6 +20,7 @@ benchmark_progs = [
>  	'kms_vblank',
>  	'prime_lookup',
>  	'vgem_mmap',
> +	'vkms_stress',
>  ]
>  
>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> new file mode 100644
> index 000000000000..b9128c208861
> --- /dev/null
> +++ b/benchmarks/vkms_stress.c
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright © 2024 Arthur Grillo
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Arthur Grillo <[e-mail]>
> + *
> + */
> +
> +#include "igt.h"
> +
> +#define FRAME_COUNT 100
> +
> +struct rect_t {
> +	int x, y;
> +	int width, height;
> +};
> +
> +struct plane_t {
> +	igt_plane_t *base;
> +	struct rect_t rect;
> +	uint32_t format;
> +	struct igt_fb fbs[2];
> +};
> +
> +struct kms_t {
> +	struct plane_t primary;
> +	struct plane_t overlay_a;
> +	struct plane_t overlay_b;
> +	struct plane_t writeback;
> +};
> +
> +struct data_t {
> +	int fd;
> +	igt_display_t display;
> +	igt_output_t *wb_output;
> +	drmModeModeInfo *mode;
> +	struct kms_t kms;
> +};
> +
> +static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
> +{
> +	igt_create_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
> +				   double b)
> +{
> +	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
> +			plane->format, DRM_FORMAT_MOD_LINEAR,
> +			r, g, b,
> +			&plane->fbs[index]);
> +}
> +
> +static void plane_setup(struct plane_t *plane, int index)
> +{
> +	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
> +	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
> +	igt_plane_set_fb(plane->base, &plane->fbs[index]);
> +}
> +
> +static void gen_fbs(struct data_t *data)
> +{
> +	struct kms_t *kms = &data->kms;
> +	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
> +
> +	for (int i = 0; i < 2; i++) {
> +		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
> +
> +		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
> +
> +		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
> +
> +		kms->writeback.rect.width = mode->hdisplay;
> +		kms->writeback.rect.height = mode->vdisplay;
> +		plane_create_fb(&kms->writeback, data->fd, i);
> +	}
> +}
> +
> +static igt_output_t *find_wb_output(struct data_t *data)
> +{
> +	for (int i = 0; i < data->display.n_outputs; i++) {
> +		igt_output_t *output = &data->display.outputs[i];
> +
> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +			continue;
> +
> +		return output;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct kms_t default_kms = {
> +	.primary = {
> +		.rect = {
> +			.x = 101, .y = 0,
> +			.width = 3639, .height = 2161,
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +	.overlay_a = {
> +		.rect = {
> +			.x = 201, .y = 199,
> +			.width = 3033, .height = 1777,
> +		},
> +		.format = DRM_FORMAT_XRGB16161616,
> +	},
> +	.overlay_b = {
> +		.rect = {
> +			.x = 1800, .y = 250,
> +			.width = 1507, .height = 1400,
> +		},
> +		.format = DRM_FORMAT_ARGB8888,
> +	},
> +	.writeback = {
> +		.rect = {
> +			.x = 0, .y = 0,
> +			// Size is to be determined at runtime
> +		},
> +		.format = DRM_FORMAT_XRGB8888,
> +	},
> +};
> +
> +
> +igt_simple_main
> +{
> +	struct data_t data;
> +	enum pipe pipe = PIPE_NONE;
> +
> +	data.kms = default_kms;
> +
> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> +
> +	igt_display_require(&data.display, data.fd);
> +
> +	kmstest_set_vt_graphics_mode();
> +
> +	igt_display_require(&data.display, data.fd);
> +	igt_require(data.display.is_atomic);
> +
> +	igt_display_require_output(&data.display);
> +
> +	igt_require(data.wb_output);
> +	igt_display_reset(&data.display);
> +
> +	data.wb_output = find_wb_output(&data);
> +
> +	for_each_pipe(&data.display, pipe) {
> +		igt_debug("Selecting pipe %s to %s\n",
> +			  kmstest_pipe_name(pipe),
> +			  igt_output_name(data.wb_output));
> +		igt_output_set_pipe(data.wb_output, pipe);
> +	}
> +
> +	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +
> +	gen_fbs(&data);
> +
> +	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
> +	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 0);
> +	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
> +								  DRM_PLANE_TYPE_OVERLAY, 1);
> +
> +	for (int i = 0; i < FRAME_COUNT; i++) {
> +		int fb_index = i % 2;
> +
> +		plane_setup(&data.kms.primary, fb_index);
> +
> +		plane_setup(&data.kms.overlay_a, fb_index);
> +
> +		plane_setup(&data.kms.overlay_b, fb_index);
> +
> +		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
> +
> +		igt_display_commit2(&data.display, COMMIT_ATOMIC);
> +	}
> +
> +	igt_display_fini(&data.display);
> +	drm_close_driver(data.fd);
> +}
> 
> ---
> base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc
> change-id: 20240207-bench-393789eaba47
> 
> Best regards,
> -- 
> Arthur Grillo <[e-mail]>
>

Arthur Grillo Feb. 8, 2024, 7:38 p.m. UTC | #5

On 08/02/24 06:50, Pekka Paalanen wrote:
> On Wed, 07 Feb 2024 17:17:15 -0300
> Arthur Grillo <[e-mail]> wrote:
> 
>> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
>> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
>> frames flipping framebuffers in each plane.
>>
>> Link: https://lore.kernel.org/all/[e-mail]/
>> Suggested-by: Pekka Paalanen <[e-mail]>
>> Signed-off-by: Arthur Grillo <[e-mail]>
>> ---
>> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
>> possible performance regression on the Virtual Kernel Modesetting(VKMS)
>> driver.
>>
>> With this benchmark I was able to determine two performance regression:
>>
>> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
>> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
>>
>> [1]: https://lore.kernel.org/all/[e-mail]/
>> ---
>>  benchmarks/meson.build   |   1 +
>>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 204 insertions(+)
>>
>> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
>> index c451268bc44f..3aa66d6dffe2 100644
>> --- a/benchmarks/meson.build
>> +++ b/benchmarks/meson.build
>> @@ -20,6 +20,7 @@ benchmark_progs = [
>>  	'kms_vblank',
>>  	'prime_lookup',
>>  	'vgem_mmap',
>> +	'vkms_stress',
>>  ]
>>  
>>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
>> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
>> new file mode 100644
>> index 000000000000..b9128c208861
>> --- /dev/null
>> +++ b/benchmarks/vkms_stress.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * Copyright © 2024 Arthur Grillo
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + *    Arthur Grillo <[e-mail]>
>> + *
>> + */
>> +
>> +#include "igt.h"
>> +
>> +#define FRAME_COUNT 100
>> +
>> +struct rect_t {
>> +	int x, y;
>> +	int width, height;
>> +};
>> +
>> +struct plane_t {
>> +	igt_plane_t *base;
>> +	struct rect_t rect;
>> +	uint32_t format;
>> +	struct igt_fb fbs[2];
>> +};
>> +
>> +struct kms_t {
>> +	struct plane_t primary;
>> +	struct plane_t overlay_a;
>> +	struct plane_t overlay_b;
>> +	struct plane_t writeback;
>> +};
>> +
>> +struct data_t {
>> +	int fd;
>> +	igt_display_t display;
>> +	igt_output_t *wb_output;
>> +	drmModeModeInfo *mode;
>> +	struct kms_t kms;
>> +};
>> +
>> +static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
>> +{
>> +	igt_create_fb(fd, plane->rect.width, plane->rect.height,
>> +			plane->format, DRM_FORMAT_MOD_LINEAR,
>> +			&plane->fbs[index]);
>> +}
>> +
>> +static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
>> +				   double b)
>> +{
>> +	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
>> +			plane->format, DRM_FORMAT_MOD_LINEAR,
>> +			r, g, b,
>> +			&plane->fbs[index]);
>> +}
>> +
>> +static void plane_setup(struct plane_t *plane, int index)
>> +{
>> +	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
>> +	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
>> +	igt_plane_set_fb(plane->base, &plane->fbs[index]);
>> +}
>> +
>> +static void gen_fbs(struct data_t *data)
>> +{
>> +	struct kms_t *kms = &data->kms;
>> +	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
>> +
>> +	for (int i = 0; i < 2; i++) {
>> +		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
>> +
>> +		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
>> +
>> +		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
>> +
>> +		kms->writeback.rect.width = mode->hdisplay;
>> +		kms->writeback.rect.height = mode->vdisplay;
>> +		plane_create_fb(&kms->writeback, data->fd, i);
>> +	}
>> +}
>> +
>> +static igt_output_t *find_wb_output(struct data_t *data)
>> +{
>> +	for (int i = 0; i < data->display.n_outputs; i++) {
>> +		igt_output_t *output = &data->display.outputs[i];
>> +
>> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
>> +			continue;
>> +
>> +		return output;
>> +
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct kms_t default_kms = {
>> +	.primary = {
>> +		.rect = {
>> +			.x = 101, .y = 0,
>> +			.width = 3639, .height = 2161,
>> +		},
>> +		.format = DRM_FORMAT_XRGB8888,
>> +	},
>> +	.overlay_a = {
>> +		.rect = {
>> +			.x = 201, .y = 199,
>> +			.width = 3033, .height = 1777,
>> +		},
>> +		.format = DRM_FORMAT_XRGB16161616,
>> +	},
>> +	.overlay_b = {
>> +		.rect = {
>> +			.x = 1800, .y = 250,
>> +			.width = 1507, .height = 1400,
>> +		},
>> +		.format = DRM_FORMAT_ARGB8888,
>> +	},
>> +	.writeback = {
>> +		.rect = {
>> +			.x = 0, .y = 0,
>> +			// Size is to be determined at runtime
>> +		},
>> +		.format = DRM_FORMAT_XRGB8888,
>> +	},
>> +};
>> +
>> +
>> +igt_simple_main
>> +{
>> +	struct data_t data;
>> +	enum pipe pipe = PIPE_NONE;
>> +
>> +	data.kms = default_kms;
>> +
> 
> Hi Arthur,
> 
> all the above looks really good!
> 
> Some things below look strange to me, but I don't know the igt API.
> 
>> +	data.fd = drm_open_driver_master(DRIVER_ANY);
>> +
>> +	igt_display_require(&data.display, data.fd);
>> +
>> +	kmstest_set_vt_graphics_mode();
>> +
>> +	igt_display_require(&data.display, data.fd);
> 
> Are you supposed to call igt_display_require twice?
> 

Only this way the writeback connector appears. I took this from
`tests/kms_writeback.c`.

I now did a bit of lore.kernel.org archaeology and found why this is
necessary:

Rodrigo Siqueira sent this patch:
https://lore.kernel.org/all/[e-mail]/

It places drmSetClientCap() before drmModeGetResources(). (The patch
description is wrong, as noted by [1])

Without this, you don't need to call igt_display_require() twice. I
don't know if this patch is wrong, but it is good to bring it up for
discussion.

[1]: https://lore.kernel.org/all/[e-mail]/

>> +	igt_require(data.display.is_atomic);
>> +
>> +	igt_display_require_output(&data.display);
>> +
>> +	igt_require(data.wb_output);
>> +	igt_display_reset(&data.display);
>> +
>> +	data.wb_output = find_wb_output(&data);
> 
> Should igt_require(data.wb_output) be after find_wb_output?
> 
>> +
>> +	for_each_pipe(&data.display, pipe) {
>> +		igt_debug("Selecting pipe %s to %s\n",
>> +			  kmstest_pipe_name(pipe),
>> +			  igt_output_name(data.wb_output));
>> +		igt_output_set_pipe(data.wb_output, pipe);
> 
> Isn't this strange if there are multiple pipes?

Yeah, I forgot to place a `break;` there.

All the others comments will be addressed on v2.

Best Regards,
~Arthur Grillo

> 
>> +	}
>> +
>> +	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> 
> What's this commit needed for?
> 
>> +
>> +	gen_fbs(&data);
>> +
>> +	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
>> +	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
>> +								  DRM_PLANE_TYPE_OVERLAY, 0);
>> +	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
>> +								  DRM_PLANE_TYPE_OVERLAY, 1);
>> +
>> +	for (int i = 0; i < FRAME_COUNT; i++) {
>> +		int fb_index = i % 2;
>> +
>> +		plane_setup(&data.kms.primary, fb_index);
>> +
>> +		plane_setup(&data.kms.overlay_a, fb_index);
>> +
>> +		plane_setup(&data.kms.overlay_b, fb_index);
>> +
>> +		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
>> +
>> +		igt_display_commit2(&data.display, COMMIT_ATOMIC);
>> +	}
>> +
>> +	igt_display_fini(&data.display);
>> +	drm_close_driver(data.fd);
>> +}
> 
> Aside those questions, I'm already happy giving a
> 
> Acked-by: Pekka Paalanen <[e-mail]>
> 
> 
> Thanks,
> pq

Pekka Paalanen Feb. 9, 2024, 8:32 a.m. UTC | #6

On Thu, 8 Feb 2024 16:38:31 -0300
Arthur Grillo <[e-mail]> wrote:

> On 08/02/24 06:50, Pekka Paalanen wrote:
> > On Wed, 07 Feb 2024 17:17:15 -0300
> > Arthur Grillo <[e-mail]> wrote:
> >   
> >> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> >> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> >> frames flipping framebuffers in each plane.
> >>
> >> Link: https://lore.kernel.org/all/[e-mail]/
> >> Suggested-by: Pekka Paalanen <[e-mail]>
> >> Signed-off-by: Arthur Grillo <[e-mail]>
> >> ---
> >> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> >> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> >> driver.
> >>
> >> With this benchmark I was able to determine two performance regression:
> >>
> >> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> >> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
> >>
> >> [1]: https://lore.kernel.org/all/[e-mail]/
> >> ---
> >>  benchmarks/meson.build   |   1 +
> >>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 204 insertions(+)
> >>
> >> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> >> index c451268bc44f..3aa66d6dffe2 100644
> >> --- a/benchmarks/meson.build
> >> +++ b/benchmarks/meson.build
> >> @@ -20,6 +20,7 @@ benchmark_progs = [
> >>  	'kms_vblank',
> >>  	'prime_lookup',
> >>  	'vgem_mmap',
> >> +	'vkms_stress',
> >>  ]
> >>  
> >>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
> >> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> >> new file mode 100644
> >> index 000000000000..b9128c208861
> >> --- /dev/null
> >> +++ b/benchmarks/vkms_stress.c

...

> >> +
> >> +igt_simple_main
> >> +{
> >> +	struct data_t data;
> >> +	enum pipe pipe = PIPE_NONE;
> >> +
> >> +	data.kms = default_kms;
> >> +  
> > 
> > Hi Arthur,
> > 
> > all the above looks really good!
> > 
> > Some things below look strange to me, but I don't know the igt API.
> >   
> >> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> >> +
> >> +	igt_display_require(&data.display, data.fd);
> >> +
> >> +	kmstest_set_vt_graphics_mode();
> >> +
> >> +	igt_display_require(&data.display, data.fd);  
> > 
> > Are you supposed to call igt_display_require twice?
> >   
> 
> Only this way the writeback connector appears. I took this from
> `tests/kms_writeback.c`.
> 
> I now did a bit of lore.kernel.org archaeology and found why this is
> necessary:
> 
> Rodrigo Siqueira sent this patch:
> https://lore.kernel.org/all/[e-mail]/
> 
> It places drmSetClientCap() before drmModeGetResources(). (The patch
> description is wrong, as noted by [1])
> 
> Without this, you don't need to call igt_display_require() twice. I
> don't know if this patch is wrong, but it is good to bring it up for
> discussion.

Hi Arthur,

did you mean "*With* this, you don't need to call igt_display_require()
twice."?

All DRM client caps do need to be set before any call to GetResources
or anything else, exactly because the client caps change the kernel
side behaviour. Client caps may also hide things, not only expose
things, at least in the future if not already (color pipelines will
replace legacy color properties).

If you need to check DRM (kernel) caps, that should be done immediately
after setting DRM client caps. I think that's the most logical and
safest order.


Thanks,
pq

> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> 
> >> +	igt_require(data.display.is_atomic);
> >> +
> >> +	igt_display_require_output(&data.display);
> >> +
> >> +	igt_require(data.wb_output);
> >> +	igt_display_reset(&data.display);
> >> +
> >> +	data.wb_output = find_wb_output(&data);  
> > 
> > Should igt_require(data.wb_output) be after find_wb_output?
> >   
> >> +
> >> +	for_each_pipe(&data.display, pipe) {
> >> +		igt_debug("Selecting pipe %s to %s\n",
> >> +			  kmstest_pipe_name(pipe),
> >> +			  igt_output_name(data.wb_output));
> >> +		igt_output_set_pipe(data.wb_output, pipe);  
> > 
> > Isn't this strange if there are multiple pipes?  
> 
> Yeah, I forgot to place a `break;` there.
> 
> All the others comments will be addressed on v2.
> 
> Best Regards,
> ~Arthur Grillo

Arthur Grillo Feb. 9, 2024, 5:29 p.m. UTC | #7

On 09/02/24 05:32, Pekka Paalanen wrote:
> On Thu, 8 Feb 2024 16:38:31 -0300
> Arthur Grillo <[e-mail]> wrote:
> 
>> On 08/02/24 06:50, Pekka Paalanen wrote:
>>> On Wed, 07 Feb 2024 17:17:15 -0300
>>> Arthur Grillo <[e-mail]> wrote:
>>>   
>>>> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
>>>> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
>>>> frames flipping framebuffers in each plane.
>>>>
>>>> Link: https://lore.kernel.org/all/[e-mail]/
>>>> Suggested-by: Pekka Paalanen <[e-mail]>
>>>> Signed-off-by: Arthur Grillo <[e-mail]>
>>>> ---
>>>> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
>>>> possible performance regression on the Virtual Kernel Modesetting(VKMS)
>>>> driver.
>>>>
>>>> With this benchmark I was able to determine two performance regression:
>>>>
>>>> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
>>>> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
>>>>
>>>> [1]: https://lore.kernel.org/all/[e-mail]/
>>>> ---
>>>>  benchmarks/meson.build   |   1 +
>>>>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 204 insertions(+)
>>>>
>>>> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
>>>> index c451268bc44f..3aa66d6dffe2 100644
>>>> --- a/benchmarks/meson.build
>>>> +++ b/benchmarks/meson.build
>>>> @@ -20,6 +20,7 @@ benchmark_progs = [
>>>>  	'kms_vblank',
>>>>  	'prime_lookup',
>>>>  	'vgem_mmap',
>>>> +	'vkms_stress',
>>>>  ]
>>>>  
>>>>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
>>>> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
>>>> new file mode 100644
>>>> index 000000000000..b9128c208861
>>>> --- /dev/null
>>>> +++ b/benchmarks/vkms_stress.c
> 
> ...
> 
>>>> +
>>>> +igt_simple_main
>>>> +{
>>>> +	struct data_t data;
>>>> +	enum pipe pipe = PIPE_NONE;
>>>> +
>>>> +	data.kms = default_kms;
>>>> +  
>>>
>>> Hi Arthur,
>>>
>>> all the above looks really good!
>>>
>>> Some things below look strange to me, but I don't know the igt API.
>>>   
>>>> +	data.fd = drm_open_driver_master(DRIVER_ANY);
>>>> +
>>>> +	igt_display_require(&data.display, data.fd);
>>>> +
>>>> +	kmstest_set_vt_graphics_mode();
>>>> +
>>>> +	igt_display_require(&data.display, data.fd);  
>>>
>>> Are you supposed to call igt_display_require twice?
>>>   
>>
>> Only this way the writeback connector appears. I took this from
>> `tests/kms_writeback.c`.
>>
>> I now did a bit of lore.kernel.org archaeology and found why this is
>> necessary:
>>
>> Rodrigo Siqueira sent this patch:
>> https://lore.kernel.org/all/[e-mail]/
>>
>> It places drmSetClientCap() before drmModeGetResources(). (The patch
>> description is wrong, as noted by [1])
>>
>> Without this, you don't need to call igt_display_require() twice. I
>> don't know if this patch is wrong, but it is good to bring it up for
>> discussion.
> 
> Hi Arthur,
> 
> did you mean "*With* this, you don't need to call igt_display_require()
> twice."?

No, I really meant _without_. The patch has already been applied, it was
added inside commit 3642acbb74f5 ("lib/igt_kms: Add writeback support")
(it says on the commit message).

If you remove this change you don't need to call igt_display_require()
twice. But, based on you explanation, I don't think it's right to remove
it. Although, for some reason, you need call igt_display_require() twice
with this fix.

Best Regards,
~Arthur Grillo

> 
> All DRM client caps do need to be set before any call to GetResources
> or anything else, exactly because the client caps change the kernel
> side behaviour. Client caps may also hide things, not only expose
> things, at least in the future if not already (color pipelines will
> replace legacy color properties).
> 
> If you need to check DRM (kernel) caps, that should be done immediately
> after setting DRM client caps. I think that's the most logical and
> safest order.
> 
> 
> Thanks,
> pq
> 
>>
>> [1]: https://lore.kernel.org/all/[e-mail]/
>>
>>>> +	igt_require(data.display.is_atomic);
>>>> +
>>>> +	igt_display_require_output(&data.display);
>>>> +
>>>> +	igt_require(data.wb_output);
>>>> +	igt_display_reset(&data.display);
>>>> +
>>>> +	data.wb_output = find_wb_output(&data);  
>>>
>>> Should igt_require(data.wb_output) be after find_wb_output?
>>>   
>>>> +
>>>> +	for_each_pipe(&data.display, pipe) {
>>>> +		igt_debug("Selecting pipe %s to %s\n",
>>>> +			  kmstest_pipe_name(pipe),
>>>> +			  igt_output_name(data.wb_output));
>>>> +		igt_output_set_pipe(data.wb_output, pipe);  
>>>
>>> Isn't this strange if there are multiple pipes?  
>>
>> Yeah, I forgot to place a `break;` there.
>>
>> All the others comments will be addressed on v2.
>>
>> Best Regards,
>> ~Arthur Grillo

Pekka Paalanen Feb. 12, 2024, 8:43 a.m. UTC | #8

On Fri, 9 Feb 2024 14:29:30 -0300
Arthur Grillo <[e-mail]> wrote:

> On 09/02/24 05:32, Pekka Paalanen wrote:
> > On Thu, 8 Feb 2024 16:38:31 -0300
> > Arthur Grillo <[e-mail]> wrote:
> >   
> >> On 08/02/24 06:50, Pekka Paalanen wrote:  
> >>> On Wed, 07 Feb 2024 17:17:15 -0300
> >>> Arthur Grillo <[e-mail]> wrote:
> >>>     
> >>>> Create a benchmark for the VKMS driver. Use a KMS layout with deliberate
> >>>> odd sizes to try to avoid alignment accidents and run it for FRAME_COUNT
> >>>> frames flipping framebuffers in each plane.
> >>>>
> >>>> Link: https://lore.kernel.org/all/[e-mail]/
> >>>> Suggested-by: Pekka Paalanen <[e-mail]>
> >>>> Signed-off-by: Arthur Grillo <[e-mail]>
> >>>> ---
> >>>> This benchmark was suggested by Pekka Paalanen on [1] to better analyse
> >>>> possible performance regression on the Virtual Kernel Modesetting(VKMS)
> >>>> driver.
> >>>>
> >>>> With this benchmark I was able to determine two performance regression:
> >>>>
> >>>> - 322d716a3e8a ("drm/vkms: isolate pixel conversion functionality")
> >>>> - cc4fd2934d41 ("drm/vkms: Isolate writeback pixel conversion functions")
> >>>>
> >>>> [1]: https://lore.kernel.org/all/[e-mail]/
> >>>> ---
> >>>>  benchmarks/meson.build   |   1 +
> >>>>  benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 204 insertions(+)
> >>>>
> >>>> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> >>>> index c451268bc44f..3aa66d6dffe2 100644
> >>>> --- a/benchmarks/meson.build
> >>>> +++ b/benchmarks/meson.build
> >>>> @@ -20,6 +20,7 @@ benchmark_progs = [
> >>>>  	'kms_vblank',
> >>>>  	'prime_lookup',
> >>>>  	'vgem_mmap',
> >>>> +	'vkms_stress',
> >>>>  ]
> >>>>  
> >>>>  benchmarksdir = join_paths(libexecdir, 'benchmarks')
> >>>> diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
> >>>> new file mode 100644
> >>>> index 000000000000..b9128c208861
> >>>> --- /dev/null
> >>>> +++ b/benchmarks/vkms_stress.c  
> > 
> > ...
> >   
> >>>> +
> >>>> +igt_simple_main
> >>>> +{
> >>>> +	struct data_t data;
> >>>> +	enum pipe pipe = PIPE_NONE;
> >>>> +
> >>>> +	data.kms = default_kms;
> >>>> +    
> >>>
> >>> Hi Arthur,
> >>>
> >>> all the above looks really good!
> >>>
> >>> Some things below look strange to me, but I don't know the igt API.
> >>>     
> >>>> +	data.fd = drm_open_driver_master(DRIVER_ANY);
> >>>> +
> >>>> +	igt_display_require(&data.display, data.fd);
> >>>> +
> >>>> +	kmstest_set_vt_graphics_mode();
> >>>> +
> >>>> +	igt_display_require(&data.display, data.fd);    
> >>>
> >>> Are you supposed to call igt_display_require twice?
> >>>     
> >>
> >> Only this way the writeback connector appears. I took this from
> >> `tests/kms_writeback.c`.
> >>
> >> I now did a bit of lore.kernel.org archaeology and found why this is
> >> necessary:
> >>
> >> Rodrigo Siqueira sent this patch:
> >> https://lore.kernel.org/all/[e-mail]/
> >>
> >> It places drmSetClientCap() before drmModeGetResources(). (The patch
> >> description is wrong, as noted by [1])
> >>
> >> Without this, you don't need to call igt_display_require() twice. I
> >> don't know if this patch is wrong, but it is good to bring it up for
> >> discussion.  
> > 
> > Hi Arthur,
> > 
> > did you mean "*With* this, you don't need to call igt_display_require()
> > twice."?  
> 
> No, I really meant _without_. The patch has already been applied, it was
> added inside commit 3642acbb74f5 ("lib/igt_kms: Add writeback support")
> (it says on the commit message).
> 
> If you remove this change you don't need to call igt_display_require()
> twice. But, based on you explanation, I don't think it's right to remove
> it. Although, for some reason, you need call igt_display_require() twice
> with this fix.

If you set client caps before GetResources, there should not be any
need to call igt_display_require() twice, unless there is yet another
problem. Sounds like something to fix.

However, I cannot understand how reverting the change could *not*
require calling igt_display_require() twice. The call to GetResources
before setting client caps will not expose writeback connectors.


Thanks,
pq


> 
> Best Regards,
> ~Arthur Grillo
> 
> > 
> > All DRM client caps do need to be set before any call to GetResources
> > or anything else, exactly because the client caps change the kernel
> > side behaviour. Client caps may also hide things, not only expose
> > things, at least in the future if not already (color pipelines will
> > replace legacy color properties).
> > 
> > If you need to check DRM (kernel) caps, that should be done immediately
> > after setting DRM client caps. I think that's the most logical and
> > safest order.
> > 
> > 
> > Thanks,
> > pq
> >   
> >>
> >> [1]: https://lore.kernel.org/all/[e-mail]/
> >>  
> >>>> +	igt_require(data.display.is_atomic);
> >>>> +
> >>>> +	igt_display_require_output(&data.display);
> >>>> +
> >>>> +	igt_require(data.wb_output);
> >>>> +	igt_display_reset(&data.display);
> >>>> +
> >>>> +	data.wb_output = find_wb_output(&data);    
> >>>
> >>> Should igt_require(data.wb_output) be after find_wb_output?
> >>>     
> >>>> +
> >>>> +	for_each_pipe(&data.display, pipe) {
> >>>> +		igt_debug("Selecting pipe %s to %s\n",
> >>>> +			  kmstest_pipe_name(pipe),
> >>>> +			  igt_output_name(data.wb_output));
> >>>> +		igt_output_set_pipe(data.wb_output, pipe);    
> >>>
> >>> Isn't this strange if there are multiple pipes?    
> >>
> >> Yeah, I forgot to place a `break;` there.
> >>
> >> All the others comments will be addressed on v2.
> >>
> >> Best Regards,
> >> ~Arthur Grillo

Patch

diff --git a/benchmarks/meson.build b/benchmarks/meson.build
index c451268bc44f..3aa66d6dffe2 100644
--- a/benchmarks/meson.build
+++ b/benchmarks/meson.build
@@ -20,6 +20,7 @@  benchmark_progs = [
 	'kms_vblank',
 	'prime_lookup',
 	'vgem_mmap',
+	'vkms_stress',
 ]
 
 benchmarksdir = join_paths(libexecdir, 'benchmarks')
diff --git a/benchmarks/vkms_stress.c b/benchmarks/vkms_stress.c
new file mode 100644
index 000000000000..b9128c208861
--- /dev/null
+++ b/benchmarks/vkms_stress.c
@@ -0,0 +1,203 @@ 
+/*
+ * Copyright © 2024 Arthur Grillo
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Arthur Grillo <[e-mail]>
+ *
+ */
+
+#include "igt.h"
+
+#define FRAME_COUNT 100
+
+struct rect_t {
+	int x, y;
+	int width, height;
+};
+
+struct plane_t {
+	igt_plane_t *base;
+	struct rect_t rect;
+	uint32_t format;
+	struct igt_fb fbs[2];
+};
+
+struct kms_t {
+	struct plane_t primary;
+	struct plane_t overlay_a;
+	struct plane_t overlay_b;
+	struct plane_t writeback;
+};
+
+struct data_t {
+	int fd;
+	igt_display_t display;
+	igt_output_t *wb_output;
+	drmModeModeInfo *mode;
+	struct kms_t kms;
+};
+
+static void plane_create_fb(struct plane_t *plane, int fd, size_t index)
+{
+	igt_create_fb(fd, plane->rect.width, plane->rect.height,
+			plane->format, DRM_FORMAT_MOD_LINEAR,
+			&plane->fbs[index]);
+}
+
+static void plane_create_color_fb(struct plane_t *plane, int fd, size_t index, double r, double g,
+				   double b)
+{
+	igt_create_color_fb(fd, plane->rect.width, plane->rect.height,
+			plane->format, DRM_FORMAT_MOD_LINEAR,
+			r, g, b,
+			&plane->fbs[index]);
+}
+
+static void plane_setup(struct plane_t *plane, int index)
+{
+	igt_plane_set_size(plane->base, plane->rect.width, plane->rect.height);
+	igt_plane_set_position(plane->base, plane->rect.x, plane->rect.y);
+	igt_plane_set_fb(plane->base, &plane->fbs[index]);
+}
+
+static void gen_fbs(struct data_t *data)
+{
+	struct kms_t *kms = &data->kms;
+	drmModeModeInfo *mode = igt_output_get_mode(data->wb_output);
+
+	for (int i = 0; i < 2; i++) {
+		plane_create_color_fb(&kms->primary, data->fd, i, !i, i, i);
+
+		plane_create_color_fb(&kms->overlay_a, data->fd, i, i, !i, i);
+
+		plane_create_color_fb(&kms->overlay_b, data->fd, i, i, i, !i);
+
+		kms->writeback.rect.width = mode->hdisplay;
+		kms->writeback.rect.height = mode->vdisplay;
+		plane_create_fb(&kms->writeback, data->fd, i);
+	}
+}
+
+static igt_output_t *find_wb_output(struct data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		igt_output_t *output = &data->display.outputs[i];
+
+		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+			continue;
+
+		return output;
+
+	}
+
+	return NULL;
+}
+
+static struct kms_t default_kms = {
+	.primary = {
+		.rect = {
+			.x = 101, .y = 0,
+			.width = 3639, .height = 2161,
+		},
+		.format = DRM_FORMAT_XRGB8888,
+	},
+	.overlay_a = {
+		.rect = {
+			.x = 201, .y = 199,
+			.width = 3033, .height = 1777,
+		},
+		.format = DRM_FORMAT_XRGB16161616,
+	},
+	.overlay_b = {
+		.rect = {
+			.x = 1800, .y = 250,
+			.width = 1507, .height = 1400,
+		},
+		.format = DRM_FORMAT_ARGB8888,
+	},
+	.writeback = {
+		.rect = {
+			.x = 0, .y = 0,
+			// Size is to be determined at runtime
+		},
+		.format = DRM_FORMAT_XRGB8888,
+	},
+};
+
+
+igt_simple_main
+{
+	struct data_t data;
+	enum pipe pipe = PIPE_NONE;
+
+	data.kms = default_kms;
+
+	data.fd = drm_open_driver_master(DRIVER_ANY);
+
+	igt_display_require(&data.display, data.fd);
+
+	kmstest_set_vt_graphics_mode();
+
+	igt_display_require(&data.display, data.fd);
+	igt_require(data.display.is_atomic);
+
+	igt_display_require_output(&data.display);
+
+	igt_require(data.wb_output);
+	igt_display_reset(&data.display);
+
+	data.wb_output = find_wb_output(&data);
+
+	for_each_pipe(&data.display, pipe) {
+		igt_debug("Selecting pipe %s to %s\n",
+			  kmstest_pipe_name(pipe),
+			  igt_output_name(data.wb_output));
+		igt_output_set_pipe(data.wb_output, pipe);
+	}
+
+	igt_display_commit_atomic(&data.display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+
+	gen_fbs(&data);
+
+	data.kms.primary.base = igt_output_get_plane_type(data.wb_output, DRM_PLANE_TYPE_PRIMARY);
+	data.kms.overlay_a.base = igt_output_get_plane_type_index(data.wb_output,
+								  DRM_PLANE_TYPE_OVERLAY, 0);
+	data.kms.overlay_b.base = igt_output_get_plane_type_index(data.wb_output,
+								  DRM_PLANE_TYPE_OVERLAY, 1);
+
+	for (int i = 0; i < FRAME_COUNT; i++) {
+		int fb_index = i % 2;
+
+		plane_setup(&data.kms.primary, fb_index);
+
+		plane_setup(&data.kms.overlay_a, fb_index);
+
+		plane_setup(&data.kms.overlay_b, fb_index);
+
+		igt_output_set_writeback_fb(data.wb_output, &data.kms.writeback.fbs[fb_index]);
+
+		igt_display_commit2(&data.display, COMMIT_ATOMIC);
+	}
+
+	igt_display_fini(&data.display);
+	drm_close_driver(data.fd);
+}