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

[tiffany lin <tiffany.lin@mediatek.com>]

Hi Hans,

On Tue, 2016-02-16 at 08:44 +0100, Hans Verkuil wrote:
> On 02/16/2016 07:37 AM, tiffany lin wrote:
> >>> +
> >>> +const struct v4l2_ioctl_ops mtk_venc_ioctl_ops = {
> >>> +	.vidioc_streamon		= v4l2_m2m_ioctl_streamon,
> >>> +	.vidioc_streamoff		= v4l2_m2m_ioctl_streamoff,
> >>> +
> >>> +	.vidioc_reqbufs			= v4l2_m2m_ioctl_reqbufs,
> >>> +	.vidioc_querybuf		= v4l2_m2m_ioctl_querybuf,
> >>> +	.vidioc_qbuf			= vidioc_venc_qbuf,
> >>> +	.vidioc_dqbuf			= vidioc_venc_dqbuf,
> >>> +
> >>> +	.vidioc_querycap		= vidioc_venc_querycap,
> >>> +	.vidioc_enum_fmt_vid_cap_mplane = vidioc_enum_fmt_vid_cap_mplane,
> >>> +	.vidioc_enum_fmt_vid_out_mplane = vidioc_enum_fmt_vid_out_mplane,
> >>> +	.vidioc_enum_framesizes		= vidioc_enum_framesizes,
> >>> +
> >>> +	.vidioc_try_fmt_vid_cap_mplane	= vidioc_try_fmt_vid_cap_mplane,
> >>> +	.vidioc_try_fmt_vid_out_mplane	= vidioc_try_fmt_vid_out_mplane,
> >>> +	.vidioc_expbuf			= v4l2_m2m_ioctl_expbuf,
> >>
> >> Please add vidioc_create_bufs and vidioc_prepare_buf as well.
> >>
> > 
> > Currently we do not support these use cases, do we need to add
> > vidioc_create_bufs and vidioc_prepare_buf now?
> 
> I would suggest you do. The vb2 framework gives it (almost) for free.
> prepare_buf is completely free (just add the helper) and create_bufs
> needs a few small changes in the queue_setup function, that's all.
> 
After try to add vidioc_create_bufs directly using
vb2_ioctl_create_bufs, it will have problem in 
	int res = vb2_verify_memory_type(vdev->queue, p->memory,
			p->format.type);
We do not init our video_device queue in device probe function.

Our vb2_queues for OUTPUT and CAPTURE are initialized in
v4l2_m2m_ctx_init when ctx instance open.
What is queue in video_device for?
If we should init vdev->queue in probe function, this queue format
should be CAPTURE queue or OUTPUT queue?

best regards,
Tiffany

> > 
> > 
> >>> +	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
> >>> +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
> >>> +
> >>> +	.vidioc_s_parm			= vidioc_venc_s_parm,
> >>> +
> >>> +	.vidioc_s_fmt_vid_cap_mplane	= vidioc_venc_s_fmt,
> >>> +	.vidioc_s_fmt_vid_out_mplane	= vidioc_venc_s_fmt,
> >>> +
> >>> +	.vidioc_g_fmt_vid_cap_mplane	= vidioc_venc_g_fmt,
> >>> +	.vidioc_g_fmt_vid_out_mplane	= vidioc_venc_g_fmt,
> >>> +
> >>> +	.vidioc_g_selection		= vidioc_venc_g_s_selection,
> >>> +	.vidioc_s_selection		= vidioc_venc_g_s_selection,
> >>> +};
> 
> <snip>
> 
> >>> +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?
> 
> > 
> >>> +	src_vq->drv_priv	= ctx;
> >>> +	src_vq->buf_struct_size = sizeof(struct mtk_video_enc_buf);
> >>> +	src_vq->ops		= &mtk_venc_vb2_ops;
> >>> +	src_vq->mem_ops		= &vb2_dma_contig_memops;
> >>> +	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >>> +	src_vq->lock = &ctx->dev->dev_mutex;
> >>> +
> >>> +	ret = vb2_queue_init(src_vq);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	dst_vq->type		= V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >>> +	dst_vq->io_modes	= VB2_DMABUF | VB2_MMAP | VB2_USERPTR;
> >>> +	dst_vq->drv_priv	= ctx;
> >>> +	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> >>> +	dst_vq->ops		= &mtk_venc_vb2_ops;
> >>> +	dst_vq->mem_ops		= &vb2_dma_contig_memops;
> >>> +	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >>> +	dst_vq->lock = &ctx->dev->dev_mutex;
> >>> +
> >>> +	return vb2_queue_init(dst_vq);
> >>> +}
> 
> Regards,
> 
> 	Hans
>