* [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations
@ 2017-06-20 20:16 ` Arnd Bergmann
0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2017-06-20 20:16 UTC (permalink / raw)
To: Rob Clark
Cc: Arnd Bergmann, linux-arm-msm, linux-kernel, dri-devel,
Bjorn Andersson, Sushmita Susheelendra, freedreno
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 <arnd@arndb.de>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index b4b54f1c24bc..eee9ac81aaa1 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -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,
--
2.9.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations
@ 2017-06-20 20:16 ` Arnd Bergmann
0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2017-06-20 20:16 UTC (permalink / raw)
To: Rob Clark
Cc: Jordan Crouse, Bjorn Andersson, Arnd Bergmann, David Airlie,
Eric Anholt, Sushmita Susheelendra, linux-arm-msm, dri-devel,
freedreno, linux-kernel
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 <arnd@arndb.de>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index b4b54f1c24bc..eee9ac81aaa1 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -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,
--
2.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM
2017-06-20 20:16 ` Arnd Bergmann
(?)
@ 2017-06-20 20:16 ` Arnd Bergmann
[not found] ` <20170620201720.225593-2-arnd-r2nGTMty4D4@public.gmane.org>
-1 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2017-06-20 20:16 UTC (permalink / raw)
To: Rob Clark
Cc: Jordan Crouse, Bjorn Andersson, Arnd Bergmann, David Airlie,
Srinivas Kandagatla, Daniel Vetter, Archit Taneja, Eric Anholt,
Sushmita Susheelendra, linux-arm-msm, dri-devel, freedreno,
linux-kernel
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 <arnd@arndb.de>
---
drivers/gpu/drm/msm/Kconfig | 2 +-
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +++--------
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index b638d192ce5e..99d39b2aefa6 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -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
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index eee9ac81aaa1..b719a5b5b88a 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -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)
--
2.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations
2017-06-20 20:16 ` Arnd Bergmann
(?)
(?)
@ 2017-06-21 14:53 ` Jordan Crouse
-1 siblings, 0 replies; 10+ messages in thread
From: Jordan Crouse @ 2017-06-21 14:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Clark, Bjorn Andersson, David Airlie, Eric Anholt,
Sushmita Susheelendra, linux-arm-msm, dri-devel, freedreno,
linux-kernel
On Tue, Jun 20, 2017 at 10:16:50PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
Seems reasonable to me if Bjorn agrees.
Acked-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index b4b54f1c24bc..eee9ac81aaa1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -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,
> --
> 2.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM
2017-06-20 20:16 ` [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM Arnd Bergmann
@ 2017-06-21 14:58 ` Jordan Crouse
0 siblings, 0 replies; 10+ messages in thread
From: Jordan Crouse @ 2017-06-21 14:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Archit Taneja, David Airlie, Daniel Vetter,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Bjorn Andersson,
Sushmita Susheelendra, Eric Anholt, Rob Clark,
Srinivas Kandagatla, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Tue, Jun 20, 2017 at 10:16:51PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
(ARM && COMPILE_TEST) really gives the dependency checks a run for their money.
I assume Bjorn is cool with this.
Acked-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
> drivers/gpu/drm/msm/Kconfig | 2 +-
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +++--------
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index b638d192ce5e..99d39b2aefa6 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -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
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index eee9ac81aaa1..b719a5b5b88a 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -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)
> --
> 2.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM
@ 2017-06-21 14:58 ` Jordan Crouse
0 siblings, 0 replies; 10+ messages in thread
From: Jordan Crouse @ 2017-06-21 14:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Clark, Bjorn Andersson, David Airlie, Srinivas Kandagatla,
Daniel Vetter, Archit Taneja, Eric Anholt, Sushmita Susheelendra,
linux-arm-msm, dri-devel, freedreno, linux-kernel
On Tue, Jun 20, 2017 at 10:16:51PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
(ARM && COMPILE_TEST) really gives the dependency checks a run for their money.
I assume Bjorn is cool with this.
Acked-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
> drivers/gpu/drm/msm/Kconfig | 2 +-
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +++--------
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index b638d192ce5e..99d39b2aefa6 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -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
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index eee9ac81aaa1..b719a5b5b88a 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -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)
> --
> 2.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations
2017-06-20 20:16 ` Arnd Bergmann
@ 2017-06-29 5:06 ` Bjorn Andersson
-1 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2017-06-29 5:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Airlie, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
Jordan Crouse, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Sushmita Susheelendra,
Eric Anholt, Rob Clark,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Tue 20 Jun 13:16 PDT 2017, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index b4b54f1c24bc..eee9ac81aaa1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -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,
> --
> 2.9.0
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations
@ 2017-06-29 5:06 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2017-06-29 5:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Clark, Jordan Crouse, David Airlie, Eric Anholt,
Sushmita Susheelendra, linux-arm-msm, dri-devel, freedreno,
linux-kernel
On Tue 20 Jun 13:16 PDT 2017, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index b4b54f1c24bc..eee9ac81aaa1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -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,
> --
> 2.9.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM
2017-06-20 20:16 ` [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM Arnd Bergmann
@ 2017-06-29 5:11 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2017-06-29 5:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Archit Taneja, David Airlie, Daniel Vetter, Jordan Crouse,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Sushmita Susheelendra,
Eric Anholt, Rob Clark, Srinivas Kandagatla,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Tue 20 Jun 13:16 PDT 2017, Arnd Bergmann wrote:
> 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.
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM
@ 2017-06-29 5:11 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2017-06-29 5:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Clark, Jordan Crouse, David Airlie, Srinivas Kandagatla,
Daniel Vetter, Archit Taneja, Eric Anholt, Sushmita Susheelendra,
linux-arm-msm, dri-devel, freedreno, linux-kernel
On Tue 20 Jun 13:16 PDT 2017, Arnd Bergmann wrote:
> 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.
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-06-29 5:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-20 20:16 [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann
2017-06-20 20:16 ` Arnd Bergmann
2017-06-20 20:16 ` [PATCH 2/2] drm/msm: gpu: call qcom_mdt interfaces only for ARCH_QCOM Arnd Bergmann
[not found] ` <20170620201720.225593-2-arnd-r2nGTMty4D4@public.gmane.org>
2017-06-21 14:58 ` Jordan Crouse
2017-06-21 14:58 ` Jordan Crouse
2017-06-29 5:11 ` Bjorn Andersson
2017-06-29 5:11 ` Bjorn Andersson
2017-06-21 14:53 ` [PATCH 1/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations Jordan Crouse
[not found] ` <20170620201720.225593-1-arnd-r2nGTMty4D4@public.gmane.org>
2017-06-29 5:06 ` Bjorn Andersson
2017-06-29 5:06 ` Bjorn Andersson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.