diff mbox series

[1/3] drm/xe/kunit: fix link failure with built-in xe

Message ID 20240224121528.1972719-1-arnd@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/xe/kunit: fix link failure with built-in xe | expand

Commit Message

Arnd Bergmann Feb. 24, 2024, 12:14 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

When the driver is built-in but the tests are in loadable modules,
the helpers don't actually get put into the driver:

ERROR: modpost: "xe_kunit_helper_alloc_xe_device" [drivers/gpu/drm/xe/tests/xe_test.ko] undefined!

Change the Makefile to ensure they are always part of the driver
even when the rest of the kunit tests are in loadable modules.

The tests/xe_kunit_helpers.c file depends on DRM_KUNIT_TEST_HELPERS,
so this has to always be selected by the main XE module now, rather
than the actual tests. In turn, the "depends on (m || (y && KUNIT=y))"
doesn't really do what it tried and can just be removed.

Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/xe/Kconfig       | 3 ++-
 drivers/gpu/drm/xe/Kconfig.debug | 1 -
 drivers/gpu/drm/xe/Makefile      | 6 ++++--
 3 files changed, 6 insertions(+), 4 deletions(-)

Comments

Lucas De Marchi Feb. 26, 2024, 3:42 a.m. UTC | #1 
On Sat, Feb 24, 2024 at 01:14:59PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <arnd@arndb.de>
>
>When the driver is built-in but the tests are in loadable modules,
>the helpers don't actually get put into the driver:
>
>ERROR: modpost: "xe_kunit_helper_alloc_xe_device" [drivers/gpu/drm/xe/tests/xe_test.ko] undefined!
>
>Change the Makefile to ensure they are always part of the driver
>even when the rest of the kunit tests are in loadable modules.
>
>The tests/xe_kunit_helpers.c file depends on DRM_KUNIT_TEST_HELPERS,
>so this has to always be selected by the main XE module now, rather
>than the actual tests. In turn, the "depends on (m || (y && KUNIT=y))"
>doesn't really do what it tried and can just be removed.

it actually did, which was to workaround issues prior to the commit you
are pointing out.  What it did  was to make sure xe.ko is m, or if it's
built-in, kunit is also built-in. Apparently the problem here is that
the xe_test.ko is missing the symbols.

See commit 08987a8b6820 ("drm/xe: Fix build with KUNIT=m").

I'm happy to remove it though if it's indeed not needed anymore.

Lucas De Marchi

>
>Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
> drivers/gpu/drm/xe/Kconfig       | 3 ++-
> drivers/gpu/drm/xe/Kconfig.debug | 1 -
> drivers/gpu/drm/xe/Makefile      | 6 ++++--
> 3 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>index 6d4428b19a4c..2948650680e1 100644
>--- a/drivers/gpu/drm/xe/Kconfig
>+++ b/drivers/gpu/drm/xe/Kconfig
>@@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> config DRM_XE
> 	tristate "Intel Xe Graphics"
>-	depends on DRM && PCI && MMU && (m || (y && KUNIT=y))
>+	depends on DRM && PCI && MMU
> 	depends on ACPI_VIDEO || !ACPI
> 	select INTERVAL_TREE
> 	# we need shmfs for the swappable backing store, and in particular
>@@ -11,6 +11,7 @@ config DRM_XE
> 	select DRM_BUDDY
> 	select DRM_EXEC
> 	select DRM_KMS_HELPER
>+	select DRM_KUNIT_TEST_HELPERS if DRM_XE_KUNIT_TEST != n
> 	select DRM_PANEL
> 	select DRM_SUBALLOC_HELPER
> 	select DRM_DISPLAY_DP_HELPER
>diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
>index 549065f57a78..df02e5d17d26 100644
>--- a/drivers/gpu/drm/xe/Kconfig.debug
>+++ b/drivers/gpu/drm/xe/Kconfig.debug
>@@ -76,7 +76,6 @@ config DRM_XE_KUNIT_TEST
> 	depends on DRM_XE && KUNIT && DEBUG_FS
> 	default KUNIT_ALL_TESTS
> 	select DRM_EXPORT_FOR_TESTS if m
>-	select DRM_KUNIT_TEST_HELPERS
> 	help
> 	  Choose this option to allow the driver to perform selftests under
> 	  the kunit framework
>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>index 4c6ffe4b2172..b596e4482a9b 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -158,8 +158,10 @@ xe-$(CONFIG_PCI_IOV) += \
> 	xe_lmtt_2l.o \
> 	xe_lmtt_ml.o
>
>-xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
>-	tests/xe_kunit_helpers.o
>+# include helpers for tests even when XE is built-in
>+ifdef CONFIG_DRM_XE_KUNIT_TEST
>+xe-y += tests/xe_kunit_helpers.o
>+endif
>
> # i915 Display compat #defines and #includes
> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
>-- 
>2.39.2
>
Maxime Ripard Feb. 26, 2024, 9:04 a.m. UTC | #2 
On Sun, Feb 25, 2024 at 09:42:06PM -0600, Lucas De Marchi wrote:
> On Sat, Feb 24, 2024 at 01:14:59PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > When the driver is built-in but the tests are in loadable modules,
> > the helpers don't actually get put into the driver:
> > 
> > ERROR: modpost: "xe_kunit_helper_alloc_xe_device" [drivers/gpu/drm/xe/tests/xe_test.ko] undefined!
> > 
> > Change the Makefile to ensure they are always part of the driver
> > even when the rest of the kunit tests are in loadable modules.
> > 
> > The tests/xe_kunit_helpers.c file depends on DRM_KUNIT_TEST_HELPERS,
> > so this has to always be selected by the main XE module now, rather
> > than the actual tests. In turn, the "depends on (m || (y && KUNIT=y))"
> > doesn't really do what it tried and can just be removed.
> 
> it actually did, which was to workaround issues prior to the commit you
> are pointing out.  What it did  was to make sure xe.ko is m, or if it's
> built-in, kunit is also built-in. Apparently the problem here is that
> the xe_test.ko is missing the symbols.
> 
> See commit 08987a8b6820 ("drm/xe: Fix build with KUNIT=m").
> 
> I'm happy to remove it though if it's indeed not needed anymore.

I think the main issue with that original patch is that it keeps the
direct include of the test files.

I assume it was done to allow kunit tests of static functions, but now
that VISIBLE_IF_KUNIT and EXPORT_SYMBOL_IF_KUNIT are a thing, you don't
have the need for that include, and you can build your tests in a module
while the code is builtin.

And the conversion should be pretty minimal.

Maxime
Arnd Bergmann Feb. 26, 2024, 12:46 p.m. UTC | #3 
On Mon, Feb 26, 2024, at 04:42, Lucas De Marchi wrote:
> On Sat, Feb 24, 2024 at 01:14:59PM +0100, Arnd Bergmann wrote:
>>From: Arnd Bergmann <arnd@arndb.de>
>>
>>When the driver is built-in but the tests are in loadable modules,
>>the helpers don't actually get put into the driver:
>>
>>ERROR: modpost: "xe_kunit_helper_alloc_xe_device" [drivers/gpu/drm/xe/tests/xe_test.ko] undefined!
>>
>>Change the Makefile to ensure they are always part of the driver
>>even when the rest of the kunit tests are in loadable modules.
>>
>>The tests/xe_kunit_helpers.c file depends on DRM_KUNIT_TEST_HELPERS,
>>so this has to always be selected by the main XE module now, rather
>>than the actual tests. In turn, the "depends on (m || (y && KUNIT=y))"
>>doesn't really do what it tried and can just be removed.
>
> it actually did, which was to workaround issues prior to the commit you
> are pointing out.  What it did  was to make sure xe.ko is m, or if it's
> built-in, kunit is also built-in. Apparently the problem here is that
> the xe_test.ko is missing the symbols.

Ah, I misunderstood the intention, as I was thrown off by the
redundant 'y &&' which sounds like it was trying to force XE
to be built-in rather than modular when Kunit is !=m.

The more common way to write this is 'depends on KUNIT || !KUNIT',
with some drivers writing it as 'depends on m || KUNIT!=m'.

I double-checked now and found that the dependency is indeed
still needed:

WARNING: unmet direct dependencies detected for DRM_KUNIT_TEST_HELPERS
  Depends on [m]: HAS_IOMEM [=y] && DRM [=y] && KUNIT [=m]
  Selected by [y]:
  - DRM_XE [=y] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && (ACPI_VIDEO [=y] || !ACPI [=y]) && DRM_XE_KUNIT_TEST [=m]!=n
  Selected by [m]:
  - DRM_KUNIT_TEST [=m] && HAS_IOMEM [=y] && DRM [=y] && KUNIT [=m] && MMU [=y]

> See commit 08987a8b6820 ("drm/xe: Fix build with KUNIT=m").
>
> I'm happy to remove it though if it's indeed not needed anymore.

Ideally the xe.ko module should not depend on anything exported
by lib/kunit, but for the moment, the tests/xe_kunit_helpers.o
file is still included in xe.ko and in turn depends on kunit.

Changing this is probably a little more complicated than my
patch, so I'll just send a v2 without the incorrect line.

     Arnd
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 6d4428b19a4c..2948650680e1 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -1,7 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_XE
 	tristate "Intel Xe Graphics"
-	depends on DRM && PCI && MMU && (m || (y && KUNIT=y))
+	depends on DRM && PCI && MMU
 	depends on ACPI_VIDEO || !ACPI
 	select INTERVAL_TREE
 	# we need shmfs for the swappable backing store, and in particular
@@ -11,6 +11,7 @@  config DRM_XE
 	select DRM_BUDDY
 	select DRM_EXEC
 	select DRM_KMS_HELPER
+	select DRM_KUNIT_TEST_HELPERS if DRM_XE_KUNIT_TEST != n
 	select DRM_PANEL
 	select DRM_SUBALLOC_HELPER
 	select DRM_DISPLAY_DP_HELPER
diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
index 549065f57a78..df02e5d17d26 100644
--- a/drivers/gpu/drm/xe/Kconfig.debug
+++ b/drivers/gpu/drm/xe/Kconfig.debug
@@ -76,7 +76,6 @@  config DRM_XE_KUNIT_TEST
 	depends on DRM_XE && KUNIT && DEBUG_FS
 	default KUNIT_ALL_TESTS
 	select DRM_EXPORT_FOR_TESTS if m
-	select DRM_KUNIT_TEST_HELPERS
 	help
 	  Choose this option to allow the driver to perform selftests under
 	  the kunit framework
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 4c6ffe4b2172..b596e4482a9b 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -158,8 +158,10 @@  xe-$(CONFIG_PCI_IOV) += \
 	xe_lmtt_2l.o \
 	xe_lmtt_ml.o
 
-xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
-	tests/xe_kunit_helpers.o
+# include helpers for tests even when XE is built-in
+ifdef CONFIG_DRM_XE_KUNIT_TEST
+xe-y += tests/xe_kunit_helpers.o
+endif
 
 # i915 Display compat #defines and #includes
 subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \