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

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

[Arnd Bergmann]

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.

Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
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

[PATCH 2/3] [v2] drm/xe/mmio: fix build warning for BAR resize on 32-bit

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
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

Re: [PATCH 2/3] [v2] drm/xe/mmio: fix build warning for BAR resize on 32-bit

[Lucas De Marchi]

On Mon, Feb 26, 2024 at 01:46:37PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <arnd@arndb.de>
>
>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 <arnd@arndb.de>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

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
>

[PATCH 3/3] [v2] drm/xe/xe2: fix 64-bit division in pte_update_size

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
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

Re: [PATCH 3/3] [v2] drm/xe/xe2: fix 64-bit division in pte_update_size

[Lucas De Marchi]

On Mon, Feb 26, 2024 at 01:46:38PM +0100, Arnd Bergmann wrote:
>From: Arnd Bergmann <arnd@arndb.de>
>
>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 <arnd@arndb.de>
>---
>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
>

Re: [PATCH 3/3] [v2] drm/xe/xe2: fix 64-bit division in pte_update_size

[Arnd Bergmann]

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 <arnd@arndb.de>
>>---
>>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

Re: [PATCH 3/3] [v2] drm/xe/xe2: fix 64-bit division in pte_update_size

[Lucas De Marchi]

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 <arnd@arndb.de>
>>>---
>>>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 <lucas.demarchi@intel.com>

Lucas De Marchi

>
>      Arnd

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

[Lucas De Marchi]

On Mon, Feb 26, 2024 at 01:46:36PM +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.
>
>Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe device")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

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
>

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

[Lucas De Marchi]

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 <lucas.demarchi@intel.com>