On Wed, 2016-02-17 at 09:31 +0100, Hans Verkuil wrote:
> 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.
>
Got it. I will add comment before this line.
src_vq->io_modes = VB2_DMABUF | VB2_MMAP | VB2_USERPTR;.
I will also add out IOMMU patches information in cover letter, like:
The following patches are needed to support VB2_USERPTR works with
dma-contig.
https://patchwork.kernel.org/patch/8335461/
https://patchwork.kernel.org/patch/8335471/
https://patchwork.kernel.org/patch/8335481/
https://patchwork.kernel.org/patch/8335491/
https://patchwork.kernel.org/patch/8335501/
https://patchwork.kernel.org/patch/7596181/
best regards,
Tiffany
> Regards,
>
> Hans
