Re: [PATCH] drm/vkms: Add information on how to benchmark

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 27/02/24 - 15:26, Pekka Paalanen a écrit :
> On Tue, 27 Feb 2024 09:29:58 -0300
> Arthur Grillo <arthurgrillo@riseup.net> wrote:
> 
> > On 27/02/24 08:55, Pekka Paalanen wrote:
> > > On Tue, 27 Feb 2024 08:44:52 -0300
> > > Arthur Grillo <arthurgrillo@riseup.net> wrote:
> > >   
> > >> On 27/02/24 06:19, Pekka Paalanen wrote:  
> > >>> On Mon, 26 Feb 2024 17:42:11 -0300
> > >>> Arthur Grillo <arthurgrillo@riseup.net> wrote:
> > >>>     
> > >>>> Now that we have a defined benchmark for testing the driver, add
> > >>>> documentation on how to run it.
> > >>>>
> > >>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > >>>> ---
> > >>>>  Documentation/gpu/vkms.rst | 6 ++++++
> > >>>>  1 file changed, 6 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> > >>>> index ba04ac7c2167..6d07f79f77ff 100644
> > >>>> --- a/Documentation/gpu/vkms.rst
> > >>>> +++ b/Documentation/gpu/vkms.rst
> > >>>> @@ -89,6 +89,12 @@ You can also run subtests if you do not want to run the entire test::
> > >>>>    sudo ./build/tests/kms_flip --run-subtest basic-plain-flip --device "sys:/sys/devices/platform/vkms"
> > >>>>    sudo IGT_DEVICE="sys:/sys/devices/platform/vkms" ./build/tests/kms_flip --run-subtest basic-plain-flip
> > >>>>  
> > >>>> +If you are developing features that may affect performance, you can run the kms_fb_stress    
> > >>>
> > >>> s/can/must/
> > >>>     
> > >>>> +benchmark::    
> > >>>
> > >>> before and after, and report the numbers.    
> > >>
> > >> Did you mean to write the benchmarks logs here?  
> > > 
> > > I mean people should be required tell their before and after numbers in
> > > either commit message (my preference) or in series cover letter (if
> > > benchmarking commits is not useful).
> > > 
> > > With the addition of YUV support in VKMS, maybe the benchmark needs to  
> > 
> > With the upcoming addition, I've sent a patch to arbitrarily change the
> > formats on the benchmark via command-line options. It's not adding a new
> > case, but maybe just this could already help.
> > 
> > https://lore.kernel.org/all/20240226-kms_fb_stress-dev-v1-0-1c14942b1244@riseup.net/
> 
> In that case you would need to document exactly what command line
> options to use, and ask people to report the numbers of each test
> case.
> 
> That works. Alternatively or additionally, the test cases could be
> built in to the benchmark, and it just reports numbers for all of them
> in a single invocation. Then people running the standard benchmark do
> not need to worry about getting the command line options right, or
> running multiple cases. And reviewers do not need to ask to re-run with
> the correct options.
> 
> I suppose rotations might get added, too.
> 
> Or maybe you'd provide a script that covers all the standard
> performance test cases?

I agree with Pekka, it would be nice to have a simple "bench everything" 
tool. Like kms_rotation is a test for all rotations, kms_plane for color 
conversions... kms_fb_test can run a few combinations (rgb+norotation, 
rgb+yuv, rgb+rotation...) and report a "global result" (this way it's easy 
to spot a regression not related directly to your changes).

I don't know the IGT benchmark API, but I think there is a way to create 
"sub-benchmarks" so you can run a specific benchmark when developping and 
the whole thing before pushing your series.

Kind regards,
Louis Chauvet

> 
> Thanks,
> pq
> 
> > > start printing YUV numbers separately as a new case.
> > > 
> > > 
> > > Thanks,
> > > pq
> > >   
> > >>  
> > >>>     
> > >>>> +
> > >>>> +  sudo ./build/benchmarks/kms_fb_stress --device "sys:/sys/devices/platform/vkms"
> > >>>> +  sudo IGT_DEVICE="sys:/sys/devices/platform/vkms" ./build/benchmarks/kms_fb_stress    
> > >>>
> > >>> Do people need to run both commands?    
> > >>
> > >> No, they don't, just two options.
> > >>
> > >> Best Regards,
> > >> ~Arthur Grillo
> > >>  
> > >>>
> > >>> Anyway, a good idea.
> > >>>
> > >>> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >>>
> > >>>
> > >>> Thanks,
> > >>> pq
> > >>>     
> > >>>> +
> > >>>>  TODO
> > >>>>  ====
> > >>>>  
> > >>>>
> > >>>> ---
> > >>>> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
> > >>>> change-id: 20240226-bench-vkms-5b8b7aab255e
> > >>>>
> > >>>> Best regards,    
> > >>>     
> > >   
> 



-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com