| Message ID | 20220530102017.471865-2-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KUnit tests for drm_format_helper | expand |
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
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
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
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
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
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.
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
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
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
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 >
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
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
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
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>");
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