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-kconf... 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-kunitconfi...
The bit of the documentation you're looking for is https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#def... 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@...
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