Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas javierm@redhat.com wrote:

Hello José,

On 5/30/22 12:20, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow and cyan
• Other colors: Randomly picked
• Destination pitch

Suggested-by: Javier Martinez Canillas javierm@redhat.com Signed-off-by: José Expósito jose.exposito89@gmail.com

---

Thanks a lot for working on this! It's very cool to see the first KUnit tests added to DRM :)

...

drivers/gpu/drm/Kconfig | 12 ++ drivers/gpu/drm/Makefile | 3 + drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++ 3 files changed, 181 insertions(+) create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index e88c497fa010..d92be6faef15 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -76,6 +76,18 @@ config DRM_KMS_HELPER help CRTC helpers for KMS drivers.

+config DRM_FORMAR_HELPER_TEST

I wonder if we want this level of detail for the test Kconfig symbols. Maybe we could just have a DRM_KUNIT_TEST symbol that will enable all the KUnit test suites for DRM ?

...

• bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
  

• depends on DRM && KUNIT=y
  

• select DRM_KMS_HELPER
  

Daniel didn't like this `select DRM_KMS_HELPER` and I think that we can avoid it if instead drm_format_helper_test.c is included in drm_format_helper.c, i.e:

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index a3ccd8bc966f..d63e02da528f 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -692,3 +692,7 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd kfree(src32); } EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);

+#ifdef DRM_KUNIT_TEST +#include "drm_format_helper_test.c" +#endif

This also has the advantage that will allow to have KUnit tests for the static functions that are only available in the drm_format_helper.c compilation unit.

...

obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/ +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \

•         drm_format_helper_test.o
  

And doing that will also allow you to get rid of this, since just selecting CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.

This is definitely something other KUnit tests (apparmor, elf, etc) are doing, and it's generally fine. You do lose the ability to build the tests as a separate module, though. (This is not usually a big problem, but there are some cases where it's useful.)

That being said, I don't think 'select' is enough of a problem that you should feel the need to refactor in this way just to avoid it. Given most of the other DRM drivers (as well as DRM_DEBUG_SELFTEST) are select-ing DRM_KMS_HELPER, it seems like a sensible enough thing to continue doing for the KUnit test. As Daniel pointed out, as a hidden option it was clearly always meant to be select-ed anyway.

Cheers, -- David