On 11.09.2023 15:42, Christian König wrote:
Am 11.09.23 um 15:33 schrieb Karolina Stolarek:
On 11.09.2023 15:12, Christian König wrote:
Am 11.09.23 um 13:47 schrieb Karolina Stolarek:
On 11.09.2023 12:49, Jani Nikula wrote:
On Mon, 11 Sep 2023, Karolina Stolarek
<karolina.stolarek@xxxxxxxxx> wrote:
Update Makefile so it can produce a module that consists of TTM
tests.
This will allow us to test non-exported functions when KUnit tests
are built as a module. Remove the tests' Makefile.
I'm asking questions instead of making assertions, because I'm not
100%
confident, but I don't feel like this Makefile could work right.
Questions, assertions and comments are fine, I'm glad you're taking
a look at the patch :) I'm not 100% confident either, so I welcome
suggestions on how to improve it.
The problem is that TTM tests try to test symbols that are not
exported, so I have to compile all the files together into one
module if I choose to build KUnit tests as a module. The other
option that I'm considering is to make the tests are builtin only.
I'm tempted to go with it (for the sake of simplicity), but I'm
trying to get the module option to work first.
I have to admit that this looks pretty awkward, but I'm not an expert
on the Linux build system in the first place.
Neither am I, and it shows :)
Would it be an option to build the unit tests into the standard
ttm.ko module and let them run whenever that module loads?
You mean appending the list of tests to ttm-y, depending on
$(CONFIG_DRM_TTM_KUNIT_TEST)?
Yes.
I _think_ I tried something similar, and couldn't get it to work, got
a bunch of "twice exported" warnings.
You might need to adjust module_init, and things like MODULE_VERSION
etc..., but I would give it a try.
I just learnt about the existence of EXPORT_SYMBOL_FOR_TESTS_ONLY()
macro. It's defined as a part of drm_util.h and used by DRM selftests.
Thankfully, the TTM KUnit KConfig already selects
CONFIG_DRM_EXPORT_FOR_TESTS, so we could use that macro (almost) for
free. Only 2-3 line additions to ttm_tt and ttm_resource files, no
significant changes to the Makefile.
What do you think?
Many thanks,
Karolina
Thanks for looking into this,
Christian.
On the other hand if this solution here works, why not?
Because it's complicated and, well, awkward. I'm still thinking about
a use case where we would prefer to have KUnit tests defined as a
module. IIRC, in CI we enable KUnit tests as bultins and run them
during the boot. kunit.py helper also defines the tests as builtins.
All the best,
Karolina
Regards,
Christian.
Signed-off-by: Karolina Stolarek <karolina.stolarek@xxxxxxxxx>
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Closes:
https://lore.kernel.org/oe-kbuild-all/202309010358.50gYLkmw-lkp@xxxxxxxxx/
Closes:
https://lore.kernel.org/oe-kbuild-all/202309011134.bwvpuyOj-lkp@xxxxxxxxx/
Closes:
https://lore.kernel.org/oe-kbuild-all/202309011935.bBpezbUQ-lkp@xxxxxxxxx/
---
drivers/gpu/drm/ttm/Makefile | 18 +++++++++++++-----
drivers/gpu/drm/ttm/tests/Makefile | 6 ------
2 files changed, 13 insertions(+), 11 deletions(-)
delete mode 100644 drivers/gpu/drm/ttm/tests/Makefile
diff --git a/drivers/gpu/drm/ttm/Makefile
b/drivers/gpu/drm/ttm/Makefile
index dad298127226..6322a33e65ed 100644
--- a/drivers/gpu/drm/ttm/Makefile
+++ b/drivers/gpu/drm/ttm/Makefile
@@ -2,10 +2,18 @@
#
# Makefile for the drm device driver. This driver provides
support for the
-ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o
ttm_module.o \
- ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o
ttm_pool.o \
- ttm_device.o ttm_sys_manager.o
+ttm := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
+ ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o
ttm_pool.o \
+ ttm_device.o ttm_sys_manager.o
+obj-$(CONFIG_DRM_TTM) += $(ttm)
Does that not lead to each object in $(ttm) becoming its own module?
Huh, yes, that is what would happen here. Not good...
ttm-$(CONFIG_AGP) += ttm_agp_backend.o
Does this not create a ttm.o with just one object, depending on
CONFIG_AGP?
I just left it as it was before my changes.
-obj-$(CONFIG_DRM_TTM) += ttm.o
-obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += tests/
+ttm-tests := tests/ttm_kunit_helpers.o tests/ttm_device_test.o \
+ tests/ttm_pool_test.o
I'd preserve the one object per line syntax. It's nicer for the
diffs in
subsequent updates.
OK, will update it in the next version (if such comes). I just
followed the style of "ttm-y".
+
+ifeq ($(CONFIG_DRM_TTM_KUNIT_TEST),m)
+ ttm-test-objs := $(ttm) $(ttm-tests)
Isn't the -objs syntax for host/userspace programs? And if not,
doesn't
it lead to objects with exported symbols being present in two places?
I saw it in use in other Makefiles, so I followed it. As for the
exported symbols, I tested both builtin and module configs, and it
was fine, but it's possible I missed something. I suspect that the
variables are first expanded, and then processed by the Makefile.
Many thanks,
Karolina
Confused.
BR,
Jani.
+ obj-m := ttm-test.o
+else
+ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += $(ttm-tests)
+endif
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
deleted file mode 100644
index ec87c4fc1ad5..000000000000
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ /dev/null
@@ -1,6 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0 AND MIT
-
-obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
- ttm_device_test.o \
- ttm_pool_test.o \
- ttm_kunit_helpers.o
