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

[Christian König]

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.

Would it be an option to build the unit tests into the standard ttm.ko 
module and let them run whenever that module loads?

On the other hand if this solution here works, why not?

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