[RFC v2 1/3] drm/ttm: Introduce KUnit tests
Karolina Stolarek
karolina.stolarek at intel.com
Fri Jun 30 12:35:53 UTC 2023
On 30.06.2023 13:18, Christian König wrote:
> Am 30.06.23 um 13:09 schrieb Karolina Stolarek:
>> Hi Christian,
>>
>> I'm taking a second look at this, and I wonder what would be the
>> benefit of combining the initialization of device and ttm_device.
>> (drm_)device can be initialized indepedently from the test params, so
>> we can utilize .init and .exit callbacks offered by KUnit[1] to
>> prepare and release the device indepedently of our setup. If we were
>> to change it so ttm_device is also initialized there, we'd have to
>> manually call init/fini in every single test. What's more, ttm_pool
>> tests don't depend on ttm_device, and I can imagine that some structs
>> can be tested in a similar way.
>>
>> Would it be fine with you to rename ttm_kunit_helper_alloc_device(),
>> but leave its definition as it is?
>
> Yeah, sure. It's perfectly up to you how to structurize that, I wasn't
> even aware that there are .init and .exit callbacks.
>
> I just found that the function name didn't match what the function was
> doing.
Right, and that was a good call, thanks a lot.
All the best,
Karolina
>
> Regards,
> Christian.
>
>>
>> All the best,
>> Karolina
>> -------------------------------------------------------------------
>> [1] -
>> https://kunit.dev/third_party/kernel/docs/api/test.html#c.kunit_suite
>>
>> On 29.06.2023 12:05, Karolina Stolarek wrote:
>>> Hi Christian,
>>>
>>> Thanks a lot for taking a look at my patches.
>>>
>>> On 29.06.2023 09:50, Christian König wrote:
>>>> Am 27.06.23 um 10:32 schrieb Karolina Stolarek:
>>>>> Add the initial version of unit tests for ttm_device struct, together
>>>>> with helper functions.
>>>>>
>>>>> Signed-off-by: Karolina Stolarek <karolina.stolarek at intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/Kconfig | 15 +++++
>>>>> drivers/gpu/drm/ttm/Makefile | 1 +
>>>>> drivers/gpu/drm/ttm/tests/.kunitconfig | 4 ++
>>>>> drivers/gpu/drm/ttm/tests/Makefile | 5 ++
>>>>> drivers/gpu/drm/ttm/tests/ttm_device_test.c | 54 +++++++++++++++++
>>>>> drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 59
>>>>> +++++++++++++++++++
>>>>> drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 29 +++++++++
>>>>> 7 files changed, 167 insertions(+)
>>>>> create mode 100644 drivers/gpu/drm/ttm/tests/.kunitconfig
>>>>> create mode 100644 drivers/gpu/drm/ttm/tests/Makefile
>>>>> create mode 100644 drivers/gpu/drm/ttm/tests/ttm_device_test.c
>>>>> create mode 100644 drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
>>>>> create mode 100644 drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
>>>>>
>>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>>>> index afb3b2f5f425..53024e44a2d5 100644
>>>>> --- a/drivers/gpu/drm/Kconfig
>>>>> +++ b/drivers/gpu/drm/Kconfig
>>>>> @@ -194,6 +194,21 @@ config DRM_TTM
>>>>> GPU memory types. Will be enabled automatically if a device
>>>>> driver
>>>>> uses it.
>>>>> +config DRM_TTM_KUNIT_TEST
>>>>> + tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS
>>>>> + default n
>>>>> + depends on DRM && KUNIT
>>>>> + select DRM_TTM
>>>>> + select DRM_EXPORT_FOR_TESTS if m
>>>>> + select DRM_KUNIT_TEST_HELPERS
>>>>> + default KUNIT_ALL_TESTS
>>>>> + help
>>>>> + Enables unit tests for TTM, a GPU memory manager
>>>>> subsystem used
>>>>> + to manage memory buffers. This option is mostly useful
>>>>> for kernel
>>>>> + developers.
>>>>> +
>>>>> + If in doubt, say "N".
>>>>> +
>>>>> config DRM_BUDDY
>>>>> tristate
>>>>> depends on DRM
>>>>> diff --git a/drivers/gpu/drm/ttm/Makefile
>>>>> b/drivers/gpu/drm/ttm/Makefile
>>>>> index f906b22959cf..dad298127226 100644
>>>>> --- a/drivers/gpu/drm/ttm/Makefile
>>>>> +++ b/drivers/gpu/drm/ttm/Makefile
>>>>> @@ -8,3 +8,4 @@ ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o
>>>>> ttm_bo_vm.o ttm_module.o \
>>>>> ttm-$(CONFIG_AGP) += ttm_agp_backend.o
>>>>> obj-$(CONFIG_DRM_TTM) += ttm.o
>>>>> +obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += tests/
>>>>> diff --git a/drivers/gpu/drm/ttm/tests/.kunitconfig
>>>>> b/drivers/gpu/drm/ttm/tests/.kunitconfig
>>>>> new file mode 100644
>>>>> index 000000000000..75fdce0cd98e
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/ttm/tests/.kunitconfig
>>>>> @@ -0,0 +1,4 @@
>>>>> +CONFIG_KUNIT=y
>>>>> +CONFIG_DRM=y
>>>>> +CONFIG_DRM_KUNIT_TEST_HELPERS=y
>>>>> +CONFIG_DRM_TTM_KUNIT_TEST=y
>>>>> diff --git a/drivers/gpu/drm/ttm/tests/Makefile
>>>>> b/drivers/gpu/drm/ttm/tests/Makefile
>>>>> new file mode 100644
>>>>> index 000000000000..7917805f37af
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/ttm/tests/Makefile
>>>>> @@ -0,0 +1,5 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0 AND MIT
>>>>> +
>>>>> +obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
>>>>> + ttm_device_test.o \
>>>>> + ttm_kunit_helpers.o
>>>>> diff --git a/drivers/gpu/drm/ttm/tests/ttm_device_test.c
>>>>> b/drivers/gpu/drm/ttm/tests/ttm_device_test.c
>>>>> new file mode 100644
>>>>> index 000000000000..08d7314b1e35
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/ttm/tests/ttm_device_test.c
>>>>> @@ -0,0 +1,54 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0 AND MIT
>>>>> +/*
>>>>> + * Copyright © 2023 Intel Corporation
>>>>> + */
>>>>> +#include <drm/ttm/ttm_resource.h>
>>>>> +#include <drm/ttm/ttm_device.h>
>>>>> +#include <drm/ttm/ttm_placement.h>
>>>>> +
>>>>> +#include "ttm_kunit_helpers.h"
>>>>> +
>>>>> +static void ttm_device_init_basic(struct kunit *test)
>>>>> +{
>>>>> + struct ttm_test_devices_priv *priv = test->priv;
>>>>> + struct ttm_device *ttm_dev;
>>>>> + struct ttm_resource_manager *ttm_sys_man;
>>>>> + int err;
>>>>> +
>>>>> + ttm_dev = kunit_kzalloc(test, sizeof(*ttm_dev), GFP_KERNEL);
>>>>> + KUNIT_ASSERT_NOT_NULL(test, ttm_dev);
>>>>> +
>>>>> + err = ttm_kunit_helper_alloc_device(test, ttm_dev, false, false);
>>>>> + KUNIT_ASSERT_EQ(test, err, 0);
>>>>> +
>>>>> + KUNIT_EXPECT_PTR_EQ(test, ttm_dev->funcs, &ttm_dev_funcs);
>>>>> + KUNIT_ASSERT_NOT_NULL(test, ttm_dev->wq);
>>>>> + KUNIT_ASSERT_NOT_NULL(test, ttm_dev->man_drv[TTM_PL_SYSTEM]);
>>>>> +
>>>>> + ttm_sys_man = &ttm_dev->sysman;
>>>>> + KUNIT_ASSERT_NOT_NULL(test, ttm_sys_man);
>>>>> + KUNIT_EXPECT_TRUE(test, ttm_sys_man->use_tt);
>>>>> + KUNIT_EXPECT_TRUE(test, ttm_sys_man->use_type);
>>>>> + KUNIT_ASSERT_NOT_NULL(test, ttm_sys_man->func);
>>>>> +
>>>>> + KUNIT_EXPECT_PTR_EQ(test, ttm_dev->dev_mapping,
>>>>> + priv->drm->anon_inode->i_mapping);
>>>>> +
>>>>> + ttm_device_fini(ttm_dev);
>>>>> +}
>>>>> +
>>>>> +static struct kunit_case ttm_device_test_cases[] = {
>>>>> + KUNIT_CASE(ttm_device_init_basic),
>>>>> + {}
>>>>> +};
>>>>> +
>>>>> +static struct kunit_suite ttm_device_test_suite = {
>>>>> + .name = "ttm_device",
>>>>> + .init = ttm_test_devices_init,
>>>>> + .exit = ttm_test_devices_fini,
>>>>> + .test_cases = ttm_device_test_cases,
>>>>> +};
>>>>> +
>>>>> +kunit_test_suites(&ttm_device_test_suite);
>>>>> +
>>>>> +MODULE_LICENSE("GPL");
>>>>> diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
>>>>> b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
>>>>> new file mode 100644
>>>>> index 000000000000..d03db0405484
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
>>>>> @@ -0,0 +1,59 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0 AND MIT
>>>>> +/*
>>>>> + * Copyright © 2023 Intel Corporation
>>>>> + */
>>>>> +#include "ttm_kunit_helpers.h"
>>>>> +
>>>>> +struct ttm_device_funcs ttm_dev_funcs = {
>>>>> +};
>>>>> +EXPORT_SYMBOL_GPL(ttm_dev_funcs);
>>>>> +
>>>>> +int ttm_kunit_helper_alloc_device(struct kunit *test,
>>>>
>>>> Since this function is only initializing the ttm device I think we
>>>> should name it ttm_kunit_helper_init_device().
>>>>
>>>> On the other hand I don't see a good reason why it can't also
>>>> allocate the device.
>>>
>>> I believe that's a leftover from times when I thought I'll reuse DRM
>>> device between the tests. No problem, I can put them into one function.
>>>
>>> All the best,
>>> Karolina
>>>
>>>>
>>>> Apart from that looks like a good start,
>>>> Christian.
>>>>
>>>>> + struct ttm_device *ttm,
>>>>> + bool use_dma_alloc,
>>>>> + bool use_dma32)
>>>>> +{
>>>>> + struct ttm_test_devices_priv *priv = test->priv;
>>>>> + struct drm_device *drm = priv->drm;
>>>>> + int err;
>>>>> +
>>>>> + err = ttm_device_init(ttm, &ttm_dev_funcs, drm->dev,
>>>>> + drm->anon_inode->i_mapping,
>>>>> + drm->vma_offset_manager,
>>>>> + use_dma_alloc, use_dma32);
>>>>> +
>>>>> + return err;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(ttm_kunit_helper_alloc_device);
>>>>> +
>>>>> +int ttm_test_devices_init(struct kunit *test)
>>>>> +{
>>>>> + struct ttm_test_devices_priv *priv;
>>>>> +
>>>>> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
>>>>> + KUNIT_ASSERT_NOT_NULL(test, priv);
>>>>> +
>>>>> + test->priv = priv;
>>>>> +
>>>>> + priv->dev = drm_kunit_helper_alloc_device(test);
>>>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
>>>>> +
>>>>> + priv->drm = __drm_kunit_helper_alloc_drm_device(test, priv->dev,
>>>>> + sizeof(*priv->drm), 0,
>>>>> + DRIVER_GEM);
>>>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(ttm_test_devices_init);
>>>>> +
>>>>> +void ttm_test_devices_fini(struct kunit *test)
>>>>> +{
>>>>> + struct ttm_test_devices_priv *priv = test->priv;
>>>>> +
>>>>> + drm_kunit_helper_free_device(test, priv->dev);
>>>>> + drm_dev_put(priv->drm);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(ttm_test_devices_fini);
>>>>> +
>>>>> +MODULE_LICENSE("GPL");
>>>>> diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
>>>>> b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
>>>>> new file mode 100644
>>>>> index 000000000000..69fb03b9c4d2
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
>>>>> @@ -0,0 +1,29 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 AND MIT */
>>>>> +/*
>>>>> + * Copyright © 2023 Intel Corporation
>>>>> + */
>>>>> +#ifndef TTM_KUNIT_HELPERS_H
>>>>> +#define TTM_KUNIT_HELPERS_H
>>>>> +
>>>>> +#include <drm/drm_drv.h>
>>>>> +#include <drm/ttm/ttm_device.h>
>>>>> +
>>>>> +#include <drm/drm_kunit_helpers.h>
>>>>> +#include <kunit/test.h>
>>>>> +
>>>>> +extern struct ttm_device_funcs ttm_dev_funcs;
>>>>> +
>>>>> +struct ttm_test_devices_priv {
>>>>> + struct drm_device *drm;
>>>>> + struct device *dev;
>>>>> +};
>>>>> +
>>>>> +int ttm_kunit_helper_alloc_device(struct kunit *test,
>>>>> + struct ttm_device *ttm,
>>>>> + bool use_dma_alloc,
>>>>> + bool use_dma32);
>>>>> +
>>>>> +int ttm_test_devices_init(struct kunit *test);
>>>>> +void ttm_test_devices_fini(struct kunit *test);
>>>>> +
>>>>> +#endif // TTM_KUNIT_HELPERS_H
>>>>
>
More information about the dri-devel
mailing list