Date: Fri, 2 Feb 2024 13:13:22 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Pekka Paalanen <pekka.paalanen@...oniitty.fi>, Louis Chauvet
 <louis.chauvet@...tlin.com>, Rodrigo Siqueira
 <rodrigosiqueiramelo@...il.com>, Melissa Wen <melissa.srw@...il.com>,
 Maíra Canal <mairacanal@...eup.net>, Haneen Mohammed
 <hamohammed.sa@...il.com>, Daniel Vetter <daniel@...ll.ch>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 marcheu@...gle.com, seanpaul@...gle.com, nicolejadeyee@...gle.com,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 thomas.petazzoni@...tlin.com, Arthur Grillo <arthurgrillo@...eup.net>
Subject: Re: [PATCH 2/2] drm/vkms: Use a simpler composition function

Hello Maxime,

+ Arthur

mripard@...nel.org 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@...oniitty.fi wrote on Fri, 2 Feb 2024 10:55:22 +0200:
> >   
> > > On Thu, 01 Feb 2024 18:31:32 +0100
> > > Louis Chauvet <louis.chauvet@...tlin.com> 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@riseup.net/

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?

Thanks,
Miquèl

Powered by blists - more mailing lists