On Fri, 9 Feb 2024 14:29:30 -0300 Arthur Grillo <arthurgrillo@xxxxxxxxxx> wrote: > 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. 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@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
Attachment:
pgpWh3WO18PIM.pgp
Description: OpenPGP digital signature
