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
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 <[email protected]>
Signed-off-by: José Expósito <[email protected]>
---
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 <[email protected]>");
--
2.25.1
On Tue, May 31, 2022 at 11:45 AM José Expósito
<[email protected]> 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
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 <[email protected]>
> Signed-off-by: Jos? Exp?sito <[email protected]>
It looks mostly good to me, but I think we should Cc
[email protected] 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
On Thu, Jun 2, 2022 at 10:29 AM Javier Martinez Canillas
<[email protected]> 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/[email protected]/
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
On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<[email protected]> 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
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
On 6/2/22 19:12, David Gow wrote:
> On Mon, May 30, 2022 at 9:29 AM José Expósito <[email protected]> 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
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 <[email protected]>
> Signed-off-by: José Expósito <[email protected]>
> ---
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
On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
> > Signed-off-by: José Expósito <[email protected]>
> > ---
>
> 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
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