* [RFC PATCH 0/1] KUnit tests for drm_format_helper @ 2022-05-30 10:20 José Expósito 2022-05-30 10:20 ` [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() José Expósito 0 siblings, 1 reply; 15+ messages in thread From: José Expósito @ 2022-05-30 10:20 UTC (permalink / raw) To: javierm Cc: tzimmermann, maarten.lankhorst, mripard, airlied, daniel, dri-devel, linux-kernel, José Expósito Hello everyone, Recently Javier added a new task in the ToDo list [1] to create KUnit tests for the functions present in "drm_format_helper". This patch, marked as RFC to start the conversation, includes tests for "drm_fb_xrgb8888_to_rgb332". Since the conversion functions present in "drm_format_helper" convert from XRGB8888 to another format, my plan is to create a set of XRGB8888 data to test all the available conversions. To illustrate it with an example, the code present in this patch: { .name = "Single pixel source", .pitch = 1 * 4, .dst_pitch = 0, .clip = CLIP(0, 0, 1, 1), .xrgb8888 = { 0x01FF0000 }, .expected = { 0xE0 }, } In a follow up patch, should look like: { .name = "Single pixel source", .pitch = 1 * 4, .clip = CLIP(0, 0, 1, 1), .xrgb8888 = { 0x01FF0000 }, .expected = { { .dst_format = DRM_FORMAT_RGB332, .dst_pitch = 0, .conv_func = drm_fb_xrgb8888_to_rgb332, .result = { 0xE0 }, }, { ... }, }, } 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 What is NOT tested? - NULL destination buffer: Because there is not validation in place - Out of bounds destination buffer: The size of the buffer is not checked. The conversion function could cause out of bound errors - Out of bounds source buffer: Similary to the destination buffer, the source buffer is read without checking its size - Invalid clip values: Because there is no clip validation. Example: clip out of or bigger than the source buffer - Invalid destination pitch: A dst_pitch < (clip->width * dst_pixsize) could cause issues. - "Big" source buffers. I don't know if this kind of tests could be of interest How to run the tests? My .kunitconfig: CONFIG_KUNIT=y CONFIG_DRM=y CONFIG_DRM_FORMAR_HELPER_TEST=y $ ./tools/testing/kunit/kunit.py run --arch=x86_64 Feedback? It'd be great to know your ideas about what else we could test, if you think that we should make the functions safer by checking the buffers and clip sizes or anything else. Thanks a lot, José Expósito [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=596c35b1440e José Expósito (1): drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 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 -- 2.25.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 10:20 [RFC PATCH 0/1] KUnit tests for drm_format_helper José Expósito @ 2022-05-30 10:20 ` José Expósito 2022-05-30 13:11 ` Maxime Ripard 2022-06-02 16:26 ` Javier Martinez Canillas 0 siblings, 2 replies; 15+ messages in thread From: José Expósito @ 2022-05-30 10:20 UTC (permalink / raw) To: javierm Cc: tzimmermann, maarten.lankhorst, mripard, airlied, daniel, dri-devel, linux-kernel, José Expósito 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> --- 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 + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS + depends on DRM && KUNIT=y + select DRM_KMS_HELPER + default KUNIT_ALL_TESTS + help + KUnit tests for the drm_format_helper APIs. This option is not + useful for distributions or general kernels, but only for kernel + developers working on DRM and associated drivers. + + If in doubt, say "N". + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 15fe3163f822..6a7d7655d38c 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -76,6 +76,9 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o # obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/ +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \ + drm_format_helper_test.o +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o diff --git a/drivers/gpu/drm/drm_format_helper_test.c b/drivers/gpu/drm/drm_format_helper_test.c new file mode 100644 index 000000000000..9c8a3007346c --- /dev/null +++ b/drivers/gpu/drm/drm_format_helper_test.c @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <kunit/test.h> + +#include <drm/drm_device.h> +#include <drm/drm_file.h> +#include <drm/drm_format_helper.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_framebuffer.h> +#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_mode.h> +#include <drm/drm_print.h> +#include <drm/drm_rect.h> + +#include "drm_crtc_internal.h" + +#define TEST_BUF_SIZE 50 +#define CLIP(x, y, w, h) { (x), (y), (x) + (w), (y) + (h) } + +struct xrgb8888_to_rgb332_case { + const char *name; + unsigned int pitch; + unsigned int dst_pitch; + struct drm_rect clip; + const u32 xrgb8888[TEST_BUF_SIZE]; + const u8 expected[4 * TEST_BUF_SIZE]; +}; + +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = { + { + .name = "Single pixel source", + .pitch = 1 * 4, + .dst_pitch = 0, + .clip = CLIP(0, 0, 1, 1), + .xrgb8888 = { 0x01FF0000 }, + .expected = { 0xE0 }, + }, + { + .name = "Single pixel clip", + .pitch = 2 * 4, + .dst_pitch = 0, + .clip = CLIP(1, 1, 1, 1), + .xrgb8888 = { + 0x00000000, 0x00000000, + 0x00000000, 0x10FF0000, + }, + .expected = { 0xE0 }, + }, + { + .name = "White, black, red, green, blue, magenta, yellow, cyan", + .pitch = 4 * 4, + .dst_pitch = 0, + .clip = CLIP(1, 1, 2, 4), + .xrgb8888 = { + 0x00000000, 0x00000000, 0x00000000, 0x00000000, + 0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000, + 0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000, + 0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000, + 0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000, + }, + .expected = { + 0xFF, 0x00, + 0xE0, 0x1C, + 0x03, 0xE3, + 0xFC, 0x1F, + }, + }, + { + .name = "Destination pitch", + .pitch = 3 * 4, + .dst_pitch = 5, + .clip = CLIP(0, 0, 3, 3), + .xrgb8888 = { + 0xA10E449C, 0xB1114D05, 0xC1A80303, + 0xD16C7073, 0xA20E449C, 0xB2114D05, + 0xC2A80303, 0xD26C7073, 0xA30E449C, + }, + .expected = { + 0x0A, 0x08, 0xA0, 0x00, 0x00, + 0x6D, 0x0A, 0x08, 0x00, 0x00, + 0xA0, 0x6D, 0x0A, 0x00, 0x00, + }, + }, +}; + +/** + * conversion_buf_size - Return the destination buffer size required to convert + * between formats. + * @src_format: source buffer pixel format (DRM_FORMAT_*) + * @dst_format: destination buffer pixel format (DRM_FORMAT_*) + * @dst_pitch: Number of bytes between two consecutive scanlines within dst + * @clip: Clip rectangle area to convert + * + * Returns: + * The size of the destination buffer or negative value on error. + */ +static size_t conversion_buf_size(u32 src_format, u32 dst_format, + unsigned int dst_pitch, + const struct drm_rect *clip) +{ + const struct drm_format_info *src_fi = drm_format_info(src_format); + const struct drm_format_info *dst_fi = drm_format_info(dst_format); + size_t width = drm_rect_width(clip); + size_t src_nbytes; + + if (!src_fi || !dst_fi) + return -EINVAL; + + if (dst_pitch) + width = dst_pitch; + + src_nbytes = width * drm_rect_height(clip) * src_fi->cpp[0]; + if (!src_nbytes) + return 0; + + return (src_nbytes * dst_fi->cpp[0]) / src_fi->cpp[0]; +} + +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t, + char *desc) +{ + strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE); +} + +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases, + xrgb8888_to_rgb332_case_desc); + +static void xrgb8888_to_rgb332_test(struct kunit *test) +{ + const struct xrgb8888_to_rgb332_case *params = test->param_value; + size_t dst_size; + __u8 *dst = NULL; + + struct drm_framebuffer fb = { + .format = drm_format_info(DRM_FORMAT_XRGB8888), + .pitches = { params->pitch, 0, 0 }, + }; + + dst_size = conversion_buf_size(DRM_FORMAT_XRGB8888, DRM_FORMAT_RGB332, + params->dst_pitch, ¶ms->clip); + KUNIT_ASSERT_GT(test, dst_size, 0); + + dst = kzalloc(dst_size, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst); + + drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888, + &fb, ¶ms->clip); + KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0); +} + +static struct kunit_case drm_format_helper_test_cases[] = { + KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test, + xrgb8888_to_rgb332_gen_params), + {} +}; + +static struct kunit_suite drm_format_helper_test_suite = { + .name = "drm-format-helper-test", + .test_cases = drm_format_helper_test_cases, +}; + +kunit_test_suite(drm_format_helper_test_suite); + +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 10:20 ` [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() José Expósito @ 2022-05-30 13:11 ` Maxime Ripard 2022-05-30 16:29 ` José Expósito 2022-06-02 16:26 ` Javier Martinez Canillas 1 sibling, 1 reply; 15+ messages in thread From: Maxime Ripard @ 2022-05-30 13:11 UTC (permalink / raw) To: José Expósito Cc: javierm, tzimmermann, maarten.lankhorst, airlied, daniel, dri-devel, linux-kernel Hi, On Mon, May 30, 2022 at 12:20:17PM +0200, 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> It looks mostly good to me, but I think we should Cc kunit-dev@googlegroups.com to have their feedback. > --- > 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 > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS > + depends on DRM && KUNIT=y > + select DRM_KMS_HELPER > + default KUNIT_ALL_TESTS > + help > + KUnit tests for the drm_format_helper APIs. This option is not > + useful for distributions or general kernels, but only for kernel > + developers working on DRM and associated drivers. > + > + If in doubt, say "N". > + AFAIK, kunit test cases are supposed to have a .kunitconfig too to enable the kunit tests easily. Maxime ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 13:11 ` Maxime Ripard @ 2022-05-30 16:29 ` José Expósito 2022-05-30 22:57 ` Daniel Latypov 2022-06-02 17:12 ` David Gow 0 siblings, 2 replies; 15+ messages in thread From: José Expósito @ 2022-05-30 16:29 UTC (permalink / raw) To: Maxime Ripard, kunit-dev Cc: javierm, tzimmermann, maarten.lankhorst, airlied, daniel, dri-devel, linux-kernel Hi Maxime, On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote: > > Hi, > > On Mon, May 30, 2022 at 12:20:17PM +0200, 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> > > It looks mostly good to me, but I think we should Cc > kunit-dev@googlegroups.com to have their feedback. Thanks a lot for the quick feedback. I just cc'ed kunit-dev@googlegroups.com. For anyone joining the conversation, here is the link to the patch and the cover letter with some questions: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/ > > > --- > > 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 > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS > > + depends on DRM && KUNIT=y > > + select DRM_KMS_HELPER > > + default KUNIT_ALL_TESTS > > + help > > + KUnit tests for the drm_format_helper APIs. This option is not > > + useful for distributions or general kernels, but only for kernel > > + developers working on DRM and associated drivers. > > + > > + If in doubt, say "N". > > + > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to > enable the kunit tests easily. > > Maxime A .kuniconfig example is present in the cover letter. My understanding from the docs: https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file Is that, like the .config file, the .kunitconfig file is not meant to be included in git, but I'm sure someone else will clarify this point. Thanks again, José Expósito ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 16:29 ` José Expósito @ 2022-05-30 22:57 ` Daniel Latypov 2022-05-31 18:44 ` José Expósito 2022-06-02 17:12 ` David Gow 1 sibling, 1 reply; 15+ messages in thread From: Daniel Latypov @ 2022-05-30 22:57 UTC (permalink / raw) To: José Expósito Cc: Maxime Ripard, kunit-dev, javierm, tzimmermann, maarten.lankhorst, airlied, daniel, dri-devel, linux-kernel On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote: > I just cc'ed kunit-dev@googlegroups.com. For anyone joining the > conversation, here is the link to the patch and the cover letter with > some questions: > > https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/ Thanks for the link. A few initial notes: a) normally, `select`ing other kconfigs is discouraged. It's not explicitly called out in https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries but this was the conclusion after some debate on the mailing lists earlier. b) I see `dst` is allocated with kzalloc but not freed. Should we use `kunit_kzalloc()` instead so it does get automatically freed? > > > > > > --- > > > 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 > > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS > > > + depends on DRM && KUNIT=y > > > + select DRM_KMS_HELPER From above, a) Specifically here, it'd be encouraged to instead do depends on DRM && KUNIT=y && DRM_KMS_HELPER Ideally, using a .kunitconfig file would make it so having to select DRM_KMS_HELPER manually isn't that annoying. > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to > > enable the kunit tests easily. > > > > Maxime > > A .kuniconfig example is present in the cover letter. My understanding > from the docs: > > https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file The bit of the documentation you're looking for is https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests You can create a drivers/gpu/drm/.kunitconfig file and run with $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86 The contents of that file would be just like CONFIG_KUNIT=y CONFIG_DRM=y CONFIG_DRM_KMS_HELPER=y # if no `select` CONFIG_DRM_FORMAR_HELPER_TEST=y Re "kunit test cases are supposed to have a .kunitconfig too" As I noted in the docs: This is a relatively new feature (5.12+) so we don’t have any conventions yet about on what files should be checked in versus just kept around locally. It’s up to you and your maintainer to decide if a config is useful enough to submit (and therefore have to maintain). So it's up to whatever people think works best/is worth it. I think in this case, it makes sense to add the file. > Is that, like the .config file, the .kunitconfig file is not meant to > be included in git, but I'm sure someone else will clarify this point. That bit of the docs needs to be rewritten. You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git. Context: `kunit.py` used to use the "<root>/.kunitconfig" file. Anytime you wanted to run more tests, you'd append to that file. So we agreed that no one should check in that file specifically. Now kunit.py * uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig" * has the --kunitconfig flag so you can use different files so it's no longer as relevant. Hope that helps, Daniel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 22:57 ` Daniel Latypov @ 2022-05-31 18:44 ` José Expósito 2022-05-31 20:42 ` Daniel Latypov 0 siblings, 1 reply; 15+ messages in thread From: José Expósito @ 2022-05-31 18:44 UTC (permalink / raw) To: Daniel Latypov Cc: Maxime Ripard, kunit-dev, javierm, tzimmermann, maarten.lankhorst, airlied, daniel, dri-devel, linux-kernel Hi Daniel, Thanks for looking into the patch and for your comments. On Mon, May 30, 2022 at 03:57:56PM -0700, Daniel Latypov wrote: > A few initial notes: > a) normally, `select`ing other kconfigs is discouraged. It's not > explicitly called out in > https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries > but this was the conclusion after some debate on the mailing lists > earlier. > > b) I see `dst` is allocated with kzalloc but not freed. Should we use > `kunit_kzalloc()` instead so it does get automatically freed? Ooops yes, it was in my "I'll handle that once it works" list, but I forgot to fix it, thanks for pointing it out > > > > --- > > > > 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 > > > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS > > > > + depends on DRM && KUNIT=y > > > > + select DRM_KMS_HELPER > > From above, a) > Specifically here, it'd be encouraged to instead do > depends on DRM && KUNIT=y && DRM_KMS_HELPER My first attempt was to go with: depends on KUNIT=y && DRM && DRM_KMS_HELPER However, when I try to run the tests I get this error: $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64 Regenerating .config ... Populating config with: $ make ARCH=x86_64 olddefconfig O=.kunit ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config. This is probably due to unsatisfied dependencies. Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y I wasn't able to figure out why that was happening, so I decided to use "select", which seems to solve the problem. Do you know why this could be happening? > Ideally, using a .kunitconfig file would make it so having to select > DRM_KMS_HELPER manually isn't that annoying. > > > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to > > > enable the kunit tests easily. > > > > > > Maxime > > > > A .kuniconfig example is present in the cover letter. My understanding > > from the docs: > > > > https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file > > The bit of the documentation you're looking for is > https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests > You can create a drivers/gpu/drm/.kunitconfig file and run with > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86 > > The contents of that file would be just like > CONFIG_KUNIT=y > CONFIG_DRM=y > CONFIG_DRM_KMS_HELPER=y # if no `select` > CONFIG_DRM_FORMAR_HELPER_TEST=y Noted, thanks a lot, I'll include it in the final version of the patch. By the way, I also included it in an unrelated patch, just in case you are wondering why I emailed you a random patch: https://lore.kernel.org/linux-input/20220531181246.190729-1-jose.exposito89@gmail.com/T/ Thanks a lot for your help, José Expósito > Re "kunit test cases are supposed to have a .kunitconfig too" > As I noted in the docs: > This is a relatively new feature (5.12+) so we don’t have any > conventions yet about on what files should be checked in versus just > kept around locally. It’s up to you and your maintainer to decide if a > config is useful enough to submit (and therefore have to maintain). > > So it's up to whatever people think works best/is worth it. > I think in this case, it makes sense to add the file. > > > Is that, like the .config file, the .kunitconfig file is not meant to > > be included in git, but I'm sure someone else will clarify this point. > > That bit of the docs needs to be rewritten. > You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git. > > Context: `kunit.py` used to use the "<root>/.kunitconfig" file. > Anytime you wanted to run more tests, you'd append to that file. > So we agreed that no one should check in that file specifically. > > Now kunit.py > * uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig" > * has the --kunitconfig flag so you can use different files > so it's no longer as relevant. > > Hope that helps, > Daniel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-31 18:44 ` José Expósito @ 2022-05-31 20:42 ` Daniel Latypov 0 siblings, 0 replies; 15+ messages in thread From: Daniel Latypov @ 2022-05-31 20:42 UTC (permalink / raw) To: José Expósito Cc: Maxime Ripard, kunit-dev, javierm, tzimmermann, maarten.lankhorst, airlied, daniel, dri-devel, linux-kernel On Tue, May 31, 2022 at 11:45 AM José Expósito <jose.exposito89@gmail.com> wrote: > > > > From above, a) > > Specifically here, it'd be encouraged to instead do > > depends on DRM && KUNIT=y && DRM_KMS_HELPER > > My first attempt was to go with: > > depends on KUNIT=y && DRM && DRM_KMS_HELPER > > However, when I try to run the tests I get this error: > > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64 > Regenerating .config ... > Populating config with: > $ make ARCH=x86_64 olddefconfig O=.kunit > ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config. > This is probably due to unsatisfied dependencies. > Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y > > I wasn't able to figure out why that was happening, so I decided to use > "select", which seems to solve the problem. > > Do you know why this could be happening? Ah, you should probably ignore my suggestion then. Looking at the Kconfig file, it's defined as config DRM_KMS_HELPER tristate depends on DRM help CRTC helpers for KMS drivers. Notably, the config lacks a description string. IIUC, this makes it a "hidden" kconfig option, i.e. you can't directly select it yourself in a .config, it must be selected by another kconfig option. E.g. you can try selecting it manually and see it fail via: $ ./tools/testing/kunit/kunit.py config --arch=x86_64 --kconfig_add=CONFIG_DRM=y --kconfig_add=CONFIG_DRM_KMS_HELPER=y ... Missing: CONFIG_DRM_KMS_HELPER=y So having the test select it makes the most sense, unless there's a better kconfig option to automatically select it (I have no idea, someone with knowledge of the DRM code might though). Daniel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 16:29 ` José Expósito 2022-05-30 22:57 ` Daniel Latypov @ 2022-06-02 17:12 ` David Gow 2022-06-02 17:29 ` Javier Martinez Canillas 1 sibling, 1 reply; 15+ messages in thread From: David Gow @ 2022-06-02 17:12 UTC (permalink / raw) To: José Expósito Cc: Maxime Ripard, KUnit Development, Javier Martinez Canillas, tzimmermann, maarten.lankhorst, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote: > > Hi Maxime, > > On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote: > > > > Hi, > > > > On Mon, May 30, 2022 at 12:20:17PM +0200, 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> > > > > It looks mostly good to me, but I think we should Cc > > kunit-dev@googlegroups.com to have their feedback. > > Thanks a lot for the quick feedback. > > I just cc'ed kunit-dev@googlegroups.com. For anyone joining the > conversation, here is the link to the patch and the cover letter with > some questions: > > https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/ > > > > > > --- > > > 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 > > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS > > > + depends on DRM && KUNIT=y > > > + select DRM_KMS_HELPER > > > + default KUNIT_ALL_TESTS > > > + help > > > + KUnit tests for the drm_format_helper APIs. This option is not > > > + useful for distributions or general kernels, but only for kernel > > > + developers working on DRM and associated drivers. > > > + > > > + If in doubt, say "N". > > > + > > > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to > > enable the kunit tests easily. > > > > Maxime > > A .kuniconfig example is present in the cover letter. (...) FYI: it's also possible to run these tests under UML with the extra options: CONFIG_VIRTIO_UML=y CONFIG_UML_PCI_OVER_VIRTIO=y I suspect it's probably better not to add those to your .kunitconfig, as they're UML-specific and will therefore break other architectures, but it does mean the tests can be run with, for example: ./tools/testing/kunit/kunit.py run --kunitconfig drivers/gpu/drm/.kunitconfig --kconfig_add CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y Or, without the .kunitconfig: ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y 'drm-*' Cheers, -- David ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 17:12 ` David Gow @ 2022-06-02 17:29 ` Javier Martinez Canillas 2022-06-02 17:45 ` Daniel Latypov 0 siblings, 1 reply; 15+ messages in thread From: Javier Martinez Canillas @ 2022-06-02 17:29 UTC (permalink / raw) To: David Gow, José Expósito Cc: Maxime Ripard, KUnit Development, tzimmermann, maarten.lankhorst, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List On 6/2/22 19:12, David Gow wrote: > On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote: [snip] >> >> A .kuniconfig example is present in the cover letter. (...) > > FYI: it's also possible to run these tests under UML with the extra options: > CONFIG_VIRTIO_UML=y > CONFIG_UML_PCI_OVER_VIRTIO=y > Oh, very interesting. I didn't notice before that his example had --arch=x86_64 > I suspect it's probably better not to add those to your .kunitconfig, > as they're UML-specific and will therefore break other architectures, > but it does mean the tests can be run with, for example: > > ./tools/testing/kunit/kunit.py run --kunitconfig > drivers/gpu/drm/.kunitconfig --kconfig_add CONFIG_VIRTIO_UML=y > --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y > > Or, without the .kunitconfig: > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y > --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add > CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y > 'drm-*' > I wonder if would make sense to have for example an arch/um/.kunitconfig with those symbols and maybe others and then the tests could also be run with something like: ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \ --kunitconfig=arch/um/.kunitconfig -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 17:29 ` Javier Martinez Canillas @ 2022-06-02 17:45 ` Daniel Latypov 0 siblings, 0 replies; 15+ messages in thread From: Daniel Latypov @ 2022-06-02 17:45 UTC (permalink / raw) To: Javier Martinez Canillas Cc: David Gow, José Expósito, Maxime Ripard, KUnit Development, tzimmermann, maarten.lankhorst, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List On Thu, Jun 2, 2022 at 10:29 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > > Or, without the .kunitconfig: > > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y > > --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add > > CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y > > 'drm-*' > > > > I wonder if would make sense to have for example an arch/um/.kunitconfig > with those symbols and maybe others and then the tests could also be run > with something like: > > ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \ > --kunitconfig=arch/um/.kunitconfig Yeah, this came up before. It'd be nice to have * --kunitconfig be repeatable (it isn't right now) * a "uml_pci.config" with the magic needed to set CONFIG_PCI=y on UML Another example where this would be useful, coverage on UML CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y CONFIG_GCOV=y I did prototype the changes to support this but never sent anything out since I had some concerns, namely: 1. we'd be blindly appending them, but that won't always work. Would users be ok with that? 2. people are already confused about .kunitconfig. It seems like the most confusing part to new people, especially those new to kernel development. Would adding this make them even more lost? Perhaps making the docs clearer on this would a good pre-req. 3. What conventions should there be around these partial configs? I think the idea should be more generic than just kunit. 4. --kconfig_add now makes this possible, even if in a noisier way than another --kunitconfig 5. we didn't have a good way of reporting options set to different values. https://lore.kernel.org/linux-kselftest/20220520224200.3764027-1-dlatypov@google.com/ would help by giving us an easier way to give clearer error messages. That said, I'm willing to whip up another version based on the patch in #5. I think we need a docs rewrite for #2, which I can take a stab at. But I think we'll need some bikeshedding about naming (#3). Daniel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-05-30 10:20 ` [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() José Expósito 2022-05-30 13:11 ` Maxime Ripard @ 2022-06-02 16:26 ` Javier Martinez Canillas 2022-06-02 16:53 ` Daniel Latypov 2022-06-02 17:07 ` David Gow 1 sibling, 2 replies; 15+ messages in thread From: Javier Martinez Canillas @ 2022-06-02 16:26 UTC (permalink / raw) To: José Expósito Cc: tzimmermann, maarten.lankhorst, mripard, airlied, daniel, dri-devel, linux-kernel, Daniel Latypov, kunit-dev 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. > +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN) > A comment on why this is needed would useful. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 16:26 ` Javier Martinez Canillas @ 2022-06-02 16:53 ` Daniel Latypov 2022-06-02 17:07 ` David Gow 1 sibling, 0 replies; 15+ messages in thread From: Daniel Latypov @ 2022-06-02 16:53 UTC (permalink / raw) To: Javier Martinez Canillas Cc: José Expósito, tzimmermann, maarten.lankhorst, mripard, airlied, daniel, dri-devel, linux-kernel, kunit-dev On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > > +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN) > > > > A comment on why this is needed would useful. Ah, I think that should not be necessary anymore. We added this to some tests to mitigate against compilers that didn't optimize away stack-local structs used internally in KUNIT_EXPECT*. Functions with ~30 or so EXPECTs could get flagged for excessively large stack frames. But in 5.18, I had some patches to reduce the naive stack usage from [48..88] => [8..32] bytes per EXPECT. I also have some RFC patches out to get it down to [0, 24] bytes. So going forward, this should only be necessary if you have something like 100s of EXPECTs in a single function (in which case you should also consider splitting that function up). Daniel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 16:26 ` Javier Martinez Canillas 2022-06-02 16:53 ` Daniel Latypov @ 2022-06-02 17:07 ` David Gow 2022-06-02 17:21 ` Javier Martinez Canillas 1 sibling, 1 reply; 15+ messages in thread From: David Gow @ 2022-06-02 17:07 UTC (permalink / raw) To: Javier Martinez Canillas Cc: José Expósito, tzimmermann, maarten.lankhorst, mripard, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Daniel Latypov, KUnit Development 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 17:07 ` David Gow @ 2022-06-02 17:21 ` Javier Martinez Canillas 2022-06-06 9:43 ` José Expósito 0 siblings, 1 reply; 15+ messages in thread From: Javier Martinez Canillas @ 2022-06-02 17:21 UTC (permalink / raw) To: David Gow Cc: José Expósito, tzimmermann, maarten.lankhorst, mripard, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Daniel Latypov, KUnit Development Hello David, On 6/2/22 19:07, David Gow wrote: > On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas [snip] >> >> 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. Oh, yes I didn't want to imply that this was the main reason but just pointed out that wouldn't even be needed if done that way. And it is something that we want to do anyway IMO, since as mentioned it would allow to test the static functions, which are the majority the format helpers in that file. > 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. > Yes, it can be done from the DRM_KUNIT_TEST symbol or just have it set (and any other needed DRM helper libraries tested by other suites) in the .kunitconfig file. I don't think is that important, since at the end that dependency will have to be maintained in some place. > Cheers, > -- David > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() 2022-06-02 17:21 ` Javier Martinez Canillas @ 2022-06-06 9:43 ` José Expósito 0 siblings, 0 replies; 15+ messages in thread From: José Expósito @ 2022-06-06 9:43 UTC (permalink / raw) To: Javier Martinez Canillas Cc: David Gow, tzimmermann, maarten.lankhorst, mripard, David Airlie, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Daniel Latypov, KUnit Development Hello everyone, On Thu, Jun 02, 2022 at 07:21:28PM +0200, Javier Martinez Canillas wrote: > Hello David, > > On 6/2/22 19:07, David Gow wrote: > > On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas > > [snip] > > >> > >> 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. > > Oh, yes I didn't want to imply that this was the main reason but just > pointed out that wouldn't even be needed if done that way. And it is > something that we want to do anyway IMO, since as mentioned it would > allow to test the static functions, which are the majority the format > helpers in that file. Conversion functions alway call drm_fb_xfrm()/drm_fb_xfrm_toio() and their *_line function. For example, drm_fb_xrgb8888_to_rgb332() calls drm_fb_xfrm() and drm_fb_xrgb8888_to_rgb332_line(). The current tests already check that the *_line() function works as expected. I'd like to test the high-level functions first and, if required, go into more detail in the future. The refactor is pretty easy, so I'd prefer to keep it as it is for the moment. About the other changes suggested, I applied all of them over the weekend. I'll send v1 of the patch to the mailing list including them so we have an up to date code to comment on. Thanks a lot for all of your comments and help, José Expósito ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-06-06 9:44 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-30 10:20 [RFC PATCH 0/1] KUnit tests for drm_format_helper José Expósito 2022-05-30 10:20 ` [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() José Expósito 2022-05-30 13:11 ` Maxime Ripard 2022-05-30 16:29 ` José Expósito 2022-05-30 22:57 ` Daniel Latypov 2022-05-31 18:44 ` José Expósito 2022-05-31 20:42 ` Daniel Latypov 2022-06-02 17:12 ` David Gow 2022-06-02 17:29 ` Javier Martinez Canillas 2022-06-02 17:45 ` Daniel Latypov 2022-06-02 16:26 ` Javier Martinez Canillas 2022-06-02 16:53 ` Daniel Latypov 2022-06-02 17:07 ` David Gow 2022-06-02 17:21 ` Javier Martinez Canillas 2022-06-06 9:43 ` José Expósito
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).