On Fri, 2 Feb 2024 17:07:34 +0100 Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > Hi Pekka, Hi Miquel, I'm happy to see no hard feelings below. I know I may be blunt at times. Another thing coming to my mind is that I suppose there is no agreed standard benchmark for VKMS performance, is there? I recall people running selected IGT tests in a loop in the past, and I worry that the IGT start-up overhead and small plane dimensions might skew the results. Would it be possible to have a standardised benchmark specifically for performance rather than correctness, in IGT or where-ever it would make sense? Then it would be simple to tell contributors to run this and report the numbers before and after. I would propose this kind of KMS layout: - CRTC size 3841 x 2161 - primary plane, XRGB8888, 3639 x 2161 @ 101,0 - overlay A, XBGR2101010, 3033 x 1777 @ 201,199 - overlay B, ARGB8888, 1507 x 1400 @ 1800,250 The sizes and positions are deliberately odd to try to avoid happy alignment accidents. The planes are big, which should let the pixel operations easily dominate performance measurement. There are different pixel formats, both opaque and semi-transparent. There is lots of plane overlap. The planes also do not cover the whole CRTC leaving the background visible a bit. There should be two FBs per each plane, flipped alternatingly each frame. Writeback should be active. Run this a number of frames, say, 100, and measure the kernel CPU time taken. It's supposed to take at least several seconds in total. I think something like this should be the base benchmark. One can add more to it, like rotated planes, YUV planes, etc. or switch settings on the existing planes. Maybe even FB_DAMAGE_CLIPS. Maybe one more overlay that is very tall and thin. Just an idea, what do you all think? Thanks, pq > pekka.paalanen@xxxxxxxxxxxxx wrote on Fri, 2 Feb 2024 17:49:13 +0200: > > > On Fri, 2 Feb 2024 13:13:22 +0100 > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > > > > Hello Maxime, > > > > > > + Arthur > > > > > > mripard@xxxxxxxxxx wrote on Fri, 2 Feb 2024 10:53:37 +0100: > > > > > > > Hi Miquel, > > > > > > > > On Fri, Feb 02, 2024 at 10:26:01AM +0100, Miquel Raynal wrote: > > > > > pekka.paalanen@xxxxxxxxxxxxx wrote on Fri, 2 Feb 2024 10:55:22 +0200: > > > > > > > > > > > On Thu, 01 Feb 2024 18:31:32 +0100 > > > > > > Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote: > > > > > > > > > > > > > Change the composition algorithm to iterate over pixels instead of lines. > > > > > > > It allows a simpler management of rotation and pixel access for complex formats. > > > > > > > > > > > > > > This new algorithm allows read_pixel function to have access to x/y > > > > > > > coordinates and make it possible to read the correct thing in a block > > > > > > > when block_w and block_h are not 1. > > > > > > > The iteration pixel-by-pixel in the same method also allows a simpler > > > > > > > management of rotation with drm_rect_* helpers. This way it's not needed > > > > > > > anymore to have misterious switch-case distributed in multiple places. > > > > > > > > > > > > Hi, > > > > > > > > > > > > there was a very good reason to write this code using lines: > > > > > > performance. Before lines, it was indeed operating on individual pixels. > > > > > > > > > > > > Please, include performance measurements before and after this series > > > > > > to quantify the impact on the previously already supported pixel > > > > > > formats, particularly the 32-bit-per-pixel RGB variants. > > > > > > > > > > > > VKMS will be used more and more in CI for userspace projects, and > > > > > > performance actually matters there. > > > > > > > > > > > > I'm worrying that this performance degradation here is significant. I > > > > > > believe it is possible to keep blending with lines, if you add new line > > > > > > getters for reading from rotated, sub-sampled etc. images. That way you > > > > > > don't have to regress the most common formats' performance. > > > > > > > > > > While I understand performance is important and should be taken into > > > > > account seriously, I cannot understand how broken testing could be > > > > > considered better. Fast but inaccurate will always be significantly > > > > > less attractive to my eyes. > > > > > > > > AFAIK, neither the cover letter nor the commit log claimed it was fixing > > > > something broken, just that it was "better" (according to what > > > > criteria?). > > > > > > Better is probably too vague and I agree the "fixing" part is not > > > clearly explained in the commit log. The cover-letter however states: > > > > > > > Patch 2/2: This patch is more complex. My main target was to solve issues > > > > I found in [1], but as it was very complex to do it "in place", I choose > > > > to rework the composition function. > > > ... > > > > [1]: https://lore.kernel.org/dri-devel/20240110-vkms-yuv-v2-0-952fcaa5a193@xxxxxxxxxx/ > > > > > > If you follow this link you will find all the feedback and especially > > > the "broken" parts. Just to be clear, writing bugs is totally expected > > > and review/testing is supposed to help on this regard. I am not blaming > > > the author in any way, just focusing on getting this code in a more > > > readable shape and hopefully reinforce the testing procedure. > > > > > > > If something is truly broken, it must be stated what exactly is so we > > > > can all come up with a solution that will satisfy everyone. > > > > > > Maybe going through the series pointed above will give more context > > > but AFAIU: the YUV composition is not totally right (and the tests used > > > to validate it need to be more complex as well in order to fail). > > > > > > Here is a proposal. > > > > > > Today's RGB implementation is only optimized in the line-by-line case > > > when there is no rotation. The logic is bit convoluted and may possibly > > > be slightly clarified with a per-format read_line() implementation, > > > at a very light performance cost. Such an improvement would definitely > > > benefit to the clarity of the code, especially when transformations > > > (especially the rotations) come into play because they would be clearly > > > handled differently instead of being "hidden" in the optimized logic. > > > Performances would not change much as this path is not optimized today > > > anyway (the pixel-oriented logic is already used in the rotation case). > > > > > > Arthur's YUV implementation is indeed well optimized but the added > > > complexity probably lead to small mistakes in the logic. The > > > per-format read_line() implementation mentioned above could be > > > extended to the YUV format as well, which would leverage Arthur's > > > proposal by re-using his optimized version. Louis will help on this > > > regard. However, for more complex cases such as when there is a > > > rotation, it will be easier (and not sub-optimized compared to the RGB > > > case) to also fallback to a pixel-oriented processing. > > > > > > Would this approach make sense? > > > > Hi, > > > > I think it would, if I understand what you mean. Ever since I proposed > > a line-by-line algorithm to improve the performance, I was thinking of > > per-format read_line() functions that would be selected outside of any > > loops. Extending that to support YUV is only natural. I can imagine > > rotation complicates things, and I won't oppose that resulting in a > > much heavier read_line() implementation used in those cases. They might > > perhaps call the original read_line() implementations pixel-by-pixel or > > plane-by-plane (i.e. YUV planes) per pixel. Chroma-siting complicates > > things even further. That way one could compose any > > rotation-format-siting combination by chaining function pointers. > > I'll let Louis also validate but on my side I feel like I totally > agree with your feedback. > > > I haven't looked at VKMS in a long time, and I am disappointed to find > > that vkms_compose_row() is calling plane->pixel_read() pixel-by-pixel. > > The reading vfunc should be called with many pixels at a time when the > > source FB layout allows it. The whole point of the line-based functions > > was that they repeat the innermost loop in every function body to make > > the per-pixel overhead as small as possible. The VKMS implementations > > benchmarked before and after the original line-based algorithm showed > > that calling a function pointer per-pixel is relatively very expensive. > > Or maybe it was a switch-case. > > Indeed, since your initial feedback Louis made a couple of comparisons > and the time penalty is between +5% and +60% depending on the case, > AFAIR. > > > Sorry, I didn't realize the optimization had already been lost. > > No problem, actually I also lost myself in my first answer as I > initially thought the (mainline) RGB logic was also broken in edge > cases, which it was not, only the YUV logic suffered from some > limitations. > > > Btw. I'd suggest renaming vkms_compose_row() to vkms_fetch_row() since > > it's not composing anything and the name mislead me. > > Makes sense. > > > I think if you inspect the compositing code as of revision > > 8356b97906503a02125c8d03c9b88a61ea46a05a you'll get a better feeling of > > what it was supposed to be. > > Excellent, thanks a lot! > > Miquèl -- Pekka Paalanen
Attachment:
pgpH0cY2YYbZz.pgp
Description: OpenPGP digital signature
