On Thu, 08 Feb 2024 17:04:52 -0300
Arthur Grillo <arthurgrillo@xxxxxxxxxx> 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.
>
> This benchmark was suggested by Pekka Paalanen 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")
>
> Link: https://lore.kernel.org/all/20240202214527.1d97c881@ferris.localdomain/
> Suggested-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Signed-off-by: Arthur Grillo <arthurgrillo@xxxxxxxxxx>
> ---
> Changes in v2:
> - Zero initialize data
> - Don't wrap igt_create_* functions (Juha-Pekka Heikkila)
> - Add a `break;` when a pipe is found (Pekka Paalanen)
> - Rename file to kms_fb_stress.c (Kamil Konieczny)
> - Replace 2 by NUM_FBS (Juha-Pekka Heikkila)
> - Replace license text by SPDX License (Kamil Konieczny)
> - Move explanations to commit description (Kamil Konieczny)
> - Require output after find_wb_output() (Pekka Paalanen & Juha-Pekka
> Heikkila)
> - Remove unnecessary commit (Pekka Paalanen)
> - Link to v1: https://lore.kernel.org/r/20240207-bench-v1-1-7135ad426860@xxxxxxxxxx
> ---
> benchmarks/kms_fb_stress.c | 176 +++++++++++++++++++++++++++++++++++++++++++++
> benchmarks/meson.build | 1 +
> 2 files changed, 177 insertions(+)
Looking good, thanks!
How do you choose the video mode, what is the CRTC size? It should be
big. Does IGT API automatically choose an explicit size, or?
It might be confusing if different people manage to run the benchmark
with different CRTC sizes. It would change the proportions of pixels
from each plane, and might also cut plane sizes to some nice round
values.
Of course, something should test planes extending outside of CRTC, but
I'm not sure that's explicitly necessary here.
It would also help people if the program reported the time spent in the
+ for (int i = 0; i < FRAME_COUNT; i++) {
loop specifically. Then all setup time will be ignored as it should, and
everyone will report the same measurement, be that kernel CPU time or
real time. Creating and filling the FBs could take time.
Thanks,
pq
>
> diff --git a/benchmarks/kms_fb_stress.c b/benchmarks/kms_fb_stress.c
> new file mode 100644
> index 000000000000..af0014bed672
> --- /dev/null
> +++ b/benchmarks/kms_fb_stress.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Arthur Grillo
> + */
> +
> +#include "igt.h"
> +
> +#define FRAME_COUNT 100
> +#define NUM_FBS 2
> +
> +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[NUM_FBS];
> +};
> +
> +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_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 < NUM_FBS; i++) {
> + igt_create_color_fb(data->fd, kms->primary.rect.width, kms->primary.rect.height,
> + kms->primary.format, DRM_FORMAT_MOD_LINEAR,
> + !i, i, i,
> + &kms->primary.fbs[i]);
> +
> + igt_create_color_fb(data->fd, kms->overlay_a.rect.width, kms->overlay_a.rect.height,
> + kms->overlay_a.format, DRM_FORMAT_MOD_LINEAR,
> + i, !i, i,
> + &kms->overlay_a.fbs[i]);
> +
> + igt_create_color_fb(data->fd, kms->overlay_b.rect.width, kms->overlay_b.rect.height,
> + kms->overlay_b.format, DRM_FORMAT_MOD_LINEAR,
> + i, i, !i,
> + &kms->overlay_b.fbs[i]);
> +
> + kms->writeback.rect.width = mode->hdisplay;
> + kms->writeback.rect.height = mode->vdisplay;
> + igt_create_fb(data->fd, kms->writeback.rect.width, kms->writeback.rect.height,
> + kms->writeback.format, DRM_FORMAT_MOD_LINEAR,
> + &kms->writeback.fbs[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 = {0};
> + 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_display_reset(&data.display);
> +
> + data.wb_output = find_wb_output(&data);
> + igt_require(data.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);
> + break;
> + }
> +
> + 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 % NUM_FBS;
> +
> + 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);
> +}
> diff --git a/benchmarks/meson.build b/benchmarks/meson.build
> index c451268bc44f..e949e6073719 100644
> --- a/benchmarks/meson.build
> +++ b/benchmarks/meson.build
> @@ -20,6 +20,7 @@ benchmark_progs = [
> 'kms_vblank',
> 'prime_lookup',
> 'vgem_mmap',
> + 'kms_fb_stress',
> ]
>
> benchmarksdir = join_paths(libexecdir, 'benchmarks')
>
> ---
> base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc
> change-id: 20240207-bench-393789eaba47
>
> Best regards,
Attachment:
pgpp5mN4TtTfE.pgp
Description: OpenPGP digital signature
