Re: [PATCH v5 11/16] drm/vkms: Add YUV support

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 7165 bytes --]

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

> Le 27/03/24 - 16:23, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:05 +0100
> > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >   
> > > From: Arthur Grillo <arthurgrillo@riseup.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.  
> > 
> > You mean fixed-point, not floating-point (floats).
> >   
> > > 
> > > 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.  
> > 
> > Sounds good!
> >   
> > > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.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]
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.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(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 23e1d247468d..f3116084de5a 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h

...

> > > +static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> > > +						  struct conversion_matrix *matrix)  
> > 
> > If you are using the "swap the matrix columns" trick, then you cannot
> > call these cb, cr nor even u,v, because they might be the opposite.
> > They are simply the first and second chroma channel, and their meaning
> > depends on the given matrix.  
> 
> I will rename them for v6, channel_1 and channel_2.
> 
> > > +{
> > > +	u8 r, g, b;
> > > +	s64 fp_y, fp_cb, fp_cr;
> > > +	s64 fp_r, fp_g, fp_b;
> > > +
> > > +	fp_y = y - matrix->y_offset;
> > > +	fp_cb = cb - 128;
> > > +	fp_cr = cr - 128;  
> > 
> > This looks like an incorrect way to convert u8 to fixed-point, but...
> >  
> > > +
> > > +	fp_y = drm_int2fixp(fp_y);
> > > +	fp_cb = drm_int2fixp(fp_cb);
> > > +	fp_cr = drm_int2fixp(fp_cr);  
> > 
> > I find it confusing to re-purpose variables like this.
> > 
> > I'd do just
> > 
> > 	fp_c1 = drm_int2fixp((int)c1 - 128);  
> 
> I agree with this remark, I will change it for the v6.
> 
> > If the function arguments were int to begin with, then the cast would
> > be obviously unnecessary.  
> 
> For this I'm less sure. The name of the function and the usage is 
> explicit: we want to use u8 as input. As we manipulate pointers in 
> read_line, I don't know how it will works if the pointer is dereferenced 
> to a int instead of a u8.

Dereference operator acts on its input type. What happens to the result
is irrelevant.

If we have

u8 *p = ...;

void foo(int x);

then you can call

foo(*v);

if that was your question. Dereference acts on u8* which results in u8.
Then it gets implicitly cast to int.

However, you have a semantic reason to keep the argument as u8, and
that is fine.

> > So, what you have in fp variables at this point is fractional numbers
> > in the 8-bit integer scale. However, because the target format is
> > 16-bit, you should not show the extra precision away here. Instead,
> > multiply by 257 to bring the values to 16-bit scale, and do the RGB
> > clamping to 16-bit, not 8-bit.
> >   
> > > +
> > > +	fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
> > > +	       drm_fixp_mul(matrix->matrix[0][1], fp_cb) +
> > > +	       drm_fixp_mul(matrix->matrix[0][2], fp_cr);
> > > +	fp_g = drm_fixp_mul(matrix->matrix[1][0], fp_y) +
> > > +	       drm_fixp_mul(matrix->matrix[1][1], fp_cb) +
> > > +	       drm_fixp_mul(matrix->matrix[1][2], fp_cr);
> > > +	fp_b = drm_fixp_mul(matrix->matrix[2][0], fp_y) +
> > > +	       drm_fixp_mul(matrix->matrix[2][1], fp_cb) +
> > > +	       drm_fixp_mul(matrix->matrix[2][2], fp_cr);
> > > +
> > > +	fp_r = drm_fixp2int_round(fp_r);
> > > +	fp_g = drm_fixp2int_round(fp_g);
> > > +	fp_b = drm_fixp2int_round(fp_b);
> > > +
> > > +	r = clamp(fp_r, 0, 0xff);
> > > +	g = clamp(fp_g, 0, 0xff);
> > > +	b = clamp(fp_b, 0, 0xff);
> > > +
> > > +	return argb_u16_from_u8888(255, r, g, b);  
> > 
> > Going through argb_u16_from_u8888() will throw away precision.  
> 
> I tried to fix it in the v6, IGT tests pass. If something is wrong in the 
> v6, please let me know.
> 
> > > +}
> > > +
> > >  /*
> > >   * The following functions are read_line function for each pixel format supported by VKMS.
> > >   *
> > > @@ -293,6 +367,79 @@ static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> > >  	}
> > >  }
> > >  
> > > +/*
> > > + * This callback can be used for yuv and yvu formats, given a properly modified conversion matrix
> > > + * (column inversion)  
> > 
> > Would be nice to explain what semi_planar_yuv means, so that the
> > documentation for these functions would show how they differ rather
> > than all saying exactly the same thing.  
> 
>  /* This callback can be used for YUV format where each color component is 
>   * stored in a different plane (often called planar formats). It will 
>   * handle correctly subsampling.
> 
>  /*
>   * This callback can be used for YUV formats where U and V values are 
>   * stored in the same plane (often called semi-planar formats). It will 
>   * corectly handle subsampling.
>   * 
>   * The conversion matrix stored in the @plane is used to:
>   * - Apply the correct color range and encoding
>   * - Convert YUV and YVU with the same function (a simple column swap is 
>   *   needed)
>   */

Sounds good. I'd just drop the "It will handle correctly subsampling."
because all code is supposed to be correct by default.

If there is a function that intentionally overlooks something, that
certainly should be documented.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]