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

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

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

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  


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