[PATCH v3 5/9] drm/xe/kunit: Define helper functions to allocate mock device

[Lucas De Marchi]

On Mon, Dec 11, 2023 at 09:04:20PM +0100, Michal Wajdeczko wrote:
>There will be more KUnit tests added that will require mock device.
>Define generic helper functions to avoid code duplications.
>
>Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>---
> drivers/gpu/drm/xe/Makefile                 |  3 +
> drivers/gpu/drm/xe/tests/xe_kunit_helpers.c | 85 +++++++++++++++++++++
> drivers/gpu/drm/xe/tests/xe_kunit_helpers.h | 19 +++++
> 3 files changed, 107 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/tests/xe_kunit_helpers.c
> create mode 100644 drivers/gpu/drm/xe/tests/xe_kunit_helpers.h
>
>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>index 0c2e247dc188..14857923d217 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -145,6 +145,9 @@ xe-$(CONFIG_PCI_IOV) += \
> 	xe_lmtt_2l.o \
> 	xe_lmtt_ml.o
>
>+xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
>+	tests/xe_kunit_helpers.o
>+
> # i915 Display compat #defines and #includes
> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> 	-I$(srctree)/$(src)/display/ext \
>diff --git a/drivers/gpu/drm/xe/tests/xe_kunit_helpers.c b/drivers/gpu/drm/xe/tests/xe_kunit_helpers.c
>new file mode 100644
>index 000000000000..236f24d3ca17
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/tests/xe_kunit_helpers.c
>@@ -0,0 +1,85 @@
>+// SPDX-License-Identifier: GPL-2.0 AND MIT
>+/*
>+ * Copyright © 2023 Intel Corporation
>+ */
>+
>+#include <kunit/test.h>
>+#include <kunit/static_stub.h>
>+#include <kunit/visibility.h>
>+
>+#include <drm/drm_drv.h>
>+#include <drm/drm_kunit_helpers.h>
>+
>+#include "tests/xe_kunit_helpers.h"
>+#include "tests/xe_pci_test.h"
>+#include "xe_device_types.h"
>+
>+/**
>+ * __xe_kunit_helper_alloc_xe_device - Allocate a mock &xe_device for a KUnit test

s/mock/fake/

>+ * @test: the &kunit where this &xe_device will be used
>+ * @data: the &xe_pci_fake_data with desired variant of the mock device (optional)
>+ *
>+ * This function creates a &xe_device based on parameters from @data.
>+ * This allocation is managed. See drm_kunit_helper_alloc_device() and
>+ * drm_kunit_helper_alloc_drm_device() for details.
>+ *
>+ * Return: A pointer to mock &xe_device described by &data
>+ */
>+struct xe_device *__xe_kunit_helper_alloc_xe_device(struct kunit *test,
>+						    struct xe_pci_fake_data *data)

no reason for __ here. However, my previous feedback on the approach is
below.

>+{
>+	struct xe_device *xe;
>+	struct device *dev;
>+	int err;
>+
>+	dev = drm_kunit_helper_alloc_device(test);
>+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>+
>+	xe = drm_kunit_helper_alloc_drm_device(test, dev,
>+					       struct xe_device,
>+					       drm, DRIVER_GEM);
>+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xe);

it's not a lot of code for individual tests to duplicate. We can always
add a level of indirection to solve the problems and make common code,
but then we risk having too much abstraction and people developing the
tests not really understanding what's going on.

The drm_kunit_helper here use the *compose* paradigm... there's one
function to alloc device, and one to alloc drm_device, and the test
initialization builds on that. What you are doing is *wrapping*
rather than composing. If we always have the same code every time,
then wrapping may make sense, but I don't think we have too many
users yet.

I'd take a step back, let the additional tests use their own init/fini
functions that are not abstracted more than it already is. Then when
we have more places we come back and try to consolidate.
Maybe consolidating it is not about adding one additional level
abstraction, but rather providing a xe_kunit_helper_init_device() to
compose with the drm ones.


Lucas De Marchi