Re: [v7] media: coda: Add driver for Coda video codec.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux