Re: drm: msm: run into issues

[Stanimir Varbanov <svarbanov@mm-sol.com>]

On 07/23/2015 06:59 PM, Rob Clark wrote:
> On Thu, Jul 23, 2015 at 5:02 AM, Stanimir Varbanov
> <stanimir.varbanov@linaro.org> 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?

In fact this is consequence of the first issue.

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

Brilliant diff, duplicating sgt fixed both issues!

Care to fix this in mainline?

-- 
regards,
Stan