drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations

When compile-testing for something other than ARCH_QCOM, we run into a link error:

drivers/gpu/drm/msm/adreno/a5xx_gpu.o: In function `a5xx_hw_init':
a5xx_gpu.c:(.text.a5xx_hw_init+0x600): undefined reference to qcom_mdt_get_size' a5xx_gpu.c:(.text.a5xx_hw_init+0x93c): undefined reference to qcom_mdt_load'

There is already an #ifdef that tries to check for CONFIG_QCOM_MDT_LOADER, but that symbol is only meaningful when building for ARCH_QCOM.

This adds a compile-time check for ARCH_QCOM, and clarifies the Kconfig select statement so we don't even try it for other targets.

The check for CONFIG_QCOM_MDT_LOADER can then go away, which also improves compile-time coverage and makes the code a little nicer to read.

Fixes 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX")

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/gpu/drm/msm/Kconfig           |  2 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +++--------
 2 files changed, 4 insertions(+), 9 deletions(-)

drivers/gpu/drm/msm/Kconfig +1 -1

index b638d192ce5e..99d39b2aefa6 100644
@@ -5,7 +5,7 @@ config DRM_MSM
 	depends on ARCH_QCOM || (ARM && COMPILE_TEST)
 	depends on OF && COMMON_CLK
 	depends on MMU
-	select QCOM_MDT_LOADER
+	select QCOM_MDT_LOADER if ARCH_QCOM
 	select REGULATOR
 	select DRM_KMS_HELPER
 	select DRM_PANEL

drivers/gpu/drm/msm/adreno/a5xx_gpu.c +3 -8

index eee9ac81aaa1..b719a5b5b88a 100644
@@ -26,8 +26,6 @@ static void a5xx_dump(struct msm_gpu *gpu);
 
 #define GPU_PAS_ID 13
 
-#if IS_ENABLED(CONFIG_QCOM_MDT_LOADER)
-
 static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 {
 	const struct firmware *fw;

@@ -36,6 +34,9 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 	void *mem_region = NULL;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_ARCH_QCOM))
+		return -EINVAL;
+
 	/* Request the MDT file for the firmware */
 	ret = request_firmware(&fw, fwname, dev);
 	if (ret) {

@@ -75,12 +76,6 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 
 	return ret;
 }
-#else
-static int zap_shader_load_mdt(struct device *dev, const char *fwname)
-{
-	return -ENODEV;
-}
-#endif
 
 static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 	struct msm_file_private *ctx)

Signature · 6 lines

2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

When compile-testing for something other than ARCH_QCOM, we run into a link error:

drivers/gpu/drm/msm/adreno/a5xx_gpu.o: In function `a5xx_hw_init':
a5xx_gpu.c:(.text.a5xx_hw_init+0x600): undefined reference to qcom_mdt_get_size' a5xx_gpu.c:(.text.a5xx_hw_init+0x93c): undefined reference to qcom_mdt_load'

There is already an #ifdef that tries to check for CONFIG_QCOM_MDT_LOADER, but that symbol is only meaningful when building for ARCH_QCOM.

This adds a compile-time check for ARCH_QCOM, and clarifies the Kconfig select statement so we don't even try it for other targets.

The check for CONFIG_QCOM_MDT_LOADER can then go away, which also improves compile-time coverage and makes the code a little nicer to read.

In zap_shader_load_mdt(), we pass a pointer to a phys_addr_t into dmam_alloc_coherent, which the compiler warns about:

drivers/gpu/drm/msm/adreno/a5xx_gpu.c: In function 'zap_shader_load_mdt':
drivers/gpu/drm/msm/adreno/a5xx_gpu.c:54:50: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]

The returned DMA address is later passed on to a function that takes a phys_addr_t, so it's clearly wrong to use the DMA mapping interface here: the memory may be uncached, or the address may be completely wrong if there is an IOMMU connected to the device.

My interpretation is that using dmam_alloc_coherent() had two purposes:

a) get a chunk of consecutive memory that may be larger than the limit for kmalloc()

b) use the devres infrastructure to simplify the unwinding in the error case.

I think ideally we'd use a devres-based version of alloc_pages_exact() here, but since that doesn't exist, let's use devm_get_free_pages() instead. This wastes a little memory as the size gets rounded up to a power of two, but is otherwise harmless. If we want to save memory here, calling devm_free_pages() to release the memory once it is no longer needed is probably better anyway.

Fixes 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX")

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/a5xx_gpu.c +3 -1

index b4b54f1c24bc..eee9ac81aaa1 100644
@@ -51,11 +51,13 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 	}
 
 	/* Allocate memory for the firmware image */
-	mem_region = dmam_alloc_coherent(dev, mem_size, &mem_phys, GFP_KERNEL);
+	mem_region = (void *)devm_get_free_pages(dev, GFP_KERNEL,
+			get_order(mem_size));
 	if (!mem_region) {
 		ret = -ENOMEM;
 		goto out;
 	}
+	mem_phys = virt_to_phys(mem_region);
 
 	/* Load the rest of the MDT */
 	ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys,

Signature · 6 lines

2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

In zap_shader_load_mdt(), we pass a pointer to a phys_addr_t into dmam_alloc_coherent, which the compiler warns about:

drivers/gpu/drm/msm/adreno/a5xx_gpu.c: In function 'zap_shader_load_mdt':
drivers/gpu/drm/msm/adreno/a5xx_gpu.c:54:50: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]

The returned DMA address is later passed on to a function that takes a phys_addr_t, so it's clearly wrong to use the DMA mapping interface here: the memory may be uncached, or the address may be completely wrong if there is an IOMMU connected to the device.

My interpretation is that using dmam_alloc_coherent() had two purposes:

a) get a chunk of consecutive memory that may be larger than the limit for kmalloc()

b) use the devres infrastructure to simplify the unwinding in the error case.

I think ideally we'd use a devres-based version of alloc_pages_exact() here, but since that doesn't exist, let's use devm_get_free_pages() instead. This wastes a little memory as the size gets rounded up to a power of two, but is otherwise harmless. If we want to save memory here, calling devm_free_pages() to release the memory once it is no longer needed is probably better anyway.

Fixes 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX")

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/a5xx_gpu.c +3 -1

index b4b54f1c24bc..eee9ac81aaa1 100644
@@ -51,11 +51,13 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 	}
 
 	/* Allocate memory for the firmware image */
-	mem_region = dmam_alloc_coherent(dev, mem_size, &mem_phys, GFP_KERNEL);
+	mem_region = (void *)devm_get_free_pages(dev, GFP_KERNEL,
+			get_order(mem_size));
 	if (!mem_region) {
 		ret = -ENOMEM;
 		goto out;
 	}
+	mem_phys = virt_to_phys(mem_region);
 
 	/* Load the rest of the MDT */
 	ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys,

Signature · 1 line

2.9.0