Message ID | 20240207-bench-v1-1-7135ad426860@riseup.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [i-g-t] benchmarks: Add VKMS benchmark | expand |
On Wed, 07 Feb 2024 17:17:15 -0300 Arthur Grillo <arthurgrillo@riseup.net> 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/20240202214527.1d97c881@ferris.localdomain/ > Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > --- > 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/20240202214527.1d97c881@ferris.localdomain/ > --- > 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 <arthurgrillo@riseup.net> > + * > + */ > + > +#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 <pekka.paalanen@collabora.com> Thanks, pq
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/20240202214527.1d97c881@ferris.localdomain/ > Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > --- > 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/20240202214527.1d97c881@ferris.localdomain/ > --- > 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 <arthurgrillo@riseup.net> > + * > + */ > + > +#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 <arthurgrillo@riseup.net> >
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/20240202214527.1d97c881@ferris.localdomain/ > Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > --- > 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/20240202214527.1d97c881@ferris.localdomain/ > --- > 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 <arthurgrillo@riseup.net> > + * > + */ > + > +#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,
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/20240202214527.1d97c881@ferris.localdomain/ > Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > --- > 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/20240202214527.1d97c881@ferris.localdomain/ > --- > 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 <arthurgrillo@riseup.net> > + * > + */ > + > +#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 <arthurgrillo@riseup.net> >
On 08/02/24 06:50, Pekka Paalanen wrote: > On Wed, 07 Feb 2024 17:17:15 -0300 > Arthur Grillo <arthurgrillo@riseup.net> 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/20240202214527.1d97c881@ferris.localdomain/ >> Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> >> --- >> 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/20240202214527.1d97c881@ferris.localdomain/ >> --- >> 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 <arthurgrillo@riseup.net> >> + * >> + */ >> + >> +#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/20190306213005.7hvbnwl7dohr3vuv@smtp.gmail.com/ 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/20190318104128.GH26454@e110455-lin.cambridge.arm.com/ >> + 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 <pekka.paalanen@collabora.com> > > > Thanks, > pq
On Thu, 8 Feb 2024 16:38:31 -0300 Arthur Grillo <arthurgrillo@riseup.net> wrote: > On 08/02/24 06:50, Pekka Paalanen wrote: > > On Wed, 07 Feb 2024 17:17:15 -0300 > > Arthur Grillo <arthurgrillo@riseup.net> 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/20240202214527.1d97c881@ferris.localdomain/ > >> Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > >> --- > >> 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/20240202214527.1d97c881@ferris.localdomain/ > >> --- > >> 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/20190306213005.7hvbnwl7dohr3vuv@smtp.gmail.com/ > > 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/20190318104128.GH26454@e110455-lin.cambridge.arm.com/ > > >> + 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
On 09/02/24 05:32, Pekka Paalanen wrote: > On Thu, 8 Feb 2024 16:38:31 -0300 > Arthur Grillo <arthurgrillo@riseup.net> wrote: > >> On 08/02/24 06:50, Pekka Paalanen wrote: >>> On Wed, 07 Feb 2024 17:17:15 -0300 >>> Arthur Grillo <arthurgrillo@riseup.net> 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/20240202214527.1d97c881@ferris.localdomain/ >>>> Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> >>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> >>>> --- >>>> 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/20240202214527.1d97c881@ferris.localdomain/ >>>> --- >>>> 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/20190306213005.7hvbnwl7dohr3vuv@smtp.gmail.com/ >> >> 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/20190318104128.GH26454@e110455-lin.cambridge.arm.com/ >> >>>> + 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
On Fri, 9 Feb 2024 14:29:30 -0300 Arthur Grillo <arthurgrillo@riseup.net> wrote: > On 09/02/24 05:32, Pekka Paalanen wrote: > > On Thu, 8 Feb 2024 16:38:31 -0300 > > Arthur Grillo <arthurgrillo@riseup.net> wrote: > > > >> On 08/02/24 06:50, Pekka Paalanen wrote: > >>> On Wed, 07 Feb 2024 17:17:15 -0300 > >>> Arthur Grillo <arthurgrillo@riseup.net> 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/20240202214527.1d97c881@ferris.localdomain/ > >>>> Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> > >>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> > >>>> --- > >>>> 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/20240202214527.1d97c881@ferris.localdomain/ > >>>> --- > >>>> 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/20190306213005.7hvbnwl7dohr3vuv@smtp.gmail.com/ > >> > >> 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/20190318104128.GH26454@e110455-lin.cambridge.arm.com/ > >> > >>>> + 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
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 <arthurgrillo@riseup.net> + * + */ + +#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); +}
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/20240202214527.1d97c881@ferris.localdomain/ Suggested-by: Pekka Paalanen <pekka.paalanen@haloniitty.fi> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net> --- 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/20240202214527.1d97c881@ferris.localdomain/ --- benchmarks/meson.build | 1 + benchmarks/vkms_stress.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+) --- base-commit: c58c5fb6aa1cb7d3627a15e364816a7a2add9edc change-id: 20240207-bench-393789eaba47 Best regards,