From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
From: Arnd Bergmann <[email protected]>
clang complains about a nonsensical test on builds with a 32-bit phys_addr_t,
which means resizing will always fail:
drivers/gpu/drm/xe/xe_mmio.c:109:23: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
109 | root_res->start > 0x100000000ull)
| ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
Previously, BAR resize was always disallowed on 32-bit kernels, but
this apparently changed recently. Since 32-bit machines can in theory
support PAE/LPAE for large address spaces, this may end up useful,
so change the driver to shut up the warning but still work when
phys_addr_t/resource_size_t is 64 bit wide.
Fixes: 9a6e6c14bfde ("drm/xe/mmio: Use non-atomic writeq/readq variant for 32b")
Fixes: ea97a66a2218 ("drm/xe: Disable 32bits build")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/gpu/drm/xe/xe_mmio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index e3db3a178760..7ba2477452d7 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -106,7 +106,7 @@ static void xe_resize_vram_bar(struct xe_device *xe)
pci_bus_for_each_resource(root, root_res, i) {
if (root_res && root_res->flags & (IORESOURCE_MEM | IORESOURCE_MEM_64) &&
- root_res->start > 0x100000000ull)
+ (u64)root_res->start > 0x100000000ul)
break;
}
--
2.39.2
From: Arnd Bergmann <[email protected]>
This function does not build on 32-bit targets when the compiler
fails to reduce DIV_ROUND_UP() into a shift:
ld.lld: error: undefined symbol: __aeabi_uldivmod
>>> referenced by xe_migrate.c
>>> drivers/gpu/drm/xe/xe_migrate.o:(pte_update_size) in archive vmlinux.a
There are two instances in this function. Change the first to
use an open-coded shift with the same behavior, and the second
one to a 32-bit calculation, which is sufficient here as the size
is never more than 2^32 pages (16TB).
Fixes: ea97a66a2218 ("drm/xe: Disable 32bits build")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/gpu/drm/xe/xe_migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
index a66fdf2d2991..ee1bb938c493 100644
--- a/drivers/gpu/drm/xe/xe_migrate.c
+++ b/drivers/gpu/drm/xe/xe_migrate.c
@@ -462,7 +462,7 @@ static u32 pte_update_size(struct xe_migrate *m,
} else {
/* Clip L0 to available size */
u64 size = min(*L0, (u64)avail_pts * SZ_2M);
- u64 num_4k_pages = DIV_ROUND_UP(size, XE_PAGE_SIZE);
+ u32 num_4k_pages = (size + XE_PAGE_SIZE - 1) >> XE_PTE_SHIFT;
*L0 = size;
*L0_ofs = xe_migrate_vm_addr(pt_ofs, 0);
--
2.39.2
On Sat, Feb 24, 2024 at 01:14:59PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <[email protected]>
>
>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 <[email protected]>
>---
> 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
>
On Sat, Feb 24, 2024 at 01:15:00PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <[email protected]>
>
>clang complains about a nonsensical test on builds with a 32-bit phys_addr_t,
>which means resizing will always fail:
>
>drivers/gpu/drm/xe/xe_mmio.c:109:23: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> 109 | root_res->start > 0x100000000ull)
> | ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>
>Previously, BAR resize was always disallowed on 32-bit kernels, but
>this apparently changed recently. Since 32-bit machines can in theory
>support PAE/LPAE for large address spaces, this may end up useful,
>so change the driver to shut up the warning but still work when
>phys_addr_t/resource_size_t is 64 bit wide.
>
>Fixes: 9a6e6c14bfde ("drm/xe/mmio: Use non-atomic writeq/readq variant for 32b")
>Fixes: ea97a66a2218 ("drm/xe: Disable 32bits build")
this second Fixes should not be here? How would "disabling 32bits build"
break 32bits build? I think just the first one is enough, otherwise
237412e45390 ("drm/xe: Enable 32bits build") is your next good
candidate.
Acked-by: Lucas De Marchi <[email protected]>
Lucas De Marchi
>Signed-off-by: Arnd Bergmann <[email protected]>
>---
> drivers/gpu/drm/xe/xe_mmio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>index e3db3a178760..7ba2477452d7 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.c
>+++ b/drivers/gpu/drm/xe/xe_mmio.c
>@@ -106,7 +106,7 @@ static void xe_resize_vram_bar(struct xe_device *xe)
>
> pci_bus_for_each_resource(root, root_res, i) {
> if (root_res && root_res->flags & (IORESOURCE_MEM | IORESOURCE_MEM_64) &&
>- root_res->start > 0x100000000ull)
>+ (u64)root_res->start > 0x100000000ul)
> break;
> }
>
>--
>2.39.2
>
On Sat, Feb 24, 2024 at 01:15:01PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <[email protected]>
>
>This function does not build on 32-bit targets when the compiler
>fails to reduce DIV_ROUND_UP() into a shift:
>
>ld.lld: error: undefined symbol: __aeabi_uldivmod
>>>> referenced by xe_migrate.c
>>>> drivers/gpu/drm/xe/xe_migrate.o:(pte_update_size) in archive vmlinux.a
>
>There are two instances in this function. Change the first to
>use an open-coded shift with the same behavior, and the second
>one to a 32-bit calculation, which is sufficient here as the size
>is never more than 2^32 pages (16TB).
>
>Fixes: ea97a66a2218 ("drm/xe: Disable 32bits build")
same comment as in patch 2... should rather be the commit *enabling* 32b
builds?
Should this be something to be dealt with at the DIV_ROUND_UP() layer?
Why did the compiler fail to reduce it here but didn't in the other
11 uses in the xe driver?
thanks
Lucas De Marchi
>Signed-off-by: Arnd Bergmann <[email protected]>
>---
> drivers/gpu/drm/xe/xe_migrate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>index a66fdf2d2991..ee1bb938c493 100644
>--- a/drivers/gpu/drm/xe/xe_migrate.c
>+++ b/drivers/gpu/drm/xe/xe_migrate.c
>@@ -462,7 +462,7 @@ static u32 pte_update_size(struct xe_migrate *m,
> } else {
> /* Clip L0 to available size */
> u64 size = min(*L0, (u64)avail_pts * SZ_2M);
>- u64 num_4k_pages = DIV_ROUND_UP(size, XE_PAGE_SIZE);
>+ u32 num_4k_pages = (size + XE_PAGE_SIZE - 1) >> XE_PTE_SHIFT;
>
> *L0 = size;
> *L0_ofs = xe_migrate_vm_addr(pt_ofs, 0);
>--
>2.39.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 <[email protected]>
> >
> > 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
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 <[email protected]>
>>
>>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