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.
Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: don't remove KUNIT dependency
---
drivers/gpu/drm/xe/Kconfig | 1 +
drivers/gpu/drm/xe/Kconfig.debug | 1 -
drivers/gpu/drm/xe/Makefile | 6 ++++--
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 6d4428b19a4c..c3a3b204ae5b 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -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: 237412e45390 ("drm/xe: Enable 32bits build")
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: use correct Fixes tag
---
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: 237412e45390 ("drm/xe: Enable 32bits build")
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: use correct Fixes tag
---
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 Mon, Feb 26, 2024 at 01:46:38PM +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: 237412e45390 ("drm/xe: Enable 32bits build")
>Signed-off-by: Arnd Bergmann <[email protected]>
>---
>v2: use correct Fixes tag
but what about the other comment? How are we supposed to use
DIV_ROUND_UP() but then in some places (which?) have to open code it?
What compiler does this fail on?
>---
> 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;
also the commit message doesn't seem to match the patch as you are only
changing one instance.
Lucas De Marchi
>
> *L0 = size;
> *L0_ofs = xe_migrate_vm_addr(pt_ofs, 0);
>--
>2.39.2
>
On Mon, Feb 26, 2024, at 17:40, Lucas De Marchi wrote:
> On Mon, Feb 26, 2024 at 01:46:38PM +0100, Arnd Bergmann wrote:
>>
>>Fixes: 237412e45390 ("drm/xe: Enable 32bits build")
>>Signed-off-by: Arnd Bergmann <[email protected]>
>>---
>>v2: use correct Fixes tag
>
> but what about the other comment? How are we supposed to use
> DIV_ROUND_UP() but then in some places (which?) have to open code it?
The problem is not DIV_ROUND_UP() but the division but the 64-bit
division itself. There is a DIV_ROUND_UP_ULL() macro that would
address the build failure as well, but doing the shift is much
more efficient here since it can be done in a couple of instructions.
> What compiler does this fail on?
I saw it with clang-19 on 32-bit arm, but I assume it happens
on others as well.
>> 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;
>
> also the commit message doesn't seem to match the patch as you are only
> changing one instance.
Not sure what you mean. As I wrote in the changelog, the
second instance is fixed by using a 32-bit division here,
which does not cause link failures.
Arnd
On Wed, Feb 28, 2024 at 01:26:29PM +0100, Arnd Bergmann wrote:
>On Mon, Feb 26, 2024, at 17:40, Lucas De Marchi wrote:
>> On Mon, Feb 26, 2024 at 01:46:38PM +0100, Arnd Bergmann wrote:
>>>
>>>Fixes: 237412e45390 ("drm/xe: Enable 32bits build")
>>>Signed-off-by: Arnd Bergmann <[email protected]>
>>>---
>>>v2: use correct Fixes tag
>>
>> but what about the other comment? How are we supposed to use
>> DIV_ROUND_UP() but then in some places (which?) have to open code it?
>
>The problem is not DIV_ROUND_UP() but the division but the 64-bit
>division itself. There is a DIV_ROUND_UP_ULL() macro that would
>address the build failure as well, but doing the shift is much
>more efficient here since it can be done in a couple of instructions.
>
>> What compiler does this fail on?
>
>I saw it with clang-19 on 32-bit arm, but I assume it happens
>on others as well.
somehow it passed on x86 :-/
>
>>> 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;
>>
>> also the commit message doesn't seem to match the patch as you are only
>> changing one instance.
>
>Not sure what you mean. As I wrote in the changelog, the
>second instance is fixed by using a 32-bit division here,
>which does not cause link failures.
I missed the type conversion to u32 and was thinking there was another
hunk missing for the second change.
all looks good to me now and I will apply later today to drm-xe-next.
Thanks.
Reviewed-by: Lucas De Marchi <[email protected]>
Lucas De Marchi
>
> Arnd
On Mon, Feb 26, 2024 at 01:46:37PM +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: 237412e45390 ("drm/xe: Enable 32bits build")
>Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Lucas De Marchi <[email protected]>
Lucas De Marchi
>---
>v2: use correct Fixes tag
>---
> 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 Mon, Feb 26, 2024 at 01:46:36PM +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.
>
>Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
>Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Lucas De Marchi <[email protected]>
thanks
Lucas De Marchi
>---
>v2: don't remove KUNIT dependency
>---
> drivers/gpu/drm/xe/Kconfig | 1 +
> drivers/gpu/drm/xe/Kconfig.debug | 1 -
> drivers/gpu/drm/xe/Makefile | 6 ++++--
> 3 files changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>index 6d4428b19a4c..c3a3b204ae5b 100644
>--- a/drivers/gpu/drm/xe/Kconfig
>+++ b/drivers/gpu/drm/xe/Kconfig
>@@ -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 Mon, 26 Feb 2024 13:46:36 +0100, Arnd Bergmann wrote:
> 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.
>
> [...]
All 3 patches applied to drm-xe-next. Thanks!
[1/3] drm/xe/kunit: fix link failure with built-in xe
commit: 0e6fec6da25167a568fbaeb8401d8172069124ad
[2/3] drm/xe/mmio: fix build warning for BAR resize on 32-bit
commit: f5d3983366c0b88ec388b3407b29c1c0862ee2b8
[3/3] drm/xe/xe2: fix 64-bit division in pte_update_size
commit: 1408784b599927d2f361bac6dc5170d2ee275f17
Best regards,
--
Lucas De Marchi <[email protected]>