On 09/02/24 05:32, Pekka Paalanen wrote: > On Thu, 8 Feb 2024 16:38:31 -0300 > Arthur Grillo <arthurgrillo@xxxxxxxxxx> wrote: > >> On 08/02/24 06:50, Pekka Paalanen wrote: >>> On Wed, 07 Feb 2024 17:17:15 -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. >>>> >>>> Link: https://lore.kernel.org/all/20240202214527.1d97c881@ferris.localdomain/ >>>> Suggested-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> >>>> Signed-off-by: Arthur Grillo <arthurgrillo@xxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxx/ >> >> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ >> >>>> + 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
