linux-kernel - Re: [PATCH 0/4] KUnit tests for RGB565 conversion

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSmC6WJxMo58VH0WSD7wdLn7_TmTktm=6txrn0ECXCNJZQ@mail.gmail.com>
Date:   Wed, 29 Jun 2022 15:27:44 +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>,
        Maxime Ripard <mripard@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        maarten.lankhorst@...ux.intel.com,
        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 0/4] KUnit tests for RGB565 conversion

On Tue, Jun 28, 2022 at 12:13 AM José Expósito
<jose.exposito89@...il.com> wrote:
>
> Hello everyone,
>
> This series is a follow up of the XRGB8888 to RGB332 conversion KUnit tests.
>
> The first 3 patches refactor the existing test to make them agnostic of the target format and add support for "swab".
>
> The last patch adds the RGB565 conversion values, and shows how more formats will be easily added in the future.
>
> Thank you very much in advance for your feedback,
> José Expósito
>
> José Expósito (4):
>   drm/format-helper: Rename test cases to make them more generic
>   drm/format-helper: Transform tests to be agnostic of target format
>   drm/format-helper: Add support for conversion functions with swab
>   drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()
>
>  .../gpu/drm/tests/drm_format_helper_test.c    | 231 +++++++++++++++---
>  1 file changed, 196 insertions(+), 35 deletions(-)
>
>
> base-commit: 6fde8eec71796f3534f0c274066862829813b21f
> prerequisite-patch-id: 8a16f4c8004d6161035eaea275c8eafaa0ac927e
> prerequisite-patch-id: 53fded2a49e6212b546db76ec52563a683752e65
> prerequisite-patch-id: 294b0ca27a6ee57096c8f097c0572336b8a2d583
> prerequisite-patch-id: 5e05bfc5287d16c207bfc616b2776ad72eb4ab29
> prerequisite-patch-id: e94560be85dffb62a5b3cf58d1f0fc3d278ad806
> prerequisite-patch-id: a471df39c7b32c69dd2b138a7d0af015ea42e00a
> --
> 2.25.1
>

These look pretty good overall to me, but there is one big issue
(which is actually with the previous series -- oops!), and a few small
stylistic thoughts.

For the big issue: these tests don't work on big-endian systems. The
'swab' bit in this series reminded me to check, and sure enough, all
of the tests fail (including the rgb332 ones).

I tested it on PowerPC with:
 ./tools/testing/kunit/kunit.py run
--kunitconfig=drivers/gpu/drm/tests --arch=powerpc
--cross_compile=powerpc64-linux-gnu-

So that's something which needs to be fixed.

The smaller stylistic thoughts basically all revolve around the
complexity of convert_xrgb8888_cases: there are arrays of structs with
arrays of unions of structs (with function pointers in them). This
isn't a problem: it's actually a lot more readable than that
description implies, but there are other ways it could be tackled
(which have their own tradeoffs, of course).

One possibility would be to split up the test into a separate test per
destination format. They could reuse the convert_xrgb8888_cases array,
and just each access a different result. You could make things even
clearer (IMO) by replacing the results[] array with a separate, named,
member (since you don't need to iterate over them any more), and
remove the need to have the function pointer and swab union/members by
just hardcoding those in the separate test functions. It'd also make
the results a bit clearer, as each destination format would get its
own separate set of results.

Of course, that's just an idea: I don't actually have a problem with
the existing design either (other than the endianness issue, of
course).

Cheers,
-- David

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