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 known colors: White, black, red, green, blue, magenta, yellow
and cyan
- Other colors: Randomly picked
- Destination pitch
How to run the tests?
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
--kconfig_add CONFIG_VIRTIO_UML=y \
--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
Suggested-by: Javier Martinez Canillas <[email protected]>
Signed-off-by: José Expósito <[email protected]>
---
RFC -> v1: https://lore.kernel.org/dri-devel/[email protected]/T/
- Add .kunitconfig (Maxime Ripard)
- Fix memory leak (Daniel Latypov)
- Make config option generic (Javier Martinez Canillas):
DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
- Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
---
drivers/gpu/drm/.kunitconfig | 3 +
drivers/gpu/drm/Kconfig | 16 +++
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
4 files changed, 187 insertions(+)
create mode 100644 drivers/gpu/drm/.kunitconfig
create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
new file mode 100644
index 000000000000..6ec04b4c979d
--- /dev/null
+++ b/drivers/gpu/drm/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e88c497fa010..3c0b1faba439 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
If in doubt, say "N".
+config DRM_KUNIT_TEST
+ tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
+ depends on DRM && KUNIT=y
+ select DRM_KMS_HELPER
+ default KUNIT_ALL_TESTS
+ help
+ This builds unit tests for DRM. This option is not useful for
+ distributions or general kernels, but only for kernel
+ developers working on DRM and associated drivers.
+
+ For more information on KUnit and unit tests in general,
+ please refer to the KUnit documentation in
+ Documentation/dev-tools/kunit/.
+
+ If in doubt, say "N".
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 15fe3163f822..6549471f09c7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
#
obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \
+ drm_format_helper_test.o
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..e9302219f3f9
--- /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 = kunit_kzalloc(test, 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
Hello José,
On 6/6/22 11:55, 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 known colors: White, black, red, green, blue, magenta, yellow
> and cyan
> - Other colors: Randomly picked
> - Destination pitch
>
> How to run the tests?
>
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> --kconfig_add CONFIG_VIRTIO_UML=y \
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> Suggested-by: Javier Martinez Canillas <[email protected]>
> Signed-off-by: José Expósito <[email protected]>
>
> ---
Thanks for addressing the issues pointed out. Patch looks good to me now.
Reviewed-by: Javier Martinez Canillas <[email protected]>
By the way, I think you should request an account [0], so that you can push
patches to drm-misc directly. Specially since AFAIU the plan is to add more
KUnit tests in future patch series.
[0]: https://www.freedesktop.org/wiki/AccountRequests/
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi,
On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
> and cyan
> - Other colors: Randomly picked
> - Destination pitch
>
> How to run the tests?
>
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> --kconfig_add CONFIG_VIRTIO_UML=y \
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
It's not clear to me why you would need VIRTIO here? The Kunit config
file should be enough to run the tests properly
Maxime
Hello Maxime,
On 6/6/22 15:42, Maxime Ripard wrote:
> Hi,
>
> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
>> and cyan
>> - Other colors: Randomly picked
>> - Destination pitch
>>
>> How to run the tests?
>>
>> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>> --kconfig_add CONFIG_VIRTIO_UML=y \
>> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> It's not clear to me why you would need VIRTIO here? The Kunit config
> file should be enough to run the tests properly
>
It's needed or otherwise KUnit will complain with:
./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
[15:47:31] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
The following works correctly but it won't use User Mode Linux:
./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
hi,
On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
> Hello Maxime,
>
> On 6/6/22 15:42, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
> >> and cyan
> >> - Other colors: Randomly picked
> >> - Destination pitch
> >>
> >> How to run the tests?
> >>
> >> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >> --kconfig_add CONFIG_VIRTIO_UML=y \
> >> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >
> > It's not clear to me why you would need VIRTIO here? The Kunit config
> > file should be enough to run the tests properly
> >
>
> It's needed or otherwise KUnit will complain with:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> [15:47:31] Configuring KUnit Kernel ...
> Regenerating .config ...
> Populating config with:
> $ make ARCH=um O=.kunit olddefconfig
> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> This is probably due to unsatisfied dependencies.
> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
>
> The following works correctly but it won't use User Mode Linux:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
But then, can't we add them to .kunitconfig?
We should avoid that gotcha entirely
Maxime
Hello Maxime,
On 6/6/22 15:52, Maxime Ripard wrote:
> hi,
>
> On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
>> Hello Maxime,
>>
>> On 6/6/22 15:42, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
>>>> and cyan
>>>> - Other colors: Randomly picked
>>>> - Destination pitch
>>>>
>>>> How to run the tests?
>>>>
>>>> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>>>> --kconfig_add CONFIG_VIRTIO_UML=y \
>>>> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>>>
>>> It's not clear to me why you would need VIRTIO here? The Kunit config
>>> file should be enough to run the tests properly
>>>
>>
>> It's needed or otherwise KUnit will complain with:
>>
>> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
>> [15:47:31] Configuring KUnit Kernel ...
>> Regenerating .config ...
>> Populating config with:
>> $ make ARCH=um O=.kunit olddefconfig
>> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
>> This is probably due to unsatisfied dependencies.
>> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
>> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
>>
>> The following works correctly but it won't use User Mode Linux:
>>
>> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
>
> But then, can't we add them to .kunitconfig?
>
That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
go there because is platform specific (AFAIU, one might want to test it on x86,
aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
The answer was that's not that simple and some agreement on how to do it is needed:
https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
On Mon, Jun 06, 2022 at 03:57:45PM +0200, Javier Martinez Canillas wrote:
> Hello Maxime,
>
> On 6/6/22 15:52, Maxime Ripard wrote:
> > hi,
> >
> > On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
> >> Hello Maxime,
> >>
> >> On 6/6/22 15:42, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
> >>>> and cyan
> >>>> - Other colors: Randomly picked
> >>>> - Destination pitch
> >>>>
> >>>> How to run the tests?
> >>>>
> >>>> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >>>> --kconfig_add CONFIG_VIRTIO_UML=y \
> >>>> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >>>
> >>> It's not clear to me why you would need VIRTIO here? The Kunit config
> >>> file should be enough to run the tests properly
> >>>
> >>
> >> It's needed or otherwise KUnit will complain with:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> >> [15:47:31] Configuring KUnit Kernel ...
> >> Regenerating .config ...
> >> Populating config with:
> >> $ make ARCH=um O=.kunit olddefconfig
> >> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> >> This is probably due to unsatisfied dependencies.
> >> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> >> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >>
> >> The following works correctly but it won't use User Mode Linux:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> >
> > But then, can't we add them to .kunitconfig?
> >
>
> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> go there because is platform specific (AFAIU, one might want to test it on x86,
> aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
>
> The answer was that's not that simple and some agreement on how to do it is needed:
>
> https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html
Thanks for pointing this out. So yeah, it's indeed not very optimal
We should probably just document it somewhere in KMS then? It doesn't
have to be in this patch series, but I have the feeling that we will end
up with that discussion a lot from people frustrated to have spent too
much time figuring it out :)
Maxime
On Mon, Jun 6, 2022 at 6:57 AM Javier Martinez Canillas
<[email protected]> wrote:
>
<snip>
> >>>> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >>>> --kconfig_add CONFIG_VIRTIO_UML=y \
> >>>> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >>>
> >>> It's not clear to me why you would need VIRTIO here? The Kunit config
> >>> file should be enough to run the tests properly
> >>>
> >>
> >> It's needed or otherwise KUnit will complain with:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> >> [15:47:31] Configuring KUnit Kernel ...
> >> Regenerating .config ...
> >> Populating config with:
> >> $ make ARCH=um O=.kunit olddefconfig
> >> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> >> This is probably due to unsatisfied dependencies.
> >> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> >> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >>
> >> The following works correctly but it won't use User Mode Linux:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> >
> > But then, can't we add them to .kunitconfig?
> >
>
> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> go there because is platform specific (AFAIU, one might want to test it on x86,
Slight correction, it was David who explicitly suggested it shouldn't
go in there.
https://lists.freedesktop.org/archives/dri-devel/2022-June/357611.html
> aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
>
> The answer was that's not that simple and some agreement on how to do it is needed:
I'm a bit more in favor of having UML-specific options in the drm
.kunitconfig file, but I agree it's a bit unclear.
If people want to easily run with --arch=x86_64 or others, then
they're indeed a liability.
Another option is to perhaps explicitly name the .kunitconfig file
something like drm/uml.kunitconfig, which doesn't solve the problem
but makes it less of a footgun.
Stepping back, I feel like perhaps a cleaner answer lies in adding a
new Kconfig option that selects CONFIG_UML_PCI_OVER_VIRTIO under UML
and just CONFIG_PCI otherwise.
But that's a bigger discussion still.
Daniel
Hello Daniel,
On 6/6/22 16:19, Daniel Latypov wrote:
[snip]
>> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
>> go there because is platform specific (AFAIU, one might want to test it on x86,
>
> Slight correction, it was David who explicitly suggested it shouldn't
> go in there.
> https://lists.freedesktop.org/archives/dri-devel/2022-June/357611.html
>
Ups, sorry for getting that wrong. I should had looked at the thread
again before writing my answer.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi!
Javier Martinez Canillas wrote:
> Hello Jos?,
>
> On 6/6/22 11:55, 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 known colors: White, black, red, green, blue, magenta, yellow
> > and cyan
> > - Other colors: Randomly picked
> > - Destination pitch
> >
> > How to run the tests?
> >
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> > --kconfig_add CONFIG_VIRTIO_UML=y \
> > --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >
> > Suggested-by: Javier Martinez Canillas <[email protected]>
> > Signed-off-by: Jos? Exp?sito <[email protected]>
> >
> > ---
>
> Thanks for addressing the issues pointed out. Patch looks good to me now.
>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
Thanks for the quick review Javier :)
Javier Martinez Canillas wrote:
> By the way, I think you should request an account [0], so that you can push
> patches to drm-misc directly. Specially since AFAIU the plan is to add more
> KUnit tests in future patch series.
>
> [0]: https://www.freedesktop.org/wiki/AccountRequests/
I'll request one, thanks for the tip.
-------
Maxime Ripard wrote:
> > > The following works correctly but it won't use User Mode Linux:
> > >
> > > ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> > >
> > > But then, can't we add them to .kunitconfig?
> > >
> >
> > That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> > go there because is platform specific (AFAIU, one might want to test it on x86,
> > aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
> >
> > The answer was that's not that simple and some agreement on how to do it is needed:
> >
> > https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html
>
> We should probably just document it somewhere in KMS then? It doesn't
> have to be in this patch series, but I have the feeling that we will end
> up with that discussion a lot from people frustrated to have spent too
> much time figuring it out :)
My understanding from Daniel's comment [1] is also that at the moment
it is not easy to support this use case, so yes, at least copy and
pasting the command in the docs should help everyone figure out how to
run the tests.
Documentation/gpu/drm-internals.rst seems like a good place to add some
information about how to run and add tests.
I'll send a patch with the docs ASAP.
Jose
[1] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/
Hi,
ading Kunit tests for the conversion helpers is pretty cool. Thanks for
doing that.
Am 06.06.22 um 11:55 schrieb 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 known colors: White, black, red, green, blue, magenta, yellow
> and cyan
> - Other colors: Randomly picked
> - Destination pitch
>
> How to run the tests?
>
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> --kconfig_add CONFIG_VIRTIO_UML=y \
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> Suggested-by: Javier Martinez Canillas <[email protected]>
> Signed-off-by: José Expósito <[email protected]>
>
> ---
>
> RFC -> v1: https://lore.kernel.org/dri-devel/[email protected]/T/
>
> - Add .kunitconfig (Maxime Ripard)
> - Fix memory leak (Daniel Latypov)
> - Make config option generic (Javier Martinez Canillas):
> DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
> - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
> ---
> drivers/gpu/drm/.kunitconfig | 3 +
> drivers/gpu/drm/Kconfig | 16 +++
> drivers/gpu/drm/Makefile | 2 +
> drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> 4 files changed, 187 insertions(+)
> create mode 100644 drivers/gpu/drm/.kunitconfig
> create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
>
> diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
> new file mode 100644
> index 000000000000..6ec04b4c979d
> --- /dev/null
> +++ b/drivers/gpu/drm/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_KUNIT_TEST=y
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..3c0b1faba439 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
>
> If in doubt, say "N".
>
> +config DRM_KUNIT_TEST
> + tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> + depends on DRM && KUNIT=y
> + select DRM_KMS_HELPER
> + default KUNIT_ALL_TESTS
> + help
> + This builds unit tests for DRM. This option is not useful for
> + distributions or general kernels, but only for kernel
> + developers working on DRM and associated drivers.
> +
> + For more information on KUnit and unit tests in general,
> + please refer to the KUnit documentation in
> + Documentation/dev-tools/kunit/.
> +
> + If in doubt, say "N".
> +
> config DRM_KMS_HELPER
> tristate
> depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 15fe3163f822..6549471f09c7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> #
>
> obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \
You already selected DRM_KMS_HELPER in Kconfig. Why do you need to list
the module here?
> + drm_format_helper_test.o
One comment about source-code organization:
There is potentially a long list of test files that will contain unit
tests. I would prefer to put the unit tests into their own subdirectory
(e.g., kunit).
>
> 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..e9302219f3f9
> --- /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) }
I have long wished for an initializer macro for drm_rect.
Please rename that CLIP macro to DRM_RECT_INIT and put it into
<drm/drm_rect.h> with docs.
> +
> +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[] = {
The names of these tests are only mildly descriptive. Maybe add a
single-line comment before each test case to describe what it does. Your
commit description has a nice list of tests, which you can copy here
almost as-is.
> + {
> + .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.
> + */
You don't need to document internal functions with formatted comments.
It will only confuse readers of the generated documentation. If you
don't want to outright remove the comment, at least remove the /** at
the top.
Best regards
Thomas
> +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 = kunit_kzalloc(test, 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]>");
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi Thomas,
Thanks a lot for your review.
On Tue, Jun 07, 2022 at 09:22:38AM +0200, Thomas Zimmermann wrote:
> Hi,
>
> ading Kunit tests for the conversion helpers is pretty cool. Thanks for
> doing that.
>
> Am 06.06.22 um 11:55 schrieb 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 known colors: White, black, red, green, blue, magenta, yellow
> > and cyan
> > - Other colors: Randomly picked
> > - Destination pitch
> >
> > How to run the tests?
> >
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> > --kconfig_add CONFIG_VIRTIO_UML=y \
> > --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >
> > Suggested-by: Javier Martinez Canillas <[email protected]>
> > Signed-off-by: Jos? Exp?sito <[email protected]>
> >
> > ---
> >
> > RFC -> v1: https://lore.kernel.org/dri-devel/[email protected]/T/
> >
> > - Add .kunitconfig (Maxime Ripard)
> > - Fix memory leak (Daniel Latypov)
> > - Make config option generic (Javier Martinez Canillas):
> > DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
> > - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
> > ---
> > drivers/gpu/drm/.kunitconfig | 3 +
> > drivers/gpu/drm/Kconfig | 16 +++
> > drivers/gpu/drm/Makefile | 2 +
> > drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > 4 files changed, 187 insertions(+)
> > create mode 100644 drivers/gpu/drm/.kunitconfig
> > create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
> > new file mode 100644
> > index 000000000000..6ec04b4c979d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/.kunitconfig
> > @@ -0,0 +1,3 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_DRM=y
> > +CONFIG_DRM_KUNIT_TEST=y
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..3c0b1faba439 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
> > If in doubt, say "N".
> > +config DRM_KUNIT_TEST
> > + tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> > + depends on DRM && KUNIT=y
> > + select DRM_KMS_HELPER
> > + default KUNIT_ALL_TESTS
> > + help
> > + This builds unit tests for DRM. This option is not useful for
> > + distributions or general kernels, but only for kernel
> > + developers working on DRM and associated drivers.
> > +
> > + For more information on KUnit and unit tests in general,
> > + please refer to the KUnit documentation in
> > + Documentation/dev-tools/kunit/.
> > +
> > + If in doubt, say "N".
> > +
> > config DRM_KMS_HELPER
> > tristate
> > depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 15fe3163f822..6549471f09c7 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> > #
> > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \
>
> You already selected DRM_KMS_HELPER in Kconfig. Why do you need to list the
> module here?
Actually, it is not required. I'll remove it in v2.
> > + drm_format_helper_test.o
>
> One comment about source-code organization:
>
> There is potentially a long list of test files that will contain unit tests.
> I would prefer to put the unit tests into their own subdirectory (e.g.,
> kunit).
It makes sense, and it'd also be more consistent with selftest tests.
> > 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..e9302219f3f9
> > --- /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) }
>
> I have long wished for an initializer macro for drm_rect.
> Please rename that CLIP macro to DRM_RECT_INIT and put it into
> <drm/drm_rect.h> with docs.
I'll include an extra patch for it on v2.
> > +
> > +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[] = {
>
> The names of these tests are only mildly descriptive. Maybe add a
> single-line comment before each test case to describe what it does. Your
> commit description has a nice list of tests, which you can copy here almost
> as-is.
Ok, written down for v2.
> > + {
> > + .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.
> > + */
>
> You don't need to document internal functions with formatted comments. It
> will only confuse readers of the generated documentation. If you don't want
> to outright remove the comment, at least remove the /** at the top.
Cool, I'll remove the extra * on v2.
Speaking about documentation, I sent a patch explaining how to run
the tests:
https://lore.kernel.org/dri-devel/[email protected]/T/
I'll send it as part of the v2 of this series.
Best wishes,
Jose
> Best regards
> Thomas
>
> > +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 = kunit_kzalloc(test, 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]>");
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Ivo Totev