Re: drm: msm: run into issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 23, 2015 at 5:02 AM, Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> wrote:
> Hi Rob,
>
> I run into issues with msm drm driver while implementing a test
> application which use v4l2 vidc (venus) decoder driver to decode videos
> and msm drm driver to display the decoded frames. The v4l2 test
> application use msm drm as dmabuf exporter by DRM_IOCTL_MODE_CREATE_DUMB
> and DRM_IOCTL_PRIME_HANDLE_TO_FD from libdrm.
>
> So far the test app is able to decode and display using dmabuf type of
> buffers (so, to honest it is slower than using mmap & memcpy but this is
> another story). The problems start when destroying drm buffers by call
> to DRM_IOCTL_MODE_DESTROY_DUMB ioctl and also when closing dmabuf fd.
> The issues I'm seeing are:
>
> - the first and the major one happens in msm_gem_free_object() where we
> are trying to free sg table which table is already freed by drm_prime
> core in dmabuf .detach callback. In fact the msm_gem_free_object is
> called by dmabuf .release callback which should be happened after
> .detach. I find weird to call sg_table_free in .detach callback and did
> not understand why it is there.

so, I think in msm_gem_prime_get_sg_table() we need to create a
duplicate sgt.. since dma_buf_map_attachment() (and therefore
->gem_prime_get_sg_table()) returns ownership of the sgt.  So it is
not correct to be returning our own internal copy.

> - the second one is again in msm_gem.c::put_pages dma_unmap_sg call.
> Some background, vidc (venus) driver use dma-iommu infrastructure copped
> from qcom downstream kernel to abstract iommu map/unmap [1].
> On the other side when drm driver is exporter it use swiotbl [2] dma map
> operations, dma_map_sg will fill sg->dma_address with phy addresses. So
> when vidc call dma_map_sg the sg dma_address is filled with iova address
> then drm call dma_unmap_sg expecting phy addresses.

hmm.. so drm_gem_map_dma_buf() should dma_map_sg() using attach->dev
(which should be vidc, not drm).  Maybe something unexepected was
happening there since we were incorrectly returning our own sgt (which
had already been dma_map_sg()'d w/ the drm device for cache
maintenance?

Could you try:

-------------
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c
b/drivers/gpu/drm/msm/msm_gem_prime.c
index dd7a7ab..831461b 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -23,8 +23,12 @@
 struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
 {
     struct msm_gem_object *msm_obj = to_msm_bo(obj);
-    BUG_ON(!msm_obj->sgt);  /* should have already pinned! */
-    return msm_obj->sgt;
+    int npages = obj->size >> PAGE_SHIFT;
+
+    if (WARN_ON(!msm_obj->pages))  /* should have already pinned! */
+        return NULL;
+
+    return drm_prime_pages_to_sg(msm_obj->pages, npages);
 }

 void *msm_gem_prime_vmap(struct drm_gem_object *obj)
-------------

> If I didn't express myself well, see the hack bellow which I made to
> prevent kernel crash.
>
> Any thoughts?
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 7482b06cd08f..c4c0f8084b11 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -155,10 +155,10 @@ static void drm_gem_map_detach(struct dma_buf
> *dma_buf,
>                 if (prime_attach->dir != DMA_NONE)
>                         dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,
>                                         prime_attach->dir);
> -               sg_free_table(sgt);
> +/*             sg_free_table(sgt);*/
>         }
>
> -       kfree(sgt);
> +/*     kfree(sgt);*/
>         kfree(prime_attach);
>         attach->priv = NULL;
>  }
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 9b6220140e99..a78a24ca2fda 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -116,9 +116,10 @@ static void put_pages(struct drm_gem_object *obj)
>                 /* For non-cached buffers, ensure the new pages are clean
>                  * because display controller, GPU, etc. are not coherent:
>                  */
> -               if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> +/*             if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
>                         dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
>                                         msm_obj->sgt->nents,
> DMA_BIDIRECTIONAL);
> +*/
>                 sg_free_table(msm_obj->sgt);
>                 kfree(msm_obj->sgt);
>
>
> --
> regards,
> Stan
>
> [1]
> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/blob/refs/heads/release/qcomlt-4.0:/arch/arm64/mm/dma-mapping.c#l1315
>
> [2]
> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/blob/refs/heads/release/qcomlt-4.0:/arch/arm64/mm/dma-mapping.c#l350
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux