Le 09/04/24 - 10:58, Pekka Paalanen a écrit :
> On Mon, 8 Apr 2024 09:50:19 +0200
> Louis Chauvet <louis.chau...@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.chau...@bootlin.com> wrote:
> > >
> > > > From: Arthur Grillo <arthurgri...@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 <arthurgri...@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.chau...@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.
Thanks for the clear explaination!
> However, you have a semantic reason to keep the argument as u8, and
> that is fine.
So I will keep u8 for the v6.
> > > 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.
Will do for the v6.
Thanks,
Louis Chauvet
> If there is a function that intentionally overlooks something, that
> certainly should be documented.
>
>
> Thanks,
> pq
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
