| Message ID | 20220627161132.33256-1-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KUnit tests for RGB565 conversion | expand |
Em seg., 27 de jun. de 2022 às 13:13, José Expósito <jose.exposito89@gmail.com> escreveu: > > 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 Tested with "./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests --arch=x86_64", "... --arch=i386" and baremetal on x86_64 to be sure; everything looks fine, but I feel like some patches could be squashed, though. Tested-by: Tales L. Aparecida <tales.aparecida@gmail.com> Inspiring work, José, keep it up! Best regards, Tales
On Tue, Jun 28, 2022 at 12:13 AM José Expósito <jose.exposito89@gmail.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
Hi Am 27.06.22 um 18:11 schrieb José Expósito: > 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. Thanks for your patches. I had one comment for the final one. With this fixed, you can add Acked-by: Thomas Zimmermann <tzimmermann@suse.de> to the series. Best regards Thomas > > 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
Hi David, Sorry for not getting back to you sooner, I've been swamped with work this week. On Wed, Jun 29, 2022 at 03:27:44PM +0800, David Gow wrote: > 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. Oops, yes, definitely something that I need to fix! I'll include an extra patch at the beginning of v2 fixing this bug. > 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). I like from your approach that the output of the tests would be easier to understand. At the moment, if a test fails, there is not enough context to know which target format failed. I'll explore this approach and see how it looks like. Thanks, Jose > Cheers, > -- David