On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas <javierm@xxxxxxxxxx> 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@xxxxxxxxxx> > > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx> > > --- > > 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
