Message-ID: <20240409103537.44e99854.pekka.paalanen@collabora.com>
Date: Tue, 9 Apr 2024 10:35:37 +0300
From: Pekka Paalanen <pekka.paalanen@...labora.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: 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>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 arthurgrillo@...eup.net, Jonathan Corbet <corbet@....net>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com,
 thomas.petazzoni@...tlin.com, seanpaul@...gle.com, marcheu@...gle.com,
 nicolejadeyee@...gle.com
Subject: Re: [PATCH v5 09/16] drm/vkms: Introduce pixel_read_direction enum

On Mon, 8 Apr 2024 09:50:18 +0200
Louis Chauvet <louis.chauvet@...tlin.com> wrote:

> Le 27/03/24 - 14:16, Pekka Paalanen a écrit :
> > On Tue, 26 Mar 2024 16:57:00 +0100
> > Louis Chauvet <louis.chauvet@...tlin.com> wrote:
> >   
> > > Le 25/03/24 - 15:11, Pekka Paalanen a écrit :  
> > > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > > Louis Chauvet <louis.chauvet@...tlin.com> wrote:
> > > >     
> > > > > The pixel_read_direction enum is useful to describe the reading direction
> > > > > in a plane. It avoids using the rotation property of DRM, which not
> > > > > practical to know the direction of reading.
> > > > > This patch also introduce two helpers, one to compute the
> > > > > pixel_read_direction from the DRM rotation property, and one to compute
> > > > > the step, in byte, between two successive pixel in a specific direction.
> > > > > 
> > > > > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/vkms/vkms_drv.h      | 11 +++++++++++
> > > > >  drivers/gpu/drm/vkms/vkms_formats.c  | 30 ++++++++++++++++++++++++++++++
> > > > >  3 files changed, 77 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > index 9254086f23ff..989bcf59f375 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c

> > > > I hope IGT uses FB patterns instead of solid color in its tests of
> > > > rotation to be able to detect the difference.    
> > > 
> > > They use solid colors, and even my new rotation test [3] use solid colors.  
> > 
> > That will completely fail to detect rotation and reflection bugs then.
> > E.g. userspace asks for 180-degree rotation, and the driver does not
> > rotate at all. Or rotate-180 getting confused with one reflection.  
> 
> I think I missunderstood what you means with "solid colors".
> 
> The tests uses a plane with multiple solid colors:
> 
> +-------+-------+
> | White | Red   |
> +-------+-------+
> | Blue  | Green |
> +-------+-------+
> 
> But it don't use gradients because of YUV.
>

Oh, that works. No worries then.

> > > It is mainly for yuv formats with subsampling: if you have formats with 
> > > subsampling, a "software rotated buffer" and a "hardware rotated buffer" 
> > > will not apply the same subsampling, so the colors will be slightly 
> > > different.  
> > 
> > Why would they not use the same subsampling?  
> 
> YUV422, for each pair of pixels along a horizontal line, the U and V 
> components are shared between those two pixels. However, along a vertical 
> line, each pixel has its own U and V components.
> 
> When you rotate an image by 90 degrees:
>  - Hardware Rotation: If you use hardware rotation, the YUV subsampling 
>    axis will align with what was previously the "White-Red" axis. The 
>    hardware will handle the rotation.
>  - Software Rotation: If you use software rotation, the YUV subsampling 
>    axis will align with what was previously the "Red-Green" axis.

That would be a bug in the software rotation.

> Because the subsampling compression axis changes depending on whether 
> you're using hardware or software rotation, the compression effect on 
> colors will differ. Specifically:
>  - Hardware rotation, a gradient along the "White-Red" axis may be 
>    compressed (i.e same UV component for multiple pixels along the 
>    gradient).
>  - Software rotation, the same gradient will not be compressed (i.e, each 
>    different color in the gradient have dedicated UV component)
> 
> The same reasoning also apply for "color borders", and my series [3] avoid 
> this issue by choosing the right number of pixels.

What is [3]?

I've used similar tactics in the Weston test suite, when I have no
implementation for chroma siting: the input and reference images
consist of 2x2 equal color pixel groups, so that chroma siting makes no
difference. When chroma siting will be implemented, the tests will be
extended.

Is there a TODO item to fix the software rotation bug and make the
tests more sensitive?

I think documenting this would be an ok intermediate solution.

> > The framebuffer contents are defined in its natural orientation, and
> > the subsampling applies in the natural orientation. If such a FB
> > is on a rotated plane, one must account for subsampling first, and
> > rotate second. 90-degree rotation does not change the encoded color.
> > 
> > Getting the subsampling exactly right is going to be necessary sooner
> > or later. There is no UAPI for setting chroma siting yet, but ideally
> > there should be.
> >   
> > > > The return values do seem correct to me, assuming I have guessed
> > > > correctly what "X" and "Y" refer to when combined with rotation. I did
> > > > not find good documentation about that.    
> > > 
> > > Yes, it is difficult to understand how rotation and reflexion should 
> > > works in drm. I spend half a day testing all the combination in drm_rect_* 
> > > helpers to understand how this works. According to the code:
> > > - If only rotation or only reflexion, easy as expected
> > > - If reflexion and rotation are mixed, the source buffer is first 
> > >   reflected and then rotated.  
> > 
> > Now that you know, you could send a documentation patch. :-)  
> 
> And now I'm not sure about it :)

You'll have people review the patch and confirm your understanding or
point out a mistake. A doc patch it easier to notice and jump in than
this series.

> I was running the tests on my v6, and for the first time ran my new 
> rotation [3] on the previous VKMS code. None of the tests for 
> ROT_90+reflexion and ROT_270+reflexion are passing...
> 
> So, either the previous vkms implementation was wrong, or mine is wrong :)
> 
> So, if a DRM expert can explain this, it could be nice.
> 
> To have a common example, if I take the same buffer as above 
> (white+red+blue+green), if I create a plane with rotation = 
> ROTATION_90 | REFLECTION_X, what is the expected result?
> 
> 1 - rotation then reflection 
> 
> +-------+-------+
> | Green | Red   |
> +-------+-------+
> | Blue  | White |
> +-------+-------+
> 
> 2 - reflection then rotation (my vkms implementation)
> 
> +-------+-------+
> | White | Blue  |
> +-------+-------+
> | Red   | Green |
> +-------+-------+
> 

I wish I knew. :-)

Thanks,
pq


> > For me as a userspace developer, the important place is
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties
> >   

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists