Re: [Freedreno] [PATCH 2/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations

[Arnd Bergmann]

 On Jul 26, 2017 6:35 PM, "Jordan Crouse"  wrote:

> >   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);
>

Ok, fixed.

> > @@ -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).

Done.

> > -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 

Thanks for testing!

I've re-sent this patch as v3 now (this one was actually v2, depending on
how you count). I decided not to resend patch 1/2 of the series. We still
need that too, but they are independent. Hope that's ok.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 2/2] drm/msm: gpu: don't abuse dma_alloc for non-DMA allocations

[Jordan Crouse]

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 
> Fixes: 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX")
> Signed-off-by: Arnd Bergmann 
> ---
> 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 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #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 

-- 
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