| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZgLwT2Pm1DbO2vh2@localhost.localdomain>
Date: Tue, 26 Mar 2024 16:57:03 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Maíra Canal <mcanal@...lia.com>
Cc: 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>, pekka.paalanen@...oniitty.fi,
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 14/16] drm/vkms: Create KUnit tests for YUV conversions
Le 25/03/24 - 11:34, Maíra Canal a écrit :
> On 3/13/24 14:45, Louis Chauvet wrote:
> > From: Arthur Grillo <arthurgrillo@...eup.net>
> >
> > Create KUnit tests to test the conversion between YUV and RGB. Test each
> > conversion and range combination with some common colors.
> >
> > The code used to compute the expected result can be found in comment.
> >
> > Signed-off-by: Arthur Grillo <arthurgrillo@...eup.net>
> > [Louis Chauvet:
> > - fix minor formating issues (whitespace, double line)
> > - change expected alpha from 0x0000 to 0xffff
> > - adapt to the new get_conversion_matrix usage
> > - apply the changes from Arthur
> > - move struct pixel_yuv_u8 to the test itself]
>
> Again, a Co-developed-by tag might be more proper.
For this patch, my contribution was very minimal (I only add a call to
get_conversion_matrix_to_argb_u16), so I will not add the Co-developed-by.
> > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > ---
> > drivers/gpu/drm/vkms/Kconfig | 15 ++
> > drivers/gpu/drm/vkms/Makefile | 1 +
> > drivers/gpu/drm/vkms/tests/.kunitconfig | 4 +
> > drivers/gpu/drm/vkms/tests/Makefile | 3 +
> > drivers/gpu/drm/vkms/tests/vkms_format_test.c | 230 ++++++++++++++++++++++++++
> > drivers/gpu/drm/vkms/vkms_formats.c | 7 +-
> > drivers/gpu/drm/vkms/vkms_formats.h | 4 +
> > 7 files changed, 262 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> > index b9ecdebecb0b..9b0e1940c14f 100644
> > --- a/drivers/gpu/drm/vkms/Kconfig
> > +++ b/drivers/gpu/drm/vkms/Kconfig
> > @@ -13,3 +13,18 @@ config DRM_VKMS
> > a VKMS.
> >
> > If M is selected the module will be called vkms.
> > +
> > +config DRM_VKMS_KUNIT_TESTS
> > + tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
>
> "KUnit tests for VKMS"
Fixed in v6.
> > + depends on DRM_VKMS && KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + This builds unit tests for VKMS. This option is not useful for
> > + distributions or general kernels, but only for kernel
> > + developers working on VKMS.
> > +
> > + For more information on KUnit and unit tests in general,
> > + please refer to the KUnit documentation in
> > + Documentation/dev-tools/kunit/.
> > +
> > + If in doubt, say "N".
> > \ No newline at end of file
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 1b28a6a32948..8d3e46dde635 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -9,3 +9,4 @@ vkms-y := \
> > vkms_writeback.o
> >
> > obj-$(CONFIG_DRM_VKMS) += vkms.o
> > +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
> > diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig b/drivers/gpu/drm/vkms/tests/.kunitconfig
> > new file mode 100644
> > index 000000000000..70e378228cbd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
> > @@ -0,0 +1,4 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_DRM=y
> > +CONFIG_DRM_VKMS=y
> > +CONFIG_DRM_VKMS_KUNIT_TESTS=y
> > diff --git a/drivers/gpu/drm/vkms/tests/Makefile b/drivers/gpu/drm/vkms/tests/Makefile
> > new file mode 100644
> > index 000000000000..2d1df668569e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/tests/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_format_test.o
> > diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> > new file mode 100644
> > index 000000000000..0954d606e44a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
[...]
> > +/*
> > + * The YUV color representation were acquired via the colour python framework.
> > + * Below are the function calls used for generating each case.
> > + *
> > + * for more information got to the docs:
>
> s/for/For
Fixed in v6.
> > + * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> > + */
> > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > + /*
> > + * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > + * K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > + * in_bits = 16,
> > + * in_legal = False,
> > + * in_int = True,
> > + * out_bits = 8,
> > + * out_legal = False,
> > + * out_int = True)
> > + */
>
> I feel that this Python code is kind of poluting the test cases.
This python code is needed to understand where the values come from. I
think we should keep it for future reference (add more cases, test yuv 16
bits...)
Maybe we can change the array comment to
/*
* The yuv color representation were acquired via the colour python framework:
*
* colour.RGB_to_YCbCr(<rgb color in 16 bits form>,
* K=color.WEIGHTS_YCBCR["<format>"],
* [...],
* out_legal = <limited or full range>)
*
* The exact function call arguments are given for each element of this list.
*
* [...]
*/
And above each test case:
/*
* format = "ITU-R BT.601"
* out_legal = False
*/
@Arthur, do you agree with those modifications?
> > + {
> > + .encoding = DRM_COLOR_YCBCR_BT601,
> > + .range = DRM_COLOR_YCBCR_FULL_RANGE,
> > + .n_colors = 6,
> > + .colors = {
> > + { "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> > + { "gray", { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> > + { "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> > + { "red", { 0x4c, 0x55, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> > + { "green", { 0x96, 0x2c, 0x15 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> > + { "blue", { 0x1d, 0xff, 0x6b }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> > + },
> > + },
[...]
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index edbf4b321b91..863fc91d6d48 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -7,6 +7,8 @@
> > #include <drm/drm_rect.h>
> > #include <drm/drm_fixed.h>
> >
> > +#include <kunit/visibility.h>
> > +
> > #include "vkms_formats.h"
> >
> > /**
> > @@ -199,8 +201,8 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const u16 *pixel)
> > return out_pixel;
> > }
> >
> > -static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> > - struct conversion_matrix *matrix)
> > +VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> > + struct conversion_matrix *matrix)
> > {
> > u8 r, g, b;
> > s64 fp_y, fp_cb, fp_cr;
> > @@ -234,6 +236,7 @@ static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> >
> > return argb_u16_from_u8888(255, r, g, b);
> > }
> > +EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
> >
> > /*
> > * The following functions are read_line function for each pixel format supported by VKMS.
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> > index e1d324764b17..21e66a0cac16 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.h
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> > @@ -13,4 +13,8 @@ struct conversion_matrix *
> > get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
> > enum drm_color_range range);
> >
> > +#if IS_ENABLED(CONFIG_KUNIT)
>
> What about the CONFIG_DRM_EXPORT_FOR_TESTS?
As the documentation for CONFIG_DRM_EXPORT_FOR_TESTS don't exists, I don't
know what to use. Maybe Arthur knows what to do here? If needed I can
apply the modifications for the next iteration.
Thanks for all your reviews,
Louis Chauvet
[...]
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists