[PATCH v2 1/5] drm/ttm: Update Makefile for KUnit

Tue Sep 12 09:59:16 UTC 2023

Am 12.09.23 um 11:51 schrieb Karolina Stolarek:
> 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?

Perfect, that's exactly what's needed here.

Thanks,
Christian.

>
> 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
>>>>>>
>>>>
>>