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

[Pekka Paalanen <pekka.paalanen@haloniitty.fi>]

[-- Attachment #1: Type: text/plain, Size: 4599 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]