On Thu, Jul 23, 2015 at 12:23 PM, Stanimir Varbanov
<svarbanov@xxxxxxxxxx> wrote:
> On 07/23/2015 06:59 PM, Rob Clark wrote:
>> 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?
>
> 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?
will do, just sent patch.. I'll include it for 4.2-fixes shortly
BR,
-R
> --
> regards,
> Stan
--
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
