[PATCH v2 1/5] drm/ttm: Update Makefile for KUnit
Karolina Stolarek
karolina.stolarek at intel.com
Tue Sep 12 09:51:23 UTC 2023
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 at intel.com> 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 at intel.com>
>>>>>> Reported-by: kernel test robot <lkp at intel.com>
>>>>>> Closes:
>>>>>> https://lore.kernel.org/oe-kbuild-all/202309010358.50gYLkmw-lkp@intel.com/
>>>>>> Closes:
>>>>>> https://lore.kernel.org/oe-kbuild-all/202309011134.bwvpuyOj-lkp@intel.com/
>>>>>> Closes:
>>>>>> https://lore.kernel.org/oe-kbuild-all/202309011935.bBpezbUQ-lkp@intel.com/
>>>>>> ---
>>>>>> 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
>>>>>
>>>
>
More information about the dri-devel
mailing list