On Tue, Jun 28, 2022 at 12:13 AM José Expósito <jose.exposito89@xxxxxxxxx> 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
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
