On Mon, 8 Apr 2024 09:50:19 +0200
Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:
> Le 27/03/24 - 16:23, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:05 +0100
> > Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:
> >
> > > From: Arthur Grillo <arthurgrillo@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > [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@xxxxxxxxxxx>
> > > ---
> > > 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:
pgpGxvPW98DwQ.pgp
Description: OpenPGP digital signature
