Re: [RFC v2 1/3] drm/ttm: Introduce KUnit tests

[Christian König <christian.koenig@xxxxxxx>]

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.

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