Message-ID: <CABVgOSmntjTF3fU9NJ_qwxbOYKudjud4Sey_v8F8UFMX5KgYQA@mail.gmail.com>
Date:   Tue, 21 Jun 2022 17:38:33 +0800
From:   David Gow <davidgow@...gle.com>
To:     José Expósito <jose.exposito89@...il.com>
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        Daniel Latypov <dlatypov@...gle.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        maarten.lankhorst@...ux.intel.com,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Maíra Canal <maira.canal@....br>,
        Isabella Basso <isabbasso@...eup.net>, magalilemes00@...il.com,
        tales.aparecida@...il.com, dri-devel@...ts.freedesktop.org,
        KUnit Development <kunit-dev@...glegroups.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@...il.com> wrote:
>
> Test the conversion from XRGB8888 to RGB332.
>
> What is tested?
>
>  - Different values for the X in XRGB8888 to make sure it is ignored
>  - Different clip values: Single pixel and full and partial buffer
>  - Well known colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
>
> How to run the tests?
>
>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
>          --kconfig_add CONFIG_VIRTIO_UML=y \
>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> Suggested-by: Javier Martinez Canillas <javierm@...hat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@...e.de>
> Signed-off-by: José Expósito <jose.exposito89@...il.com>
> ---

These tests all pass properly on my system, and look good to me from a
KUnit point of view. Thanks very much.

A couple of small notes below, which you can take or leave as you
wish: they mostly focus on potential future tests.

Regardless,
Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David

>  drivers/gpu/drm/Kconfig                       |  16 ++
>  drivers/gpu/drm/Makefile                      |   1 +
>  drivers/gpu/drm/tests/.kunitconfig            |   3 +
>  drivers/gpu/drm/tests/Makefile                |   3 +
>  .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
>  5 files changed, 184 insertions(+)
>  create mode 100644 drivers/gpu/drm/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/tests/Makefile
>  create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 22e7fa48d693..6c2256e8474b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
>
>           If in doubt, say "N".
>
> +config DRM_KUNIT_TEST
> +       tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> +       depends on DRM && KUNIT=y
> +       select DRM_KMS_HELPER
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds unit tests for DRM. This option is not useful for
> +         distributions or general kernels, but only for kernel
> +         developers working on DRM and associated drivers.
> +
> +         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".
> +
>  config DRM_KMS_HELPER
>         tristate
>         depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 13ef240b3d2b..db8ffcf4e048 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  #
>
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
>
>  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
>  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
> new file mode 100644
> index 000000000000..6ec04b4c979d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_KUNIT_TEST=y
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> new file mode 100644
> index 000000000000..2c8273796d9d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> new file mode 100644
> index 000000000000..98583bf56044
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_file.h>
> +#include <drm/drm_format_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_mode.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_rect.h>
> +
> +#include "../drm_crtc_internal.h"
> +
> +#define TEST_BUF_SIZE 50
> +
> +struct xrgb8888_to_rgb332_case {
> +       const char *name;
> +       unsigned int pitch;
> +       unsigned int dst_pitch;
> +       struct drm_rect clip;
> +       const u32 xrgb8888[TEST_BUF_SIZE];
> +       const u8 expected[4 * TEST_BUF_SIZE];

Why is this 4*TEST_BUF_SIZE if there are the same number of pixels
(which in rgb332 are 8-bit, not 32-bit) as in xrgb8888. I see there's
a pitch test, which does need some extra memory, but not a full 4
times (less than double, by the looks of things). Having this be 4 *
implies (to me) that the aim is to have the same total memory
available between xrgb8888 and expected, which doesn't seem to need to
be the case. Maybe make this 2 * or similar?

Relatedly, if instead of naming this 'expected', it were named rgb332,
it'd be possible to extend this struct to add other formats expected
values, and test several formats with the same list of test inputs.
(dst_pitch would probably need to become dst_pitch_rgb332 eventually,
too). This is all something which could wait for a later patch, but is
food for thought. I'd love to see an xrgb8888_to_rgb565 test at some
point, too.


> +};
> +
> +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
> +       {
> +               .name = "single_pixel_source_buffer",
> +               .pitch = 1 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(0, 0, 1, 1),
> +               .xrgb8888 = { 0x01FF0000 },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               .name = "single_pixel_clip_rectangle",
> +               .pitch = 2 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 1, 1),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000,
> +                       0x00000000, 0x10FF0000,
> +               },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               /* Well known colors: White, black, red, green, blue, magenta,
> +                * yellow and cyan. Different values for the X in XRGB8888 to
> +                * make sure it is ignored. Partial clip area.
> +                */
> +               .name = "well_known_colors",
> +               .pitch = 4 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 2, 4),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +                       0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
> +                       0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
> +                       0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
> +                       0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
> +               },
> +               .expected = {
> +                       0xFF, 0x00,
> +                       0xE0, 0x1C,
> +                       0x03, 0xE3,
> +                       0xFC, 0x1F,
> +               },
> +       },
> +       {
> +               /* Randomly picked colors. Full buffer within the clip area. */
> +               .name = "destination_pitch",
> +               .pitch = 3 * 4,
> +               .dst_pitch = 5,
> +               .clip = DRM_RECT_INIT(0, 0, 3, 3),
> +               .xrgb8888 = {
> +                       0xA10E449C, 0xB1114D05, 0xC1A80303,
> +                       0xD16C7073, 0xA20E449C, 0xB2114D05,
> +                       0xC2A80303, 0xD26C7073, 0xA30E449C,
> +               },
> +               .expected = {
> +                       0x0A, 0x08, 0xA0, 0x00, 0x00,
> +                       0x6D, 0x0A, 0x08, 0x00, 0x00,
> +                       0xA0, 0x6D, 0x0A, 0x00, 0x00,
> +               },
> +       },
> +};
> +
> +/*
> + * conversion_buf_size - Return the destination buffer size required to convert
> + * between formats.
> + * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @clip: Clip rectangle area to convert
> + *
> + * Returns:
> + * The size of the destination buffer or negative value on error.
> + */
> +static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> +                                 const struct drm_rect *clip)
> +{
> +       const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> +
> +       if (!dst_fi)
> +               return -EINVAL;
> +
> +       if (!dst_pitch)
> +               dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> +
> +       return dst_pitch * drm_rect_height(clip);
> +}
> +
> +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
> +                                        char *desc)
> +{
> +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
> +                 xrgb8888_to_rgb332_case_desc);
> +
> +static void xrgb8888_to_rgb332_test(struct kunit *test)
> +{
> +       const struct xrgb8888_to_rgb332_case *params = test->param_value;
> +       size_t dst_size;
> +       __u8 *dst = NULL;
> +
> +       struct drm_framebuffer fb = {
> +               .format = drm_format_info(DRM_FORMAT_XRGB8888),
> +               .pitches = { params->pitch, 0, 0 },
> +       };
> +
> +       dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
> +                                      &params->clip);
> +       KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> +       dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> +
> +       drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> +                                 &fb, &params->clip);
> +       KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> +}
> +
> +static struct kunit_case drm_format_helper_test_cases[] = {
> +       KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> +                        xrgb8888_to_rgb332_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite drm_format_helper_test_suite = {
> +       .name = "drm_format_helper_test",
> +       .test_cases = drm_format_helper_test_cases,
> +};
> +
> +kunit_test_suite(drm_format_helper_test_suite);
> +
> +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("José Expósito <jose.exposito89@...il.com>");
> --
> 2.25.1
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists