linux-kernel - Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56C42FE3.8000105@xs4all.nl>
Date:	Wed, 17 Feb 2016 09:31:31 +0100
From:	Hans Verkuil <hverkuil@...all.nl>
To:	tiffany lin <tiffany.lin@...iatek.com>,
	Hans Verkuil <hansverk@...co.com>
Cc:	Hans Verkuil <hans.verkuil@...co.com>, daniel.thompson@...aro.org,
	Rob Herring <robh+dt@...nel.org>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Daniel Kurtz <djkurtz@...omium.org>,
	Pawel Osciak <posciak@...omium.org>,
	Eddie Huang <eddie.huang@...iatek.com>,
	Yingjoe Chen <yingjoe.chen@...iatek.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
	linux-mediatek@...ts.infradead.org, PoChun.Lin@...iatek.com,
	Andrew-CT Chen <andrew-ct.chen@...iatek.com>
Subject: Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video
 Encoder Driver

On 02/17/16 09:01, tiffany lin wrote:
> On Tue, 2016-02-16 at 14:48 +0100, Hans Verkuil wrote:
>> Hi Tiffany,
>>
>>>>>>> +int mtk_vcodec_enc_queue_init(void *priv, struct vb2_queue *src_vq,
>>>>>>> +			   struct vb2_queue *dst_vq)
>>>>>>> +{
>>>>>>> +	struct mtk_vcodec_ctx *ctx = priv;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	src_vq->type		= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>>>>>>> +	src_vq->io_modes	= VB2_DMABUF | VB2_MMAP | VB2_USERPTR;
>>>>>>
>>>>>> I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
>>>>>> and you use physically contiguous DMA.
>>>>>>
>>>>> Now our userspace app use VB2_USERPTR. I need to check if we could drop
>>>>> VB2_USERPTR.
>>>>> We use src_vq->mem_ops = &vb2_dma_contig_memops;
>>>>> And there are
>>>>> 	.get_userptr	= vb2_dc_get_userptr,
>>>>> 	.put_userptr	= vb2_dc_put_userptr,
>>>>> I was confused why it only make sense for scatter-gather.
>>>>> Could you kindly explain more?
>>>>
>>>> VB2_USERPTR indicates that the application can use malloc to allocate buffers
>>>> and pass those to the driver. Since malloc uses virtual memory the physical
>>>> memory is scattered all over. And the first page typically does not start at
>>>> the beginning of the page but at a random offset.
>>>>
>>>> To support that the DMA generally has to be able to do scatter-gather.
>>>>
>>>> Now, where things get ugly is that a hack was added to the USERPTR support where
>>>> apps could pass a pointer to physically contiguous memory as a user pointer. This
>>>> was a hack for embedded systems that preallocated a pool of buffers and needed to
>>>> pass those pointers around somehow. So the dma-contig USERPTR support is for that
>>>> 'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
>>>> reject it. One big problem is that this specific hack isn't signaled anywhere, so
>>>> applications have no way of knowing if the USERPTR support is the proper version
>>>> or the hack where physically contiguous memory is expected.
>>>>
>>>> This hack has been replaced with DMABUF which is the proper way of passing buffers
>>>> around.
>>>>
>>>> New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
>>>> external buffers around.
>>>>
>>>> How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
>>>> to physically contiguous buffers?
>>>>
>>> Understood now. Thanks for your explanation.
>>> Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops. 
>>> I don't know why that dma-contig driver do not reject it.
>>> I will try to figure it out.
>>
>> Is there an iommu involved that turns the scatter-gather list into what looks like
>> contiguous memory for the DMA?
>>
> Yes, We have iommu that could make scatter-gather list looks like
> contiguous memory.
> 
>> At the end of vb2_dc_get_userptr() in videobuf2-dma-contig.c there is a check
>> 'if (contig_size < size)' that verifies that the sg DMA is contiguous. This would
>> work if there is an iommu involved (if I understand it correctly).
>>
> I see. We saw this error before we add iommu support.
> 
>> If that's the case, then it's OK to keep VB2_USERPTR because you have real sg
>> support (although not via the DMA engine, but via iommu mappings).
>>
> Got it. We will keep VB2_USERPTR.

Can you add a comment here mentioning that VB2_USERPTR works with dma-contig because
there is an iommu? That should clarify this.

Regards,

	Hans