| Message ID | 20170620201720.225593-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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,
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(-)