| Message ID | 1454585703-42428-5-git-send-email-tiffany.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 04, 2016 at 07:34:59PM +0800, Tiffany Lin wrote: > Add a DT binding documentation of Video Encoder for the > MT8173 SoC from Mediatek. > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com> > --- > .../devicetree/bindings/media/mediatek-vcodec.txt | 59 ++++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt Acked-by: Rob Herring <robh@kernel.org>
Hi Tiffany, On Thu, Feb 4, 2016 at 7:34 PM, Tiffany Lin <tiffany.lin@mediatek.com> wrote: > Add a DT binding documentation of Video Encoder for the > MT8173 SoC from Mediatek. > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com> > --- > .../devicetree/bindings/media/mediatek-vcodec.txt | 59 ++++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt > > diff --git a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt > new file mode 100644 > index 0000000..572bfdd > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt > @@ -0,0 +1,59 @@ > +Mediatek Video Codec > + > +Mediatek Video Codec is the video codec hw present in Mediatek SoCs which > +supports high resolution encoding functionalities. > + > +Required properties: > +- compatible : "mediatek,mt8173-vcodec-enc" for encoder > +- reg : Physical base address of the video codec registers and length of > + memory mapped region. > +- interrupts : interrupt number to the cpu. > +- mediatek,larb : must contain the local arbiters in the current Socs. > +- clocks : list of clock specifiers, corresponding to entries in > + the clock-names property. > +- clock-names: encoder must contain "vencpll_d2", "venc_sel", "univpll1_d2", > + "venc_lt_sel". > +- iommus : should point to the respective IOMMU block with master port as > + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > + for details. > +- mediatek,vpu : the node of video processor unit > + > +Example: > +vcodec_enc: vcodec@0x18002000 { > + compatible = "mediatek,mt8173-vcodec-enc"; > + reg = <0 0x18002000 0 0x1000>, /*VENC_SYS*/ > + <0 0x19002000 0 0x1000>; /*VENC_LT_SYS*/ This really looks like two encoder devices combined into a single device tree node. There are two register sets, two irqs, two sets of iommus, and two sets of clocks. If possible, please split this node into two, one for each encoder. > + interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_LOW>, > + <GIC_SPI 202 IRQ_TYPE_LEVEL_LOW>; > + mediatek,larb = <&larb3>, > + <&larb5>; > + iommus = <&iommu M4U_PORT_VENC_RCPU>, > + <&iommu M4U_PORT_VENC_REC>, > + <&iommu M4U_PORT_VENC_BSDMA>, > + <&iommu M4U_PORT_VENC_SV_COMV>, > + <&iommu M4U_PORT_VENC_RD_COMV>, > + <&iommu M4U_PORT_VENC_CUR_LUMA>, > + <&iommu M4U_PORT_VENC_CUR_CHROMA>, > + <&iommu M4U_PORT_VENC_REF_LUMA>, > + <&iommu M4U_PORT_VENC_REF_CHROMA>, > + <&iommu M4U_PORT_VENC_NBM_RDMA>, > + <&iommu M4U_PORT_VENC_NBM_WDMA>, > + <&iommu M4U_PORT_VENC_RCPU_SET2>, > + <&iommu M4U_PORT_VENC_REC_FRM_SET2>, > + <&iommu M4U_PORT_VENC_BSDMA_SET2>, > + <&iommu M4U_PORT_VENC_SV_COMA_SET2>, > + <&iommu M4U_PORT_VENC_RD_COMA_SET2>, > + <&iommu M4U_PORT_VENC_CUR_LUMA_SET2>, > + <&iommu M4U_PORT_VENC_CUR_CHROMA_SET2>, > + <&iommu M4U_PORT_VENC_REF_LUMA_SET2>, > + <&iommu M4U_PORT_VENC_REC_CHROMA_SET2>; > + mediatek,vpu = <&vpu>; > + clocks = <&topckgen CLK_TOP_VENCPLL_D2>, > + <&topckgen CLK_TOP_VENC_SEL>, > + <&topckgen CLK_TOP_UNIVPLL1_D2>, > + <&topckgen CLK_TOP_VENC_LT_SEL>; > + clock-names = "vencpll_d2", > + "venc_sel", > + "univpll1_d2", > + "venc_lt_sel"; The names of these clocks should be from the perspective of the encoder, not the clock provider. -Dan > + }; > -- > 1.7.9.5 >
On Tue, Feb 9, 2016 at 7:29 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > Hi Tiffany, > > On Thu, Feb 4, 2016 at 7:34 PM, Tiffany Lin <tiffany.lin@mediatek.com> wrote: >> Add a DT binding documentation of Video Encoder for the >> MT8173 SoC from Mediatek. >> >> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com> >> --- >> .../devicetree/bindings/media/mediatek-vcodec.txt | 59 ++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt >> >> diff --git a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt >> new file mode 100644 >> index 0000000..572bfdd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt >> @@ -0,0 +1,59 @@ >> +Mediatek Video Codec >> + >> +Mediatek Video Codec is the video codec hw present in Mediatek SoCs which >> +supports high resolution encoding functionalities. >> + >> +Required properties: >> +- compatible : "mediatek,mt8173-vcodec-enc" for encoder >> +- reg : Physical base address of the video codec registers and length of >> + memory mapped region. >> +- interrupts : interrupt number to the cpu. >> +- mediatek,larb : must contain the local arbiters in the current Socs. >> +- clocks : list of clock specifiers, corresponding to entries in >> + the clock-names property. >> +- clock-names: encoder must contain "vencpll_d2", "venc_sel", "univpll1_d2", >> + "venc_lt_sel". >> +- iommus : should point to the respective IOMMU block with master port as >> + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt >> + for details. >> +- mediatek,vpu : the node of video processor unit >> + >> +Example: >> +vcodec_enc: vcodec@0x18002000 { >> + compatible = "mediatek,mt8173-vcodec-enc"; >> + reg = <0 0x18002000 0 0x1000>, /*VENC_SYS*/ >> + <0 0x19002000 0 0x1000>; /*VENC_LT_SYS*/ > > This really looks like two encoder devices combined into a single > device tree node. > There are two register sets, two irqs, two sets of iommus, and two > sets of clocks. > > If possible, please split this node into two, one for each encoder. I chatted offline with Mediatek. They explained that there really is just one encoder hardware, that happens to support multiple formats. The encoder cannot encode with both formats at the same time. The Mediatek HW designers added a new format to an existing encoder by adding a second interface (register set, irq, iommus, clocks) without modifying the original interface. However in the hardware itself there is really just one encoder device. So, although this node looks like it is for two encoder devices (one for each format), really there is just one device that supports each format through its large interface. So, I'm fine with this being a single device node. >> + interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_LOW>, >> + <GIC_SPI 202 IRQ_TYPE_LEVEL_LOW>; >> + mediatek,larb = <&larb3>, >> + <&larb5>; >> + iommus = <&iommu M4U_PORT_VENC_RCPU>, >> + <&iommu M4U_PORT_VENC_REC>, >> + <&iommu M4U_PORT_VENC_BSDMA>, >> + <&iommu M4U_PORT_VENC_SV_COMV>, >> + <&iommu M4U_PORT_VENC_RD_COMV>, >> + <&iommu M4U_PORT_VENC_CUR_LUMA>, >> + <&iommu M4U_PORT_VENC_CUR_CHROMA>, >> + <&iommu M4U_PORT_VENC_REF_LUMA>, >> + <&iommu M4U_PORT_VENC_REF_CHROMA>, >> + <&iommu M4U_PORT_VENC_NBM_RDMA>, >> + <&iommu M4U_PORT_VENC_NBM_WDMA>, >> + <&iommu M4U_PORT_VENC_RCPU_SET2>, >> + <&iommu M4U_PORT_VENC_REC_FRM_SET2>, >> + <&iommu M4U_PORT_VENC_BSDMA_SET2>, >> + <&iommu M4U_PORT_VENC_SV_COMA_SET2>, >> + <&iommu M4U_PORT_VENC_RD_COMA_SET2>, >> + <&iommu M4U_PORT_VENC_CUR_LUMA_SET2>, >> + <&iommu M4U_PORT_VENC_CUR_CHROMA_SET2>, >> + <&iommu M4U_PORT_VENC_REF_LUMA_SET2>, >> + <&iommu M4U_PORT_VENC_REC_CHROMA_SET2>; >> + mediatek,vpu = <&vpu>; >> + clocks = <&topckgen CLK_TOP_VENCPLL_D2>, >> + <&topckgen CLK_TOP_VENC_SEL>, >> + <&topckgen CLK_TOP_UNIVPLL1_D2>, >> + <&topckgen CLK_TOP_VENC_LT_SEL>; >> + clock-names = "vencpll_d2", >> + "venc_sel", >> + "univpll1_d2", >> + "venc_lt_sel"; > > The names of these clocks should be from the perspective of the > encoder, not the clock provider. I still think these clock names should be updated, however. -Dan
Hi Daniel, On Mon, 2016-02-15 at 18:42 +0800, Daniel Kurtz wrote: > On Tue, Feb 9, 2016 at 7:29 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > > Hi Tiffany, > > > > On Thu, Feb 4, 2016 at 7:34 PM, Tiffany Lin <tiffany.lin@mediatek.com> wrote: > >> Add a DT binding documentation of Video Encoder for the > >> MT8173 SoC from Mediatek. > >> > >> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com> > >> --- > >> .../devicetree/bindings/media/mediatek-vcodec.txt | 59 ++++++++++++++++++++ > >> 1 file changed, 59 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt > >> > >> diff --git a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt > >> new file mode 100644 > >> index 0000000..572bfdd > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt > >> @@ -0,0 +1,59 @@ > >> +Mediatek Video Codec > >> + > >> +Mediatek Video Codec is the video codec hw present in Mediatek SoCs which > >> +supports high resolution encoding functionalities. > >> + > >> +Required properties: > >> +- compatible : "mediatek,mt8173-vcodec-enc" for encoder > >> +- reg : Physical base address of the video codec registers and length of > >> + memory mapped region. > >> +- interrupts : interrupt number to the cpu. > >> +- mediatek,larb : must contain the local arbiters in the current Socs. > >> +- clocks : list of clock specifiers, corresponding to entries in > >> + the clock-names property. > >> +- clock-names: encoder must contain "vencpll_d2", "venc_sel", "univpll1_d2", > >> + "venc_lt_sel". > >> +- iommus : should point to the respective IOMMU block with master port as > >> + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > >> + for details. > >> +- mediatek,vpu : the node of video processor unit > >> + > >> +Example: > >> +vcodec_enc: vcodec@0x18002000 { > >> + compatible = "mediatek,mt8173-vcodec-enc"; > >> + reg = <0 0x18002000 0 0x1000>, /*VENC_SYS*/ > >> + <0 0x19002000 0 0x1000>; /*VENC_LT_SYS*/ > > > > This really looks like two encoder devices combined into a single > > device tree node. > > There are two register sets, two irqs, two sets of iommus, and two > > sets of clocks. > > > > If possible, please split this node into two, one for each encoder. > > I chatted offline with Mediatek. They explained that there really is > just one encoder hardware, that happens to support multiple formats. > The encoder cannot encode with both formats at the same time. The > Mediatek HW designers added a new format to an existing encoder by > adding a second interface (register set, irq, iommus, clocks) without > modifying the original interface. However in the hardware itself > there is really just one encoder device. > > So, although this node looks like it is for two encoder devices (one > for each format), really there is just one device that supports each > format through its large interface. > > So, I'm fine with this being a single device node. > > >> + interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_LOW>, > >> + <GIC_SPI 202 IRQ_TYPE_LEVEL_LOW>; > >> + mediatek,larb = <&larb3>, > >> + <&larb5>; > >> + iommus = <&iommu M4U_PORT_VENC_RCPU>, > >> + <&iommu M4U_PORT_VENC_REC>, > >> + <&iommu M4U_PORT_VENC_BSDMA>, > >> + <&iommu M4U_PORT_VENC_SV_COMV>, > >> + <&iommu M4U_PORT_VENC_RD_COMV>, > >> + <&iommu M4U_PORT_VENC_CUR_LUMA>, > >> + <&iommu M4U_PORT_VENC_CUR_CHROMA>, > >> + <&iommu M4U_PORT_VENC_REF_LUMA>, > >> + <&iommu M4U_PORT_VENC_REF_CHROMA>, > >> + <&iommu M4U_PORT_VENC_NBM_RDMA>, > >> + <&iommu M4U_PORT_VENC_NBM_WDMA>, > >> + <&iommu M4U_PORT_VENC_RCPU_SET2>, > >> + <&iommu M4U_PORT_VENC_REC_FRM_SET2>, > >> + <&iommu M4U_PORT_VENC_BSDMA_SET2>, > >> + <&iommu M4U_PORT_VENC_SV_COMA_SET2>, > >> + <&iommu M4U_PORT_VENC_RD_COMA_SET2>, > >> + <&iommu M4U_PORT_VENC_CUR_LUMA_SET2>, > >> + <&iommu M4U_PORT_VENC_CUR_CHROMA_SET2>, > >> + <&iommu M4U_PORT_VENC_REF_LUMA_SET2>, > >> + <&iommu M4U_PORT_VENC_REC_CHROMA_SET2>; > >> + mediatek,vpu = <&vpu>; > >> + clocks = <&topckgen CLK_TOP_VENCPLL_D2>, > >> + <&topckgen CLK_TOP_VENC_SEL>, > >> + <&topckgen CLK_TOP_UNIVPLL1_D2>, > >> + <&topckgen CLK_TOP_VENC_LT_SEL>; > >> + clock-names = "vencpll_d2", > >> + "venc_sel", > >> + "univpll1_d2", > >> + "venc_lt_sel"; > > > > The names of these clocks should be from the perspective of the > > encoder, not the clock provider. > > I still think these clock names should be updated, however. > Got it. We will fix this in next version. > -Dan
On 02/16/2016 07:37 AM, tiffany lin wrote: <snip> >>> +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. <snip> >>> +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, >>> + >>> + .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. > > >>> + .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
On Tue, 2016-02-16 at 08:44 +0100, Hans Verkuil wrote: > On 02/16/2016 07:37 AM, tiffany lin wrote: > > <snip> > > >>> +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. > <snip> > > >>> +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, > >>> + .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. > Got it. We will add these in next version. > > > > > >>> + .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? > 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. > > > >>> + 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 >
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
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.:) > > >>> + }
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
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
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.:) > > > > > >>> + } >
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
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 >
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
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
diff --git a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt new file mode 100644 index 0000000..572bfdd --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt @@ -0,0 +1,59 @@ +Mediatek Video Codec + +Mediatek Video Codec is the video codec hw present in Mediatek SoCs which +supports high resolution encoding functionalities. + +Required properties: +- compatible : "mediatek,mt8173-vcodec-enc" for encoder +- reg : Physical base address of the video codec registers and length of + memory mapped region. +- interrupts : interrupt number to the cpu. +- mediatek,larb : must contain the local arbiters in the current Socs. +- clocks : list of clock specifiers, corresponding to entries in + the clock-names property. +- clock-names: encoder must contain "vencpll_d2", "venc_sel", "univpll1_d2", + "venc_lt_sel". +- iommus : should point to the respective IOMMU block with master port as + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt + for details. +- mediatek,vpu : the node of video processor unit + +Example: +vcodec_enc: vcodec@0x18002000 { + compatible = "mediatek,mt8173-vcodec-enc"; + reg = <0 0x18002000 0 0x1000>, /*VENC_SYS*/ + <0 0x19002000 0 0x1000>; /*VENC_LT_SYS*/ + interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_LOW>, + <GIC_SPI 202 IRQ_TYPE_LEVEL_LOW>; + mediatek,larb = <&larb3>, + <&larb5>; + iommus = <&iommu M4U_PORT_VENC_RCPU>, + <&iommu M4U_PORT_VENC_REC>, + <&iommu M4U_PORT_VENC_BSDMA>, + <&iommu M4U_PORT_VENC_SV_COMV>, + <&iommu M4U_PORT_VENC_RD_COMV>, + <&iommu M4U_PORT_VENC_CUR_LUMA>, + <&iommu M4U_PORT_VENC_CUR_CHROMA>, + <&iommu M4U_PORT_VENC_REF_LUMA>, + <&iommu M4U_PORT_VENC_REF_CHROMA>, + <&iommu M4U_PORT_VENC_NBM_RDMA>, + <&iommu M4U_PORT_VENC_NBM_WDMA>, + <&iommu M4U_PORT_VENC_RCPU_SET2>, + <&iommu M4U_PORT_VENC_REC_FRM_SET2>, + <&iommu M4U_PORT_VENC_BSDMA_SET2>, + <&iommu M4U_PORT_VENC_SV_COMA_SET2>, + <&iommu M4U_PORT_VENC_RD_COMA_SET2>, + <&iommu M4U_PORT_VENC_CUR_LUMA_SET2>, + <&iommu M4U_PORT_VENC_CUR_CHROMA_SET2>, + <&iommu M4U_PORT_VENC_REF_LUMA_SET2>, + <&iommu M4U_PORT_VENC_REC_CHROMA_SET2>; + mediatek,vpu = <&vpu>; + clocks = <&topckgen CLK_TOP_VENCPLL_D2>, + <&topckgen CLK_TOP_VENC_SEL>, + <&topckgen CLK_TOP_UNIVPLL1_D2>, + <&topckgen CLK_TOP_VENC_LT_SEL>; + clock-names = "vencpll_d2", + "venc_sel", + "univpll1_d2", + "venc_lt_sel"; + };
Add a DT binding documentation of Video Encoder for the MT8173 SoC from Mediatek. Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com> --- .../devicetree/bindings/media/mediatek-vcodec.txt | 59 ++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt