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

[Arthur Grillo]

On 09/02/24 05:32, Pekka Paalanen wrote:
> On Thu, 8 Feb 2024 16:38:31 -0300
> Arthur Grillo <arthurgrillo at riseup.net> wrote:
> 
>> On 08/02/24 06:50, Pekka Paalanen wrote:
>>> On Wed, 07 Feb 2024 17:17:15 -0300
>>> Arthur Grillo <arthurgrillo at 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 at haloniitty.fi>
>>>> Signed-off-by: Arthur Grillo <arthurgrillo at 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