Message-ID: <20240327145916.6a9d9998.pekka.paalanen@collabora.com>
Date: Wed, 27 Mar 2024 14:59:16 +0200
From: Pekka Paalanen <pekka.paalanen@...labora.com>
To: Maíra Canal <mcanal@...lia.com>
Cc: Louis Chauvet <louis.chauvet@...tlin.com>, Rodrigo Siqueira
 <rodrigosiqueiramelo@...il.com>, Melissa Wen <melissa.srw@...il.com>,
 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 11/16] drm/vkms: Add YUV support

On Tue, 26 Mar 2024 16:57:03 +0100
Louis Chauvet <louis.chauvet@...tlin.com> wrote:

> Le 25/03/24 - 11:26, Maíra Canal a écrit :
> > On 3/13/24 14:45, Louis Chauvet wrote:  
> > > From: Arthur Grillo <arthurgrillo@...eup.net>
> > > 
> > > Add support to the YUV formats bellow:
> > > 
> > > - NV12/NV16/NV24
> > > - NV21/NV61/NV42
> > > - YUV420/YUV422/YUV444
> > > - YVU420/YVU422/YVU444
> > > 
> > > The conversion from yuv to rgb is done with fixed-point arithmetic, using
> > > 32.32 floats and the drm_fixed helpers.
> > > 
> > > To do the conversion, a specific matrix must be used for each color range
> > > (DRM_COLOR_*_RANGE) and encoding (DRM_COLOR_*). This matrix is stored in
> > > the `conversion_matrix` struct, along with the specific y_offset needed.
> > > This matrix is queried only once, in `vkms_plane_atomic_update` and
> > > stored in a `vkms_plane_state`. Those conversion matrices of each
> > > encoding and range were obtained by rounding the values of the original
> > > conversion matrices multiplied by 2^32. This is done to avoid the use of
> > > floating point operations.
> > > 
> > > The same reading function is used for YUV and YVU formats. As the only
> > > difference between those two category of formats is the order of field, a
> > > simple swap in conversion matrix columns allows using the same function.
> > > 
> > > Signed-off-by: Arthur Grillo <arthurgrillo@...eup.net>
> > > [Louis Chauvet:
> > > - Adapted Arthur's work
> > > - Implemented the read_line_t callbacks for yuv
> > > - add struct conversion_matrix
> > > - remove struct pixel_yuv_u8
> > > - update the commit message
> > > - Merge the modifications from Arthur]  
> > 
> > A Co-developed-by tag would be more appropriate.  
> 
> I am not the main author of this part, I only applied a few simple 
> suggestions, the complex part was done by Arthur.
> 
> I will wait for Arthur's confirmation to change it to Co-developed by if
> he agrees.

Co-developed-by is an additional tag, and does not replace S-o-b. To my
understanding, the kernel rules and Developers' Certificate of Origin
require S-o-b to be added by anyone who has taken a patch and
re-submitted it, regardless of who the original author is, and
especially if the patch was modified.

Personally I also like to keep the list of changes like Louis added, to
credit people better.

> > > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > ---
> > >   drivers/gpu/drm/vkms/vkms_drv.h     |  22 ++
> > >   drivers/gpu/drm/vkms/vkms_formats.c | 431 ++++++++++++++++++++++++++++++++++++
> > >   drivers/gpu/drm/vkms/vkms_formats.h |   4 +
> > >   drivers/gpu/drm/vkms/vkms_plane.c   |  17 +-
> > >   4 files changed, 473 insertions(+), 1 deletion(-)
> > > 

..

> > >   };
> > >   
> > >   static struct drm_plane_state *
> > > @@ -117,12 +129,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >   	drm_framebuffer_get(frame_info->fb);
> > >   	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_0 |
> > >   									  DRM_MODE_ROTATE_90 |
> > > +									  DRM_MODE_ROTATE_180 |  
> > 
> > Why do we need to add DRM_MODE_ROTATE_180 here? Isn't the same as
> > reflecting both along the X and Y axis?  
> 
> Oops, I had no intention of putting that change here. I will move it to 
> another patch.
> 
> I don't understand why DRM_MODE_ROTATE_180 isn't in this list. If I read 
> the drm_rotation_simplify documentation, it explains that this argument 
> should contain all supported rotations and reflections, and ROT_180 is 
> supported by VKMS. Perhaps this call is unnecessary because all 
> combinations are supported by vkms?

If you truly handle all bit patterns that the rotation bitfield can
have, then yes, the call seems unnecessary.

However, as documented, the bitfield contains redundancy: the same
orientation can be expressed in more than one bit pattern. One example
is that ROTATE_180 is equivalent to REFLECT_X | REFLECT_Y.

Since it's a bitmask, userspace can give you funny values like
ROTATE_0 | ROTATE_90 | ROTATE_180. That is a valid orientation of
270-degree rotation (according to UAPI doc), but it is very awkwardly
expressed, hence the need to normalise it into a minimal bit pattern.

It does not look like drm_rotation_simplify() actually does this
minimisation!

I was not able to tell if DRM common code actually stops userspace from
combining multiple ROTATE bits in the same value. I suspect it must
stop them, or perhaps all code dealing with rotation is actually broken.

drm_rotation_simplify() is useful for cases where your hardware does
not have exactly the same flexibility. Maybe it cannot do REFLECT_Y but
it can do everything else? Then drm_rotation_simplify() gives you a bit
pattern that you can use directly, or fails if the orientation is not
representable with what your hardware can do.

At least, that's my understanding of quickly glancing over it.

IOW, if you wanted to never have to deal with REFLECT_Y bit, you could
leave it out here. Or, if you never want to deal with ROTATE_180, leave
that out - you will get REFLECT_X | REFLECT_Y instead. In theory.


Thanks,
pq

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists