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

[Karolina Stolarek <karolina.stolarek@intel.com>]

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@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)? I _think_ I tried something similar, and 
couldn't get it to work, got a bunch of "twice exported" warnings.

> 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@intel.com>
>>>> Reported-by: kernel test robot <lkp@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
>>>
>