On Wed, Jul 26, 2017 at 05:52:45PM +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. What the code actually wants to do is to get the physical address from the reserved-mem node. It goes through the dma-mapping interfaces for obscure reasons, and this apparently only works by chance, relying on specific bugs in the error handling of the arm64 dma-mapping implementation.
The same problem existed in the "venus" media driver, which was now fixed by Stanimir Varbanov after long discussions.
In order to make some progress here, I have now ported his approach over to the adreno driver. The patch is currently untested, and should get a good review, but it is now much simpler than the original, and it should be obvious what goes wrong if I made a mistake in the port.
See also: a6e2d36bf6b7 ("media: venus: don't abuse dma_alloc for non-DMA allocations") Cc: Stanimir Varbanov stanimir.varbanov@linaro.org Fixes: 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX") Signed-off-by: Arnd Bergmann arnd@arndb.de
I think we want this to be applied for 4.13, as the upstream code that was added in the merge window is seriously broken without it
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 71 ++++++++++++----------------------- drivers/gpu/drm/msm/adreno/a5xx_gpu.h | 2 - 2 files changed, 23 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 1d54c76a7778..ce545b3a9d17 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -15,7 +15,7 @@ #include <linux/cpumask.h> #include <linux/qcom_scm.h> #include <linux/dma-mapping.h> -#include <linux/of_reserved_mem.h> +#include <linux/of_address.h> #include <linux/soc/qcom/mdt_loader.h> #include "msm_gem.h" #include "msm_mmu.h" @@ -29,6 +29,8 @@ static void a5xx_dump(struct msm_gpu *gpu); static int zap_shader_load_mdt(struct device *dev, const char *fwname) { const struct firmware *fw;
- struct device_node *np;
- struct resource r; phys_addr_t mem_phys; ssize_t mem_size; void *mem_region = NULL;
@@ -37,6 +39,21 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname) if (!IS_ENABLED(CONFIG_ARCH_QCOM)) return -EINVAL;
- np = of_get_child_by_name(dev->of_node, "zap-shader");
- if (!np)
return -ENODEV;- np = of_parse_phandle(dev->of_node, "memory-region", 0);
I think this should be np = of_parse_phandle(np, "memory-region", 0);
With that change this patch works great.
@@ -373,44 +393,6 @@ static int a5xx_zap_shader_resume(struct msm_gpu *gpu) }
/* Set up a child device to "own" the zap shader */
This now incorrect comment can be zapped (pun intended).
-static int a5xx_zap_shader_dev_init(struct device *parent, struct device *dev) -{
- struct device_node *node;
- int ret;
- if (dev->parent)
return 0;
With above changes, Acked-and-Tested-By: Jordan Crouse jcrouse@codeaurora.org