Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
Hi Wucheng, On Tue, 2016-02-23 at 13:46 +0800, Wu-Cheng Li (李務誠) wrote: > On Thu, Feb 4, 2016 at 7:35 PM, Tiffany Lin wrote: > > From: Andrew-CT Chen > > > > Add v4l2 layer encoder driver for MT8173 > > > > Signed-off-by: Tiffany Lin > > --- > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/Makefile|2 + > > drivers/media/platform/mtk-vcodec/Makefile |8 + > > drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 388 ++ > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1380 > > > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h | 46 + > > .../media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 476 +++ > > .../media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 132 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_intr.c| 102 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_intr.h| 29 + > > drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h | 26 + > > .../media/platform/mtk-vcodec/mtk_vcodec_util.c| 106 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_util.h| 85 ++ > > drivers/media/platform/mtk-vcodec/venc_drv_base.h | 62 + > > drivers/media/platform/mtk-vcodec/venc_drv_if.c| 100 ++ > > drivers/media/platform/mtk-vcodec/venc_drv_if.h| 175 +++ > > drivers/media/platform/mtk-vcodec/venc_ipi_msg.h | 212 +++ > > include/uapi/linux/v4l2-controls.h |4 + > > 18 files changed, 3344 insertions(+) > > create mode 100644 drivers/media/platform/mtk-vcodec/Makefile > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_base.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.c > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_ipi_msg.h > > mode change 100644 => 100755 include/uapi/linux/v4l2-controls.h > > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index ba812d6..3e831c5 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -157,6 +157,17 @@ config VIDEO_MEDIATEK_VPU > > codec embedded in new Mediatek's SOCs. It is able > > to handle video decoding/encoding in a range of formats. > > > > +config VIDEO_MEDIATEK_VCODEC > > +tristate "Mediatek Video Codec driver" > > +depends on VIDEO_DEV && VIDEO_V4L2 > > +depends on ARCH_MEDIATEK || COMPILE_TEST > > +select VIDEOBUF2_DMA_CONTIG > > +select V4L2_MEM2MEM_DEV > > +select MEDIATEK_VPU > > +default n > > +---help--- > > +Mediatek video codec driver for V4L2 > > + > > config VIDEO_MEM2MEM_DEINTERLACE > > tristate "Deinterlace support" > > depends on VIDEO_DEV && VIDEO_V4L2 && DMA_ENGINE > > diff --git a/drivers/media/platform/Makefile > > b/drivers/media/platform/Makefile > > index e5b19c6..510e06b 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -57,3 +57,5 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/ > > ccflags-y += -I$(srctree)/drivers/media/i2c > > > > obj-$(CONFIG_VIDEO_MEDIATEK_VPU) += mtk-vpu/ > > + > > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)+= mtk-vcodec/ > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > > b/drivers/media/platform/mtk-vcodec/Makefile > > new file mode 100644 > > index 000..ce38689 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-vcodec/Makefile > > @@ -0,0 +1,8 @@ > > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk_vcodec_intr.o \ > > + mtk_vcodec_util.o \ > > + mtk_vcodec_enc_drv.o \ > > + mtk_vcodec_enc.o \ > > + mtk_vcodec_enc_pm.o \ > > + venc_drv_if.o > > + > > +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-vpu > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > new file mode 100644 > > index 000..9da2818 > > --- /dev/null > > +++ b/drivers/medi
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On Thu, Feb 4, 2016 at 7:35 PM, Tiffany Lin wrote: > From: Andrew-CT Chen > > Add v4l2 layer encoder driver for MT8173 > > Signed-off-by: Tiffany Lin > --- > drivers/media/platform/Kconfig | 11 + > drivers/media/platform/Makefile|2 + > drivers/media/platform/mtk-vcodec/Makefile |8 + > drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 388 ++ > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1380 > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h | 46 + > .../media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 476 +++ > .../media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 132 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_intr.c| 102 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_intr.h| 29 + > drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h | 26 + > .../media/platform/mtk-vcodec/mtk_vcodec_util.c| 106 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_util.h| 85 ++ > drivers/media/platform/mtk-vcodec/venc_drv_base.h | 62 + > drivers/media/platform/mtk-vcodec/venc_drv_if.c| 100 ++ > drivers/media/platform/mtk-vcodec/venc_drv_if.h| 175 +++ > drivers/media/platform/mtk-vcodec/venc_ipi_msg.h | 212 +++ > include/uapi/linux/v4l2-controls.h |4 + > 18 files changed, 3344 insertions(+) > create mode 100644 drivers/media/platform/mtk-vcodec/Makefile > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_base.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.c > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_ipi_msg.h > mode change 100644 => 100755 include/uapi/linux/v4l2-controls.h > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index ba812d6..3e831c5 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -157,6 +157,17 @@ config VIDEO_MEDIATEK_VPU > codec embedded in new Mediatek's SOCs. It is able > to handle video decoding/encoding in a range of formats. > > +config VIDEO_MEDIATEK_VCODEC > +tristate "Mediatek Video Codec driver" > +depends on VIDEO_DEV && VIDEO_V4L2 > +depends on ARCH_MEDIATEK || COMPILE_TEST > +select VIDEOBUF2_DMA_CONTIG > +select V4L2_MEM2MEM_DEV > +select MEDIATEK_VPU > +default n > +---help--- > +Mediatek video codec driver for V4L2 > + > config VIDEO_MEM2MEM_DEINTERLACE > tristate "Deinterlace support" > depends on VIDEO_DEV && VIDEO_V4L2 && DMA_ENGINE > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index e5b19c6..510e06b 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -57,3 +57,5 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/ > ccflags-y += -I$(srctree)/drivers/media/i2c > > obj-$(CONFIG_VIDEO_MEDIATEK_VPU) += mtk-vpu/ > + > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)+= mtk-vcodec/ > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > b/drivers/media/platform/mtk-vcodec/Makefile > new file mode 100644 > index 000..ce38689 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/Makefile > @@ -0,0 +1,8 @@ > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk_vcodec_intr.o \ > + mtk_vcodec_util.o \ > + mtk_vcodec_enc_drv.o \ > + mtk_vcodec_enc.o \ > + mtk_vcodec_enc_pm.o \ > + venc_drv_if.o > + > +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-vpu > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > new file mode 100644 > index 000..9da2818 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > @@ -0,0 +1,388 @@ > +/* > +* Copyright (c) 2015 MediaTek Inc. > +* Author: PC Chen > +* Tiffany Lin > +* > +* This program is free software; you can redistribute it and/or modify > +* it under the terms of the GNU General
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
Hi Hans,
On Sat, 2016-02-20 at 10:18 +0100, Hans Verkuil wrote:
> On 02/20/2016 10:11 AM, tiffany lin wrote:
> > 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
>
> This is a m2m device, so you should use the m2m variant of this:
> v4l2_m2m_ioctl_create_bufs
>
> That should solve this problem.
>
> Ditto for prepare_buf: you need to use v4l2_m2m_ioctl_prepare_buf.
>
Got it. After using v4l2_m2m_ioctl_create_bufs, the problem was solved.
Thanks for your help.
> Regards,
>
> Hans
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On 02/20/2016 10:11 AM, tiffany lin wrote:
> 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
This is a m2m device, so you should use the m2m variant of this:
v4l2_m2m_ioctl_create_bufs
That should solve this problem.
Ditto for prepare_buf: you need to use v4l2_m2m_ioctl_prepare_buf.
Regards,
Hans
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
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,
> >>> +};
>
>
>
> >>> +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_vid
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
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
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
Hi Hans,
On Wed, 2016-02-17 at 08:47 +0100, Hans Verkuil wrote:
> On 02/16/2016 07:37 AM, tiffany lin wrote:
> >>> +static int fops_vcodec_open(struct file *file)
> >>> +{
> >>> + struct video_device *vfd = video_devdata(file);
> >>> + struct mtk_vcodec_dev *dev = video_drvdata(file);
> >>> + struct mtk_vcodec_ctx *ctx = NULL;
> >>> + int ret = 0;
> >>> +
> >>> + mutex_lock(&dev->dev_mutex);
> >>> +
> >>> + ctx = devm_kzalloc(&dev->plat_dev->dev, sizeof(*ctx), GFP_KERNEL);
> >>> + if (!ctx) {
> >>> + ret = -ENOMEM;
> >>> + goto err_alloc;
> >>> + }
> >>> +
> >>> + if (dev->num_instances >= MTK_VCODEC_MAX_ENCODER_INSTANCES) {
> >>> + mtk_v4l2_err("Too many open contexts\n");
> >>> + ret = -EBUSY;
> >>> + goto err_no_ctx;
> >>
> >> Hmm. I never like it if you can't open a video node because of a reason
> >> like this.
> >>
> >> I.e. a simple 'v4l2-ctl -D' (i.e. calling QUERYCAP) should never fail.
> >>
> >> If there are hardware limitation that prevent more than X instances from
> >> running at
> >> the same time, then those limitations typically kick in when you start to
> >> stream
> >> (or possibly when calling REQBUFS). But before that it should always be
> >> possible to
> >> open the device.
> >>
> >> Having this check at open() is an indication of a poor design.
> >>
> >> Is this is a hardware limitation at all?
> >>
> > This is to make sure performance meet requirements, such as bitrate and
> > framerate.
>
> Is it the driver's job to make enforce this? What if the application only
> deals with low-res video, but wants to encode a lot of those? Or is encoding
> a video off-line?
>
> The driver generally doesn't know the use-case, so if this is an artificial
> limitation as opposed to a hardware limitation, then I would just drop this.
>
We got your point, we will remove this artificial limitation in next
version.
best regards,
Tiffany
> Regards,
>
> Hans
>
> > We got your point. We will remove this and move limitation control to
> > start_streaming or REQBUFS.
> > Appreciated for your suggestion.:)
> >
> >
> >>> + }
>
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
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
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.
> Regards,
>
> Hans
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On 02/16/2016 07:37 AM, tiffany lin wrote:
>>> +static int fops_vcodec_open(struct file *file)
>>> +{
>>> + struct video_device *vfd = video_devdata(file);
>>> + struct mtk_vcodec_dev *dev = video_drvdata(file);
>>> + struct mtk_vcodec_ctx *ctx = NULL;
>>> + int ret = 0;
>>> +
>>> + mutex_lock(&dev->dev_mutex);
>>> +
>>> + ctx = devm_kzalloc(&dev->plat_dev->dev, sizeof(*ctx), GFP_KERNEL);
>>> + if (!ctx) {
>>> + ret = -ENOMEM;
>>> + goto err_alloc;
>>> + }
>>> +
>>> + if (dev->num_instances >= MTK_VCODEC_MAX_ENCODER_INSTANCES) {
>>> + mtk_v4l2_err("Too many open contexts\n");
>>> + ret = -EBUSY;
>>> + goto err_no_ctx;
>>
>> Hmm. I never like it if you can't open a video node because of a reason like
>> this.
>>
>> I.e. a simple 'v4l2-ctl -D' (i.e. calling QUERYCAP) should never fail.
>>
>> If there are hardware limitation that prevent more than X instances from
>> running at
>> the same time, then those limitations typically kick in when you start to
>> stream
>> (or possibly when calling REQBUFS). But before that it should always be
>> possible to
>> open the device.
>>
>> Having this check at open() is an indication of a poor design.
>>
>> Is this is a hardware limitation at all?
>>
> This is to make sure performance meet requirements, such as bitrate and
> framerate.
Is it the driver's job to make enforce this? What if the application only
deals with low-res video, but wants to encode a lot of those? Or is encoding
a video off-line?
The driver generally doesn't know the use-case, so if this is an artificial
limitation as opposed to a hardware limitation, then I would just drop this.
Regards,
Hans
> We got your point. We will remove this and move limitation control to
> start_streaming or REQBUFS.
> Appreciated for your suggestion.:)
>
>
>>> + }
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
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?
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).
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).
Regards,
Hans
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On Tue, 2016-02-16 at 08:44 +0100, Hans Verkuil wrote:
> On 02/16/2016 07:37 AM, tiffany lin wrote:
>
>
>
> >>> +static int vidioc_venc_s_parm(struct file *file, void *priv,
> >>> + struct v4l2_streamparm *a)
> >>> +{
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> +
> >>> + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >>> + ctx->enc_params.framerate_num =
> >>> + a->parm.output.timeperframe.denominator;
> >>> + ctx->enc_params.framerate_denom =
> >>> + a->parm.output.timeperframe.numerator;
> >>> + ctx->param_change |= MTK_ENCODE_PARAM_FRAMERATE;
> >>> + } else {
> >>> + return -EINVAL;
> >>> + }
> >>
> >> I'd invert the test:
> >>
> >>if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> >>return -EINVAL;
> >>
> >> and now you can just set ctx->enc_params.
> >>
> > We will fix this in next version.
> >
> >
> >>> + return 0;
> >>> +}
> >>
> >> And if there is an s_parm, then there should be a g_parm as well!
> >>
> > Now our driver does not support g_parm, our use cases do not use g_parm
> > too.
> > Do we need to add g_parm at this moment? Or we could add it when we need
> > g_parm?
>
> No, you need it. You can see why if you look at the v4l2-compliance output:
>
> test VIDIOC_G/S_PARM: OK (Not Supported)
>
> Why does it think it is unsupported? Because (just like most applications) it
> tries to call G_PARM first, and if that succeeds it tries to call S_PARM with
> the value it got from G_PARM. Thus ensuring the application doesn't change the
> driver state. So you can have a 'get' ioctl without the 'set' ioctl, but if
> there is a 'set' ioctl there must always be a 'get' ioctl.
>
Got it. We will add g_parm in next version.
>
>
> >>> +static int vidioc_venc_g_s_selection(struct file *file, void *priv,
> >>> +struct v4l2_selection *s)
> >>
> >> Why support s_selection if you can only return the current width and
> >> height?
> >> And why support g_selection if you can't change the selection?
> >>
> >> In other words, why implement this at all?
> >>
> >> Unless I am missing something here, I would just drop this.
> >>
> > Now our driver do not support these capabilities, but userspace app will
> > check whether g/s_crop are implemented when using encoder.
> > Because g/s_crop are deprecated as you mentioned in previous v2 review
> > comments. We change to use g_s_selection.
> > We will check if we could add this capability.
>
> It's true that you should use g/s_selection instead of g/s_crop, but only if
> there is actually something to select. As long as you don't offer this
> capability,
> just drop this for now.
>
> When you add the capability later you can just add the g/s_selection
> functions.
>
> Getting selection right can be tricky. I wouldn't mind if this is done later
> in a
> separate patch.
>
Got it. We will add the capability later.
> >
> >>> +{
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> + struct mtk_q_data *q_data;
> >>> +
> >>> + if (V4L2_TYPE_IS_OUTPUT(s->type)) {
> >>> + if (s->target != V4L2_SEL_TGT_COMPOSE)
> >>> + return -EINVAL;
> >>> + } else {
> >>> + if (s->target != V4L2_SEL_TGT_CROP)
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (s->r.left || s->r.top)
> >>> + return -EINVAL;
> >>> +
> >>> + q_data = mtk_venc_get_q_data(ctx, s->type);
> >>> + if (!q_data)
> >>> + return -EINVAL;
> >>> +
> >>> + s->r.width = q_data->width;
> >>> + s->r.height = q_data->height;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +
> >>> +static int vidioc_venc_qbuf(struct file *file, void *priv,
> >>> + struct v4l2_buffer *buf)
> >>> +{
> >>> +
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> +
> >>> + if (ctx->state == MTK_STATE_ABORT) {
> >>> + mtk_v4l2_err("[%d] Call on QBUF after unrecoverable error\n",
> >>> ctx->idx);
> >>> + return -EIO;
> >>> + }
> >>> +
> >>> + return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
> >>> +}
> >>> +
> >>> +static int vidioc_venc_dqbuf(struct file *file, void *priv,
> >>> + struct v4l2_buffer *buf)
> >>> +{
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> + if (ctx->state == MTK_STATE_ABORT) {
> >>> + mtk_v4l2_err("[%d] Call on QBUF after unrecoverable error\n",
> >>> ctx->idx);
> >>> + return -EIO;
> >>> + }
> >>> +
> >>> + return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
> >>> +}
> >>> +
> >>> +
> >>> +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,
> >>> + .vidi
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On 02/16/2016 07:37 AM, tiffany lin wrote:
>>> +static int vidioc_venc_s_parm(struct file *file, void *priv,
>>> + struct v4l2_streamparm *a)
>>> +{
>>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
>>> +
>>> + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>> + ctx->enc_params.framerate_num =
>>> + a->parm.output.timeperframe.denominator;
>>> + ctx->enc_params.framerate_denom =
>>> + a->parm.output.timeperframe.numerator;
>>> + ctx->param_change |= MTK_ENCODE_PARAM_FRAMERATE;
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>
>> I'd invert the test:
>>
>> if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> return -EINVAL;
>>
>> and now you can just set ctx->enc_params.
>>
> We will fix this in next version.
>
>
>>> + return 0;
>>> +}
>>
>> And if there is an s_parm, then there should be a g_parm as well!
>>
> Now our driver does not support g_parm, our use cases do not use g_parm
> too.
> Do we need to add g_parm at this moment? Or we could add it when we need
> g_parm?
No, you need it. You can see why if you look at the v4l2-compliance output:
test VIDIOC_G/S_PARM: OK (Not Supported)
Why does it think it is unsupported? Because (just like most applications) it
tries to call G_PARM first, and if that succeeds it tries to call S_PARM with
the value it got from G_PARM. Thus ensuring the application doesn't change the
driver state. So you can have a 'get' ioctl without the 'set' ioctl, but if
there is a 'set' ioctl there must always be a 'get' ioctl.
>>> +static int vidioc_venc_g_s_selection(struct file *file, void *priv,
>>> +struct v4l2_selection *s)
>>
>> Why support s_selection if you can only return the current width and height?
>> And why support g_selection if you can't change the selection?
>>
>> In other words, why implement this at all?
>>
>> Unless I am missing something here, I would just drop this.
>>
> Now our driver do not support these capabilities, but userspace app will
> check whether g/s_crop are implemented when using encoder.
> Because g/s_crop are deprecated as you mentioned in previous v2 review
> comments. We change to use g_s_selection.
> We will check if we could add this capability.
It's true that you should use g/s_selection instead of g/s_crop, but only if
there is actually something to select. As long as you don't offer this
capability,
just drop this for now.
When you add the capability later you can just add the g/s_selection functions.
Getting selection right can be tricky. I wouldn't mind if this is done later in
a
separate patch.
>
>>> +{
>>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
>>> + struct mtk_q_data *q_data;
>>> +
>>> + if (V4L2_TYPE_IS_OUTPUT(s->type)) {
>>> + if (s->target != V4L2_SEL_TGT_COMPOSE)
>>> + return -EINVAL;
>>> + } else {
>>> + if (s->target != V4L2_SEL_TGT_CROP)
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (s->r.left || s->r.top)
>>> + return -EINVAL;
>>> +
>>> + q_data = mtk_venc_get_q_data(ctx, s->type);
>>> + if (!q_data)
>>> + return -EINVAL;
>>> +
>>> + s->r.width = q_data->width;
>>> + s->r.height = q_data->height;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
>>> +static int vidioc_venc_qbuf(struct file *file, void *priv,
>>> + struct v4l2_buffer *buf)
>>> +{
>>> +
>>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
>>> +
>>> + if (ctx->state == MTK_STATE_ABORT) {
>>> + mtk_v4l2_err("[%d] Call on QBUF after unrecoverable error\n",
>>> ctx->idx);
>>> + return -EIO;
>>> + }
>>> +
>>> + return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
>>> +}
>>> +
>>> +static int vidioc_venc_dqbuf(struct file *file, void *priv,
>>> +struct v4l2_buffer *buf)
>>> +{
>>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
>>> + if (ctx->state == MTK_STATE_ABORT) {
>>> + mtk_v4l2_err("[%d] Call on QBUF after unrecoverable error\n",
>>> ctx->idx);
>>> + return -EIO;
>>> + }
>>> +
>>> + return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
>>> +}
>>> +
>>> +
>>> +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,
>>>
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
Hi Hans, Thanks for your time. On Mon, 2016-02-15 at 12:21 +0100, Hans Verkuil wrote: > On 02/04/2016 12:35 PM, Tiffany Lin wrote: > > From: Andrew-CT Chen > > > > Add v4l2 layer encoder driver for MT8173 > > > > Signed-off-by: Tiffany Lin > > If Andrew is the author, shouldn't there be a Signed-off-by from him as well? > > And in copyright notices (might want to update the year to 2016 BTW) PC Chen > is mentioned among others. It might be useful to update the Signed-off-by > lines. > Author is PC Chen and Tiffany Lin, Andrew-CT Chen is author of mtk-vpu module. We will fix copyright and this in next version. > > --- > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/Makefile|2 + > > drivers/media/platform/mtk-vcodec/Makefile |8 + > > drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 388 ++ > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1380 > > > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h | 46 + > > .../media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 476 +++ > > .../media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 132 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_intr.c| 102 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_intr.h| 29 + > > drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h | 26 + > > .../media/platform/mtk-vcodec/mtk_vcodec_util.c| 106 ++ > > .../media/platform/mtk-vcodec/mtk_vcodec_util.h| 85 ++ > > drivers/media/platform/mtk-vcodec/venc_drv_base.h | 62 + > > drivers/media/platform/mtk-vcodec/venc_drv_if.c| 100 ++ > > drivers/media/platform/mtk-vcodec/venc_drv_if.h| 175 +++ > > drivers/media/platform/mtk-vcodec/venc_ipi_msg.h | 212 +++ > > include/uapi/linux/v4l2-controls.h |4 + > > 18 files changed, 3344 insertions(+) > > create mode 100644 drivers/media/platform/mtk-vcodec/Makefile > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_base.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.c > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.h > > create mode 100644 drivers/media/platform/mtk-vcodec/venc_ipi_msg.h > > mode change 100644 => 100755 include/uapi/linux/v4l2-controls.h > > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index ba812d6..3e831c5 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -157,6 +157,17 @@ config VIDEO_MEDIATEK_VPU > > codec embedded in new Mediatek's SOCs. It is able > > to handle video decoding/encoding in a range of formats. > > > > +config VIDEO_MEDIATEK_VCODEC > > +tristate "Mediatek Video Codec driver" > > +depends on VIDEO_DEV && VIDEO_V4L2 > > +depends on ARCH_MEDIATEK || COMPILE_TEST > > +select VIDEOBUF2_DMA_CONTIG > > +select V4L2_MEM2MEM_DEV > > +select MEDIATEK_VPU > > +default n > > +---help--- > > +Mediatek video codec driver for V4L2 > > + > > config VIDEO_MEM2MEM_DEINTERLACE > > tristate "Deinterlace support" > > depends on VIDEO_DEV && VIDEO_V4L2 && DMA_ENGINE > > diff --git a/drivers/media/platform/Makefile > > b/drivers/media/platform/Makefile > > index e5b19c6..510e06b 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -57,3 +57,5 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/ > > ccflags-y += -I$(srctree)/drivers/media/i2c > > > > obj-$(CONFIG_VIDEO_MEDIATEK_VPU) += mtk-vpu/ > > + > > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)+= mtk-vcodec/ > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > > b/drivers/media/platform/mtk-vcodec/Makefile > > new file mode 100644 > > index 000..ce38689 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-vcodec/Makefile > > @@ -0,0 +1,8 @@ > > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk_vcodec_intr.o \ > > + mtk_vcodec_util.o \ > > + mtk_vcodec_enc_drv.o \ > > + mtk_vcodec_enc.o \ > > +
Re: [PATCH v4 5/8] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver
On 02/04/2016 12:35 PM, Tiffany Lin wrote: > From: Andrew-CT Chen > > Add v4l2 layer encoder driver for MT8173 > > Signed-off-by: Tiffany Lin If Andrew is the author, shouldn't there be a Signed-off-by from him as well? And in copyright notices (might want to update the year to 2016 BTW) PC Chen is mentioned among others. It might be useful to update the Signed-off-by lines. > --- > drivers/media/platform/Kconfig | 11 + > drivers/media/platform/Makefile|2 + > drivers/media/platform/mtk-vcodec/Makefile |8 + > drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 388 ++ > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1380 > > drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h | 46 + > .../media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 476 +++ > .../media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 132 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_intr.c| 102 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_intr.h| 29 + > drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h | 26 + > .../media/platform/mtk-vcodec/mtk_vcodec_util.c| 106 ++ > .../media/platform/mtk-vcodec/mtk_vcodec_util.h| 85 ++ > drivers/media/platform/mtk-vcodec/venc_drv_base.h | 62 + > drivers/media/platform/mtk-vcodec/venc_drv_if.c| 100 ++ > drivers/media/platform/mtk-vcodec/venc_drv_if.h| 175 +++ > drivers/media/platform/mtk-vcodec/venc_ipi_msg.h | 212 +++ > include/uapi/linux/v4l2-controls.h |4 + > 18 files changed, 3344 insertions(+) > create mode 100644 drivers/media/platform/mtk-vcodec/Makefile > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_pm.h > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_base.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.c > create mode 100644 drivers/media/platform/mtk-vcodec/venc_drv_if.h > create mode 100644 drivers/media/platform/mtk-vcodec/venc_ipi_msg.h > mode change 100644 => 100755 include/uapi/linux/v4l2-controls.h > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index ba812d6..3e831c5 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -157,6 +157,17 @@ config VIDEO_MEDIATEK_VPU > codec embedded in new Mediatek's SOCs. It is able > to handle video decoding/encoding in a range of formats. > > +config VIDEO_MEDIATEK_VCODEC > +tristate "Mediatek Video Codec driver" > +depends on VIDEO_DEV && VIDEO_V4L2 > +depends on ARCH_MEDIATEK || COMPILE_TEST > +select VIDEOBUF2_DMA_CONTIG > +select V4L2_MEM2MEM_DEV > +select MEDIATEK_VPU > +default n > +---help--- > +Mediatek video codec driver for V4L2 > + > config VIDEO_MEM2MEM_DEINTERLACE > tristate "Deinterlace support" > depends on VIDEO_DEV && VIDEO_V4L2 && DMA_ENGINE > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index e5b19c6..510e06b 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -57,3 +57,5 @@ obj-$(CONFIG_VIDEO_XILINX) += xilinx/ > ccflags-y += -I$(srctree)/drivers/media/i2c > > obj-$(CONFIG_VIDEO_MEDIATEK_VPU) += mtk-vpu/ > + > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec/ > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > b/drivers/media/platform/mtk-vcodec/Makefile > new file mode 100644 > index 000..ce38689 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/Makefile > @@ -0,0 +1,8 @@ > +obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk_vcodec_intr.o \ > +mtk_vcodec_util.o \ > +mtk_vcodec_enc_drv.o \ > +mtk_vcodec_enc.o \ > +mtk_vcodec_enc_pm.o \ > +venc_drv_if.o > + > +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-vpu > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > new file mode 100644 > index 000..9da2818 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > @@ -0,0 +1,388 @@
