On Fri, Aug 03, 2012 at 10:47:01AM +0200, Hans Verkuil wrote:
> On Fri August 3 2012 10:24:43 Richard Zhao wrote:
> > Hi Javier,
> >
> > Glad to see the vpu patch. I'd like to try it on imx6. What else
> > do I need to do besides add vpu devices in dts? Do you have a gst
> > plugin or any other test program to test it?
> >
> > Please also see below comments.
> >
> > On Mon, Jul 23, 2012 at 11:31:01AM +0000, Javier Martin wrote:
> > > Coda is a range of video codecs from Chips&Media that
> > > support H.264, H.263, MPEG4 and other video standards.
> > >
> > > Currently only support for the codadx6 included in the
> > > i.MX27 SoC is added. H.264 and MPEG4 video encoding
> > > are the only supported capabilities by now.
> > >
> > > Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> > > Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > > Reviewed-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> > >
> > > ---
> > > Changes since v6:
> > > - Cosmetic fixes pointed out by Sakari.
> > > - Now passes 'v4l2-compliance'.
> > >
> > > ---
> > > drivers/media/video/Kconfig | 9 +
> > > drivers/media/video/Makefile | 1 +
> > > drivers/media/video/coda.c | 1848 ++++++++++++++++++++++++++++++++++++++++++
> > > drivers/media/video/coda.h | 216 +++++
> > > 4 files changed, 2074 insertions(+)
> > > create mode 100644 drivers/media/video/coda.c
> > > create mode 100644 drivers/media/video/coda.h
> > >
> >
> > [...]
> >
> > > +static int coda_start_streaming(struct vb2_queue *q, unsigned int count)
> > > +{
> > > + struct coda_ctx *ctx = vb2_get_drv_priv(q);
> > > + struct v4l2_device *v4l2_dev = &ctx->dev->v4l2_dev;
> > > + u32 bitstream_buf, bitstream_size;
> > > + struct coda_dev *dev = ctx->dev;
> > > + struct coda_q_data *q_data_src, *q_data_dst;
> > > + u32 dst_fourcc;
> > > + struct vb2_buffer *buf;
> > > + struct vb2_queue *src_vq;
> > > + u32 value;
> > > + int i = 0;
> > > +
> > > + if (count < 1)
> > > + return -EINVAL;
> > > +
> > > + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > + ctx->rawstreamon = 1;
> > > + else
> > > + ctx->compstreamon = 1;
> > > +
> > > + /* Don't start the coda unless both queues are on */
> > > + if (!(ctx->rawstreamon & ctx->compstreamon))
> > > + return 0;
> > > +
> > Remove spaces above.
> > > + ctx->gopcounter = ctx->params.gop_size - 1;
> > > +
> > > + q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > > + buf = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> > > + bitstream_buf = vb2_dma_contig_plane_dma_addr(buf, 0);
> > > + q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > + bitstream_size = q_data_dst->sizeimage;
> > > + dst_fourcc = q_data_dst->fmt->fourcc;
> > > +
> > > + /* Find out whether coda must encode or decode */
> > > + if (q_data_src->fmt->type == CODA_FMT_RAW &&
> > > + q_data_dst->fmt->type == CODA_FMT_ENC) {
> > > + ctx->inst_type = CODA_INST_ENCODER;
> > > + } else if (q_data_src->fmt->type == CODA_FMT_ENC &&
> > > + q_data_dst->fmt->type == CODA_FMT_RAW) {
> > > + ctx->inst_type = CODA_INST_DECODER;
> > > + v4l2_err(v4l2_dev, "decoding not supported.\n");
> > > + return -EINVAL;
> > > + } else {
> > > + v4l2_err(v4l2_dev, "couldn't tell instance type.\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!coda_is_initialized(dev)) {
> > > + v4l2_err(v4l2_dev, "coda is not initialized.\n");
> > > + return -EFAULT;
> > > + }
> > > + coda_write(dev, ctx->parabuf.paddr, CODA_REG_BIT_PARA_BUF_ADDR);
> > > + coda_write(dev, bitstream_buf, CODA_REG_BIT_RD_PTR(ctx->idx));
> > > + coda_write(dev, bitstream_buf, CODA_REG_BIT_WR_PTR(ctx->idx));
> > > + switch (dev->devtype->product) {
> > > + case CODA_DX6:
> > > + coda_write(dev, CODADX6_STREAM_BUF_DYNALLOC_EN |
> > > + CODADX6_STREAM_BUF_PIC_RESET, CODA_REG_BIT_STREAM_CTRL);
> > > + break;
> > > + default:
> > > + coda_write(dev, CODA7_STREAM_BUF_DYNALLOC_EN |
> > > + CODA7_STREAM_BUF_PIC_RESET, CODA_REG_BIT_STREAM_CTRL);
> > > + }
> > > +
> > > + /* Configure the coda */
> > > + coda_write(dev, 0xffff4c00, CODA_REG_BIT_SEARCH_RAM_BASE_ADDR);
> > > +
> > > + /* Could set rotation here if needed */
> > > + switch (dev->devtype->product) {
> > > + case CODA_DX6:
> > > + value = (q_data_src->width & CODADX6_PICWIDTH_MASK) << CODADX6_PICWIDTH_OFFSET;
> > longer than 80 characters. Could you run checkpatch to do further check?
>
> This is a checkpatch warning, not an error. One should use one's own judgement whether
> to take action or not. It is more important that the code is readable than that it fits
> within 80 characters, although long lines can be an indication of poor readability.
Yes, you are right. I don't insist but I like cut it off except for
files like register definition with aligned lines.
>
> In this case I personally don't think it will be easier to read if this line is split up.
My point here is checkpatch.
total: 2 errors, 30 warnings, 2086 lines checked
Thanks
Richard
>
> Regards,
>
> Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
