diff mbox

[06/15,media] marvell-ccic: add new formats support for marvell-ccic driver

Message ID 1353677621-24143-1-git-send-email-twang13@marvell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Albert Wang Nov. 23, 2012, 1:33 p.m. UTC 
From: Libin Yang <lbyang@marvell.com>

This patch adds the new formats support for marvell-ccic.

Signed-off-by: Albert Wang <twang13@marvell.com>
Signed-off-by: Libin Yang <lbyang@marvell.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c |  178 ++++++++++++++++++-----
 drivers/media/platform/marvell-ccic/mcam-core.h |    6 +
 2 files changed, 151 insertions(+), 33 deletions(-)

Comments

Guennadi Liakhovetski Nov. 27, 2012, 11:44 a.m. UTC | #1 
On Fri, 23 Nov 2012, Albert Wang wrote:

> From: Libin Yang <lbyang@marvell.com>
> 
> This patch adds the new formats support for marvell-ccic.
> 
> Signed-off-by: Albert Wang <twang13@marvell.com>
> Signed-off-by: Libin Yang <lbyang@marvell.com>
> ---
>  drivers/media/platform/marvell-ccic/mcam-core.c |  178 ++++++++++++++++++-----
>  drivers/media/platform/marvell-ccic/mcam-core.h |    6 +
>  2 files changed, 151 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
> index 67d4f2f..c9f7250 100755
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -110,6 +110,30 @@ static struct mcam_format_struct {
>  		.bpp		= 2,
>  	},
>  	{
> +		.desc		= "UYVY 4:2:2",
> +		.pixelformat	= V4L2_PIX_FMT_UYVY,
> +		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
> +		.bpp		= 2,
> +	},
> +	{
> +		.desc		= "YUV 4:2:2 PLANAR",
> +		.pixelformat	= V4L2_PIX_FMT_YUV422P,
> +		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
> +		.bpp		= 2,
> +	},
> +	{
> +		.desc		= "YUV 4:2:0 PLANAR",
> +		.pixelformat	= V4L2_PIX_FMT_YUV420,
> +		.mbus_code	= V4L2_MBUS_FMT_YUYV8_1_5X8,
> +		.bpp		= 2,
> +	},
> +	{
> +		.desc		= "YVU 4:2:0 PLANAR",
> +		.pixelformat	= V4L2_PIX_FMT_YVU420,
> +		.mbus_code	= V4L2_MBUS_FMT_YVYU8_1_5X8,
> +		.bpp		= 2,
> +	},
> +	{
>  		.desc		= "RGB 444",
>  		.pixelformat	= V4L2_PIX_FMT_RGB444,
>  		.mbus_code	= V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
> @@ -168,6 +192,12 @@ struct mcam_dma_desc {
>  	u32 segment_len;
>  };
>  
> +struct yuv_pointer_t {
> +	dma_addr_t y;
> +	dma_addr_t u;
> +	dma_addr_t v;
> +};
> +
>  /*
>   * Our buffer type for working with videobuf2.  Note that the vb2
>   * developers have decreed that struct vb2_buffer must be at the
> @@ -179,6 +209,7 @@ struct mcam_vb_buffer {
>  	struct mcam_dma_desc *dma_desc;	/* Descriptor virtual address */
>  	dma_addr_t dma_desc_pa;		/* Descriptor physical address */
>  	int dma_desc_nent;		/* Number of mapped descriptors */
> +	struct yuv_pointer_t yuv_p;
>  };
>  
>  static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
> @@ -465,6 +496,18 @@ static inline int mcam_check_dma_buffers(struct mcam_camera *cam)
>  /*
>   * DMA-contiguous code.
>   */
> +
> +static bool mcam_fmt_is_planar(__u32 pfmt)
> +{
> +	switch (pfmt) {
> +	case V4L2_PIX_FMT_YUV422P:
> +	case V4L2_PIX_FMT_YUV420:
> +	case V4L2_PIX_FMT_YVU420:
> +		return true;
> +	}
> +	return false;
> +}
> +
>  /*
>   * Set up a contiguous buffer for the given frame.  Here also is where
>   * the underrun strategy is set: if there is no buffer available, reuse
> @@ -476,6 +519,8 @@ static inline int mcam_check_dma_buffers(struct mcam_camera *cam)
>  static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
>  {
>  	struct mcam_vb_buffer *buf;
> +	struct v4l2_pix_format *fmt = &cam->pix_format;
> +
>  	/*
>  	 * If there are no available buffers, go into single mode
>  	 */
> @@ -494,8 +539,13 @@ static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
>  	}
>  
>  	cam->vb_bufs[frame] = buf;
> -	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
> -			vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
> +	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR, buf->yuv_p.y);
> +	if (mcam_fmt_is_planar(fmt->pixelformat)) {
> +		mcam_reg_write(cam, frame == 0 ?
> +					REG_U0BAR : REG_U1BAR, buf->yuv_p.u);
> +		mcam_reg_write(cam, frame == 0 ?
> +					REG_V0BAR : REG_V1BAR, buf->yuv_p.v);
> +	}
>  }
>  
>  /*
> @@ -653,49 +703,91 @@ static inline void mcam_sg_restart(struct mcam_camera *cam)
>   */
>  static void mcam_ctlr_image(struct mcam_camera *cam)
>  {
> -	int imgsz;
>  	struct v4l2_pix_format *fmt = &cam->pix_format;
> +	u32 widthy = 0, widthuv = 0, imgsz_h, imgsz_w;
> +
> +	cam_dbg(cam, "camera: bytesperline = %d; height = %d\n",
> +		fmt->bytesperline, fmt->sizeimage / fmt->bytesperline);
> +	imgsz_h = (fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK;
> +	imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
> +
> +	if (fmt->pixelformat == V4L2_PIX_FMT_YUV420
> +		|| fmt->pixelformat == V4L2_PIX_FMT_YVU420)
> +		imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
> +	else if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> +		imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
> +
> +	switch (fmt->pixelformat) {
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_UYVY:
> +		widthy = fmt->width * 2;
> +		widthuv = fmt->width * 2;
> +		break;

I think, you can move imgsz_h and imgsz_w calculations from above to this 
switch too?

> +	case V4L2_PIX_FMT_RGB565:
> +		widthy = fmt->width * 2;
> +		widthuv = 0;
> +		break;
> +	case V4L2_PIX_FMT_JPEG:
> +		widthy = fmt->bytesperline;
> +		widthuv = fmt->bytesperline;
> +		break;
> +	case V4L2_PIX_FMT_YUV422P:
> +	case V4L2_PIX_FMT_YUV420:
> +	case V4L2_PIX_FMT_YVU420:
> +		widthy = fmt->width;
> +		widthuv = fmt->width / 2;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	mcam_reg_write_mask(cam, REG_IMGPITCH, widthuv << 16 | widthy,
> +			IMGP_YP_MASK | IMGP_UVP_MASK);

Hm, are you sure you're not breaking other currently supported pixel 
formats, like V4L2_PIX_FMT_RGB444, V4L2_PIX_FMT_SBGGR8?

> +	mcam_reg_write(cam, REG_IMGSIZE, imgsz_h | imgsz_w);
> +	mcam_reg_write(cam, REG_IMGOFFSET, 0x0);
>  
> -	imgsz = ((fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK) |
> -		(fmt->bytesperline & IMGSZ_H_MASK);
> -	mcam_reg_write(cam, REG_IMGSIZE, imgsz);
> -	mcam_reg_write(cam, REG_IMGOFFSET, 0);
> -	/* YPITCH just drops the last two bits */
> -	mcam_reg_write_mask(cam, REG_IMGPITCH, fmt->bytesperline,
> -			IMGP_YP_MASK);

Let's see. Before we had

	REG_IMGPITCH = fmt->bytesperline & 0x3ffc;

after your change, say, for V4L2_PIX_FMT_YUYV (fmt->bytesperline = 
fmt->width * 2), you get

	REG_IMGPITCH = ((fmt->bytesperline << 16) | fmt->bytesperline) &
		0x3ffc3ffc;

Is this intentional? Doesn't widthuv have to be 0 for non-planar formats 
like V4L2_PIX_FMT_YUYV? Please, always when you change existing code make 
sure the original behaviour is preserved, unless there used to be a bug, 
in which case it should be fixed by a separate patch.

>  	/*
>  	 * Tell the controller about the image format we are using.
>  	 */
> -	switch (cam->pix_format.pixelformat) {
> +	switch (fmt->pixelformat) {
> +	case V4L2_PIX_FMT_YUV422P:
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_YUV | C0_YUV_PLANAR | C0_YUVE_YVYU, C0_DF_MASK);
> +		break;
> +	case V4L2_PIX_FMT_YUV420:
> +	case V4L2_PIX_FMT_YVU420:
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_YUV | C0_YUV_420PL | C0_YUVE_YVYU, C0_DF_MASK);
> +		break;
>  	case V4L2_PIX_FMT_YUYV:
> -	    mcam_reg_write_mask(cam, REG_CTRL0,
> -			    C0_DF_YUV|C0_YUV_PACKED|C0_YUVE_YUYV,
> -			    C0_DF_MASK);
> -	    break;
> -
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_UYVY, C0_DF_MASK);
> +		break;
> +	case V4L2_PIX_FMT_UYVY:
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV, C0_DF_MASK);
> +		break;
> +	case V4L2_PIX_FMT_JPEG:
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV, C0_DF_MASK);
> +		break;
>  	case V4L2_PIX_FMT_RGB444:
> -	    mcam_reg_write_mask(cam, REG_CTRL0,
> -			    C0_DF_RGB|C0_RGBF_444|C0_RGB4_XRGB,
> -			    C0_DF_MASK);
> -		/* Alpha value? */

Unless you have a specific reason, you could just as well preserve the 
comment.

> -	    break;
> -
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XRGB, C0_DF_MASK);
> +		break;
>  	case V4L2_PIX_FMT_RGB565:
> -	    mcam_reg_write_mask(cam, REG_CTRL0,
> -			    C0_DF_RGB|C0_RGBF_565|C0_RGB5_BGGR,
> -			    C0_DF_MASK);
> -	    break;
> -
> +		mcam_reg_write_mask(cam, REG_CTRL0,
> +			C0_DF_RGB | C0_RGBF_565 | C0_RGB5_BGGR, C0_DF_MASK);
> +		break;
>  	default:
> -	    cam_err(cam, "Unknown format %x\n", cam->pix_format.pixelformat);
> -	    break;
> +		cam_err(cam, "camera: unknown format: %c\n", fmt->pixelformat);

Don't think "%c" will produce a meaningful result here.

> +		break;
>  	}
> +
>  	/*
>  	 * Make sure it knows we want to use hsync/vsync.
>  	 */
> -	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC,
> -			C0_SIFM_MASK);
> -
> +	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK);
>  	/*
>  	 * This field controls the generation of EOF(DVP only)
>  	 */
> @@ -706,7 +798,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
>  	}
>  }
>  
> -
>  /*
>   * Configure the controller for operation; caller holds the
>   * device mutex.
> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer *vb)
>  {
>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
> +	struct v4l2_pix_format *fmt = &cam->pix_format;
>  	unsigned long flags;
>  	int start;
> +	dma_addr_t dma_handle;
> +	u32 base_size = fmt->width * fmt->height;

Shouldn't you be just using bytesperline? Is stride != width * height 
supported?

>  
>  	spin_lock_irqsave(&cam->dev_lock, flags);
> +	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
> +	BUG_ON(!dma_handle);
>  	start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
> +
> +	if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV422P) {

Better use a "switch."

> +		mvb->yuv_p.y = dma_handle;
> +		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
> +		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 2;
> +	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV420) {
> +		mvb->yuv_p.y = dma_handle;
> +		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
> +		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 4;
> +	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YVU420) {
> +		mvb->yuv_p.y = dma_handle;
> +		mvb->yuv_p.v = mvb->yuv_p.y + base_size;
> +		mvb->yuv_p.u = mvb->yuv_p.v + base_size / 4;
> +	} else
> +		mvb->yuv_p.y = dma_handle;
> +
>  	list_add(&mvb->queue, &cam->buffers);
>  	if (cam->state == S_STREAMING && test_bit(CF_SG_RESTART, &cam->flags))
>  		mcam_sg_restart(cam);
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
> index 40368f6..3f75d7d 100755
> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
> @@ -233,6 +233,12 @@ int mccic_resume(struct mcam_camera *cam);
>  #define REG_Y0BAR	0x00
>  #define REG_Y1BAR	0x04
>  #define REG_Y2BAR	0x08
> +#define REG_U0BAR	0x0c
> +#define REG_U1BAR	0x10
> +#define REG_U2BAR	0x14
> +#define REG_V0BAR	0x18
> +#define REG_V1BAR	0x1C
> +#define REG_V2BAR	0x20
>  
>  /*
>   * register definitions for MIPI support
> -- 
> 1.7.9.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Albert Wang Nov. 27, 2012, 12:09 p.m. UTC | #2 
Thanks
Albert Wang
86-21-61092656


>-----Original Message-----
>From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de]
>Sent: Tuesday, 27 November, 2012 19:45
>To: Albert Wang
>Cc: corbet@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH 06/15] [media] marvell-ccic: add new formats support for marvell-ccic
>driver
>
>On Fri, 23 Nov 2012, Albert Wang wrote:
>
>> From: Libin Yang <lbyang@marvell.com>
>>
>> This patch adds the new formats support for marvell-ccic.
>>
>> Signed-off-by: Albert Wang <twang13@marvell.com>
>> Signed-off-by: Libin Yang <lbyang@marvell.com>
>> ---
>>  drivers/media/platform/marvell-ccic/mcam-core.c |  178 ++++++++++++++++++-----
>>  drivers/media/platform/marvell-ccic/mcam-core.h |    6 +
>>  2 files changed, 151 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
>> b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 67d4f2f..c9f7250 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>> @@ -110,6 +110,30 @@ static struct mcam_format_struct {
>>  		.bpp		= 2,
>>  	},
>>  	{
>> +		.desc		= "UYVY 4:2:2",
>> +		.pixelformat	= V4L2_PIX_FMT_UYVY,
>> +		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
>> +		.bpp		= 2,
>> +	},
>> +	{
>> +		.desc		= "YUV 4:2:2 PLANAR",
>> +		.pixelformat	= V4L2_PIX_FMT_YUV422P,
>> +		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
>> +		.bpp		= 2,
>> +	},
>> +	{
>> +		.desc		= "YUV 4:2:0 PLANAR",
>> +		.pixelformat	= V4L2_PIX_FMT_YUV420,
>> +		.mbus_code	= V4L2_MBUS_FMT_YUYV8_1_5X8,
>> +		.bpp		= 2,
>> +	},
>> +	{
>> +		.desc		= "YVU 4:2:0 PLANAR",
>> +		.pixelformat	= V4L2_PIX_FMT_YVU420,
>> +		.mbus_code	= V4L2_MBUS_FMT_YVYU8_1_5X8,
>> +		.bpp		= 2,
>> +	},
>> +	{
>>  		.desc		= "RGB 444",
>>  		.pixelformat	= V4L2_PIX_FMT_RGB444,
>>  		.mbus_code	= V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
>> @@ -168,6 +192,12 @@ struct mcam_dma_desc {
>>  	u32 segment_len;
>>  };
>>
>> +struct yuv_pointer_t {
>> +	dma_addr_t y;
>> +	dma_addr_t u;
>> +	dma_addr_t v;
>> +};
>> +
>>  /*
>>   * Our buffer type for working with videobuf2.  Note that the vb2
>>   * developers have decreed that struct vb2_buffer must be at the @@
>> -179,6 +209,7 @@ struct mcam_vb_buffer {
>>  	struct mcam_dma_desc *dma_desc;	/* Descriptor virtual address */
>>  	dma_addr_t dma_desc_pa;		/* Descriptor physical address */
>>  	int dma_desc_nent;		/* Number of mapped descriptors */
>> +	struct yuv_pointer_t yuv_p;
>>  };
>>
>>  static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
>> @@ -465,6 +496,18 @@ static inline int mcam_check_dma_buffers(struct
>> mcam_camera *cam)
>>  /*
>>   * DMA-contiguous code.
>>   */
>> +
>> +static bool mcam_fmt_is_planar(__u32 pfmt) {
>> +	switch (pfmt) {
>> +	case V4L2_PIX_FMT_YUV422P:
>> +	case V4L2_PIX_FMT_YUV420:
>> +	case V4L2_PIX_FMT_YVU420:
>> +		return true;
>> +	}
>> +	return false;
>> +}
>> +
>>  /*
>>   * Set up a contiguous buffer for the given frame.  Here also is where
>>   * the underrun strategy is set: if there is no buffer available,
>> reuse @@ -476,6 +519,8 @@ static inline int
>> mcam_check_dma_buffers(struct mcam_camera *cam)  static void
>> mcam_set_contig_buffer(struct mcam_camera *cam, int frame)  {
>>  	struct mcam_vb_buffer *buf;
>> +	struct v4l2_pix_format *fmt = &cam->pix_format;
>> +
>>  	/*
>>  	 * If there are no available buffers, go into single mode
>>  	 */
>> @@ -494,8 +539,13 @@ static void mcam_set_contig_buffer(struct mcam_camera *cam,
>int frame)
>>  	}
>>
>>  	cam->vb_bufs[frame] = buf;
>> -	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
>> -			vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
>> +	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR, buf->yuv_p.y);
>> +	if (mcam_fmt_is_planar(fmt->pixelformat)) {
>> +		mcam_reg_write(cam, frame == 0 ?
>> +					REG_U0BAR : REG_U1BAR, buf->yuv_p.u);
>> +		mcam_reg_write(cam, frame == 0 ?
>> +					REG_V0BAR : REG_V1BAR, buf->yuv_p.v);
>> +	}
>>  }
>>
>>  /*
>> @@ -653,49 +703,91 @@ static inline void mcam_sg_restart(struct mcam_camera
>*cam)
>>   */
>>  static void mcam_ctlr_image(struct mcam_camera *cam)  {
>> -	int imgsz;
>>  	struct v4l2_pix_format *fmt = &cam->pix_format;
>> +	u32 widthy = 0, widthuv = 0, imgsz_h, imgsz_w;
>> +
>> +	cam_dbg(cam, "camera: bytesperline = %d; height = %d\n",
>> +		fmt->bytesperline, fmt->sizeimage / fmt->bytesperline);
>> +	imgsz_h = (fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK;
>> +	imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>> +
>> +	if (fmt->pixelformat == V4L2_PIX_FMT_YUV420
>> +		|| fmt->pixelformat == V4L2_PIX_FMT_YVU420)
>> +		imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
>> +	else if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
>> +		imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
>> +
>> +	switch (fmt->pixelformat) {
>> +	case V4L2_PIX_FMT_YUYV:
>> +	case V4L2_PIX_FMT_UYVY:
>> +		widthy = fmt->width * 2;
>> +		widthuv = fmt->width * 2;
>> +		break;
>
>I think, you can move imgsz_h and imgsz_w calculations from above to this switch too?
>
Sure, we can do it.

>> +	case V4L2_PIX_FMT_RGB565:
>> +		widthy = fmt->width * 2;
>> +		widthuv = 0;
>> +		break;
>> +	case V4L2_PIX_FMT_JPEG:
>> +		widthy = fmt->bytesperline;
>> +		widthuv = fmt->bytesperline;
>> +		break;
>> +	case V4L2_PIX_FMT_YUV422P:
>> +	case V4L2_PIX_FMT_YUV420:
>> +	case V4L2_PIX_FMT_YVU420:
>> +		widthy = fmt->width;
>> +		widthuv = fmt->width / 2;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	mcam_reg_write_mask(cam, REG_IMGPITCH, widthuv << 16 | widthy,
>> +			IMGP_YP_MASK | IMGP_UVP_MASK);
>
>Hm, are you sure you're not breaking other currently supported pixel formats, like
>V4L2_PIX_FMT_RGB444, V4L2_PIX_FMT_SBGGR8?
>
>> +	mcam_reg_write(cam, REG_IMGSIZE, imgsz_h | imgsz_w);
>> +	mcam_reg_write(cam, REG_IMGOFFSET, 0x0);
>>
>> -	imgsz = ((fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK) |
>> -		(fmt->bytesperline & IMGSZ_H_MASK);
>> -	mcam_reg_write(cam, REG_IMGSIZE, imgsz);
>> -	mcam_reg_write(cam, REG_IMGOFFSET, 0);
>> -	/* YPITCH just drops the last two bits */
>> -	mcam_reg_write_mask(cam, REG_IMGPITCH, fmt->bytesperline,
>> -			IMGP_YP_MASK);
>
>Let's see. Before we had
>
>	REG_IMGPITCH = fmt->bytesperline & 0x3ffc;
>
>after your change, say, for V4L2_PIX_FMT_YUYV (fmt->bytesperline =
>fmt->width * 2), you get
>
>	REG_IMGPITCH = ((fmt->bytesperline << 16) | fmt->bytesperline) &
>		0x3ffc3ffc;
>
>Is this intentional? Doesn't widthuv have to be 0 for non-planar formats like
>V4L2_PIX_FMT_YUYV? Please, always when you change existing code make sure the
>original behaviour is preserved, unless there used to be a bug, in which case it should be
>fixed by a separate patch.
>
I see. We will re-check this case carefully, and then give a update.
Thanks for point it out.

>>  	/*
>>  	 * Tell the controller about the image format we are using.
>>  	 */
>> -	switch (cam->pix_format.pixelformat) {
>> +	switch (fmt->pixelformat) {
>> +	case V4L2_PIX_FMT_YUV422P:
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_YUV | C0_YUV_PLANAR | C0_YUVE_YVYU,
>C0_DF_MASK);
>> +		break;
>> +	case V4L2_PIX_FMT_YUV420:
>> +	case V4L2_PIX_FMT_YVU420:
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_YUV | C0_YUV_420PL | C0_YUVE_YVYU,
>C0_DF_MASK);
>> +		break;
>>  	case V4L2_PIX_FMT_YUYV:
>> -	    mcam_reg_write_mask(cam, REG_CTRL0,
>> -			    C0_DF_YUV|C0_YUV_PACKED|C0_YUVE_YUYV,
>> -			    C0_DF_MASK);
>> -	    break;
>> -
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_UYVY,
>C0_DF_MASK);
>> +		break;
>> +	case V4L2_PIX_FMT_UYVY:
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV,
>C0_DF_MASK);
>> +		break;
>> +	case V4L2_PIX_FMT_JPEG:
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV,
>C0_DF_MASK);
>> +		break;
>>  	case V4L2_PIX_FMT_RGB444:
>> -	    mcam_reg_write_mask(cam, REG_CTRL0,
>> -			    C0_DF_RGB|C0_RGBF_444|C0_RGB4_XRGB,
>> -			    C0_DF_MASK);
>> -		/* Alpha value? */
>
>Unless you have a specific reason, you could just as well preserve the comment.
>
OK. We can keep this comments, just adjust the code format for aligning with others.

>> -	    break;
>> -
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XRGB, C0_DF_MASK);
>> +		break;
>>  	case V4L2_PIX_FMT_RGB565:
>> -	    mcam_reg_write_mask(cam, REG_CTRL0,
>> -			    C0_DF_RGB|C0_RGBF_565|C0_RGB5_BGGR,
>> -			    C0_DF_MASK);
>> -	    break;
>> -
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_RGB | C0_RGBF_565 | C0_RGB5_BGGR,
>C0_DF_MASK);
>> +		break;
>>  	default:
>> -	    cam_err(cam, "Unknown format %x\n", cam->pix_format.pixelformat);
>> -	    break;
>> +		cam_err(cam, "camera: unknown format: %c\n", fmt->pixelformat);
>
>Don't think "%c" will produce a meaningful result here.
>
Sorry, should be %u or %x.
>> +		break;
>>  	}
>> +
>>  	/*
>>  	 * Make sure it knows we want to use hsync/vsync.
>>  	 */
>> -	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC,
>> -			C0_SIFM_MASK);
>> -
>> +	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK);
>>  	/*
>>  	 * This field controls the generation of EOF(DVP only)
>>  	 */
>> @@ -706,7 +798,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
>>  	}
>>  }
>>
>> -
>>  /*
>>   * Configure the controller for operation; caller holds the
>>   * device mutex.
>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
>> *vb)  {
>>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct v4l2_pix_format *fmt = &cam->pix_format;
>>  	unsigned long flags;
>>  	int start;
>> +	dma_addr_t dma_handle;
>> +	u32 base_size = fmt->width * fmt->height;
>
>Shouldn't you be just using bytesperline? Is stride != width * height supported?
>
We will update it.
>>
>>  	spin_lock_irqsave(&cam->dev_lock, flags);
>> +	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
>> +	BUG_ON(!dma_handle);
>>  	start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
>> +
>> +	if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV422P) {
>
>Better use a "switch."
>
OK.

>> +		mvb->yuv_p.y = dma_handle;
>> +		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
>> +		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 2;
>> +	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV420) {
>> +		mvb->yuv_p.y = dma_handle;
>> +		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
>> +		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 4;
>> +	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YVU420) {
>> +		mvb->yuv_p.y = dma_handle;
>> +		mvb->yuv_p.v = mvb->yuv_p.y + base_size;
>> +		mvb->yuv_p.u = mvb->yuv_p.v + base_size / 4;
>> +	} else
>> +		mvb->yuv_p.y = dma_handle;
>> +
>>  	list_add(&mvb->queue, &cam->buffers);
>>  	if (cam->state == S_STREAMING && test_bit(CF_SG_RESTART, &cam->flags))
>>  		mcam_sg_restart(cam);
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
>> b/drivers/media/platform/marvell-ccic/mcam-core.h
>> index 40368f6..3f75d7d 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
>> @@ -233,6 +233,12 @@ int mccic_resume(struct mcam_camera *cam);
>>  #define REG_Y0BAR	0x00
>>  #define REG_Y1BAR	0x04
>>  #define REG_Y2BAR	0x08
>> +#define REG_U0BAR	0x0c
>> +#define REG_U1BAR	0x10
>> +#define REG_U2BAR	0x14
>> +#define REG_V0BAR	0x18
>> +#define REG_V1BAR	0x1C
>> +#define REG_V2BAR	0x20
>>
>>  /*
>>   * register definitions for MIPI support
>> --
>> 1.7.9.5
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Libin Yang Dec. 4, 2012, 2:58 a.m. UTC | #3 
Hi Guennadi,

When I'm refining the patch based on your comments, I met an issue. Could you please help me?

[snip]


>>> -
>>>  /*
>>>   * Configure the controller for operation; caller holds the
>>>   * device mutex.
>>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
>>> *vb)  {
>>>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>>>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>>> +	struct v4l2_pix_format *fmt = &cam->pix_format;
>>>  	unsigned long flags;
>>>  	int start;
>>> +	dma_addr_t dma_handle;
>>> +	u32 base_size = fmt->width * fmt->height;
>>
>>Shouldn't you be just using bytesperline? Is stride != width * height supported?
>>
>We will update it.

[Libin] What I understand is width is the pixel number perline, so bytesperline = width * BytePerPixel. This means bytesperline should include Y data and UV data. 

For example, for yuv420 legacy 8-bit, it transfers like below:
U Y Y U Y Y U Y Y U Y Y ......
V Y Y V Y Y V Y Y V Y Y ......

As each Y is one byte, so all Y data length is nuber_Y_per_line * height. While the nuber_Y_per_line is the pixel_per_line, which is fmt->width.

So for the planner, the first block is saving the Y data, whose length is nuber_Y_per_line * height, which equals to fmt->width * height.

Do I understand correctly?

The base_size here means the size of Y data, so it should be fmt->width * fmt->height.

>>>
>>>  	spin_lock_irqsave(&cam->dev_lock, flags);
>>> +	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);

Best Regards,
Libin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Dec. 4, 2012, 6:56 a.m. UTC | #4 
Hi Libin

On Mon, 3 Dec 2012, Libin Yang wrote:

> Hi Guennadi,
> 
> When I'm refining the patch based on your comments, I met an issue. 
> Could you please help me?
> 
> [snip]
> 
> 
> >>> -
> >>>  /*
> >>>   * Configure the controller for operation; caller holds the
> >>>   * device mutex.
> >>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
> >>> *vb)  {
> >>>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
> >>>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
> >>> +	struct v4l2_pix_format *fmt = &cam->pix_format;
> >>>  	unsigned long flags;
> >>>  	int start;
> >>> +	dma_addr_t dma_handle;
> >>> +	u32 base_size = fmt->width * fmt->height;
> >>
> >>Shouldn't you be just using bytesperline? Is stride != width * height supported?
> >>
> >We will update it.
> 
> [Libin] What I understand is width is the pixel number perline, so 
> bytesperline = width * BytePerPixel. This means bytesperline should 
> include Y data and UV data.
> 
> For example, for yuv420 legacy 8-bit, it transfers like below:
> U Y Y U Y Y U Y Y U Y Y ......
> V Y Y V Y Y V Y Y V Y Y ......
> 
> As each Y is one byte, so all Y data length is nuber_Y_per_line * 
> height. While the nuber_Y_per_line is the pixel_per_line, which is 
> fmt->width.
> 
> So for the planner, the first block is saving the Y data, whose length 
> is nuber_Y_per_line * height, which equals to fmt->width * height.
> 
> Do I understand correctly?
> 
> The base_size here means the size of Y data, so it should be fmt->width 
> * fmt->height.

Right, in this case you're right. Sorry for a misleading comment. Just a 
suggestion, if you prefer, you can keep the name, but maybe a name like 
pixel_count or pixel_num or pixel_per_frame would be clearer for that 
variable? But keeping the name is also ok with me.

> >>>
> >>>  	spin_lock_irqsave(&cam->dev_lock, flags);
> >>> +	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Libin Yang Dec. 4, 2012, 7:14 a.m. UTC | #5 
Hi Guennadi,

>-----Original Message-----
>From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de]
>Sent: Tuesday, December 04, 2012 2:57 PM
>To: Libin Yang
>Cc: corbet@lwn.net; linux-media@vger.kernel.org
>Subject: RE: [PATCH 06/15] [media] marvell-ccic: add new formats support for marvell-ccic
>driver
>
>Hi Libin
>
>On Mon, 3 Dec 2012, Libin Yang wrote:
>
>> Hi Guennadi,
>>
>> When I'm refining the patch based on your comments, I met an issue.
>> Could you please help me?
>>
>> [snip]
>>
>>
>> >>> -
>> >>>  /*
>> >>>   * Configure the controller for operation; caller holds the
>> >>>   * device mutex.
>> >>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
>> >>> *vb)  {
>> >>>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>> >>>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>> >>> +	struct v4l2_pix_format *fmt = &cam->pix_format;
>> >>>  	unsigned long flags;
>> >>>  	int start;
>> >>> +	dma_addr_t dma_handle;
>> >>> +	u32 base_size = fmt->width * fmt->height;
>> >>
>> >>Shouldn't you be just using bytesperline? Is stride != width * height supported?
>> >>
>> >We will update it.
>>
>> [Libin] What I understand is width is the pixel number perline, so
>> bytesperline = width * BytePerPixel. This means bytesperline should
>> include Y data and UV data.
>>
>> For example, for yuv420 legacy 8-bit, it transfers like below:
>> U Y Y U Y Y U Y Y U Y Y ......
>> V Y Y V Y Y V Y Y V Y Y ......
>>
>> As each Y is one byte, so all Y data length is nuber_Y_per_line *
>> height. While the nuber_Y_per_line is the pixel_per_line, which is
>> fmt->width.
>>
>> So for the planner, the first block is saving the Y data, whose length
>> is nuber_Y_per_line * height, which equals to fmt->width * height.
>>
>> Do I understand correctly?
>>
>> The base_size here means the size of Y data, so it should be fmt->width
>> * fmt->height.
>
>Right, in this case you're right. Sorry for a misleading comment. Just a
>suggestion, if you prefer, you can keep the name, but maybe a name like
>pixel_count or pixel_num or pixel_per_frame would be clearer for that
>variable? But keeping the name is also ok with me.
>
[Libin] OK, I see. Thanks for your suggestion. The name really seems a little 
misleading. I will use a more reasonable name in the coming patch.

>> >>>
>> >>>  	spin_lock_irqsave(&cam->dev_lock, flags);
>> >>> +	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer
>http://www.open-technology.de/

Regards,
Libin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 67d4f2f..c9f7250 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -110,6 +110,30 @@  static struct mcam_format_struct {
 		.bpp		= 2,
 	},
 	{
+		.desc		= "UYVY 4:2:2",
+		.pixelformat	= V4L2_PIX_FMT_UYVY,
+		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
+		.bpp		= 2,
+	},
+	{
+		.desc		= "YUV 4:2:2 PLANAR",
+		.pixelformat	= V4L2_PIX_FMT_YUV422P,
+		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
+		.bpp		= 2,
+	},
+	{
+		.desc		= "YUV 4:2:0 PLANAR",
+		.pixelformat	= V4L2_PIX_FMT_YUV420,
+		.mbus_code	= V4L2_MBUS_FMT_YUYV8_1_5X8,
+		.bpp		= 2,
+	},
+	{
+		.desc		= "YVU 4:2:0 PLANAR",
+		.pixelformat	= V4L2_PIX_FMT_YVU420,
+		.mbus_code	= V4L2_MBUS_FMT_YVYU8_1_5X8,
+		.bpp		= 2,
+	},
+	{
 		.desc		= "RGB 444",
 		.pixelformat	= V4L2_PIX_FMT_RGB444,
 		.mbus_code	= V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
@@ -168,6 +192,12 @@  struct mcam_dma_desc {
 	u32 segment_len;
 };
 
+struct yuv_pointer_t {
+	dma_addr_t y;
+	dma_addr_t u;
+	dma_addr_t v;
+};
+
 /*
  * Our buffer type for working with videobuf2.  Note that the vb2
  * developers have decreed that struct vb2_buffer must be at the
@@ -179,6 +209,7 @@  struct mcam_vb_buffer {
 	struct mcam_dma_desc *dma_desc;	/* Descriptor virtual address */
 	dma_addr_t dma_desc_pa;		/* Descriptor physical address */
 	int dma_desc_nent;		/* Number of mapped descriptors */
+	struct yuv_pointer_t yuv_p;
 };
 
 static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
@@ -465,6 +496,18 @@  static inline int mcam_check_dma_buffers(struct mcam_camera *cam)
 /*
  * DMA-contiguous code.
  */
+
+static bool mcam_fmt_is_planar(__u32 pfmt)
+{
+	switch (pfmt) {
+	case V4L2_PIX_FMT_YUV422P:
+	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YVU420:
+		return true;
+	}
+	return false;
+}
+
 /*
  * Set up a contiguous buffer for the given frame.  Here also is where
  * the underrun strategy is set: if there is no buffer available, reuse
@@ -476,6 +519,8 @@  static inline int mcam_check_dma_buffers(struct mcam_camera *cam)
 static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
 {
 	struct mcam_vb_buffer *buf;
+	struct v4l2_pix_format *fmt = &cam->pix_format;
+
 	/*
 	 * If there are no available buffers, go into single mode
 	 */
@@ -494,8 +539,13 @@  static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
 	}
 
 	cam->vb_bufs[frame] = buf;
-	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
-			vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
+	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR, buf->yuv_p.y);
+	if (mcam_fmt_is_planar(fmt->pixelformat)) {
+		mcam_reg_write(cam, frame == 0 ?
+					REG_U0BAR : REG_U1BAR, buf->yuv_p.u);
+		mcam_reg_write(cam, frame == 0 ?
+					REG_V0BAR : REG_V1BAR, buf->yuv_p.v);
+	}
 }
 
 /*
@@ -653,49 +703,91 @@  static inline void mcam_sg_restart(struct mcam_camera *cam)
  */
 static void mcam_ctlr_image(struct mcam_camera *cam)
 {
-	int imgsz;
 	struct v4l2_pix_format *fmt = &cam->pix_format;
+	u32 widthy = 0, widthuv = 0, imgsz_h, imgsz_w;
+
+	cam_dbg(cam, "camera: bytesperline = %d; height = %d\n",
+		fmt->bytesperline, fmt->sizeimage / fmt->bytesperline);
+	imgsz_h = (fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK;
+	imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
+
+	if (fmt->pixelformat == V4L2_PIX_FMT_YUV420
+		|| fmt->pixelformat == V4L2_PIX_FMT_YVU420)
+		imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
+	else if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
+		imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
+
+	switch (fmt->pixelformat) {
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_UYVY:
+		widthy = fmt->width * 2;
+		widthuv = fmt->width * 2;
+		break;
+	case V4L2_PIX_FMT_RGB565:
+		widthy = fmt->width * 2;
+		widthuv = 0;
+		break;
+	case V4L2_PIX_FMT_JPEG:
+		widthy = fmt->bytesperline;
+		widthuv = fmt->bytesperline;
+		break;
+	case V4L2_PIX_FMT_YUV422P:
+	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YVU420:
+		widthy = fmt->width;
+		widthuv = fmt->width / 2;
+		break;
+	default:
+		break;
+	}
+
+	mcam_reg_write_mask(cam, REG_IMGPITCH, widthuv << 16 | widthy,
+			IMGP_YP_MASK | IMGP_UVP_MASK);
+	mcam_reg_write(cam, REG_IMGSIZE, imgsz_h | imgsz_w);
+	mcam_reg_write(cam, REG_IMGOFFSET, 0x0);
 
-	imgsz = ((fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK) |
-		(fmt->bytesperline & IMGSZ_H_MASK);
-	mcam_reg_write(cam, REG_IMGSIZE, imgsz);
-	mcam_reg_write(cam, REG_IMGOFFSET, 0);
-	/* YPITCH just drops the last two bits */
-	mcam_reg_write_mask(cam, REG_IMGPITCH, fmt->bytesperline,
-			IMGP_YP_MASK);
 	/*
 	 * Tell the controller about the image format we are using.
 	 */
-	switch (cam->pix_format.pixelformat) {
+	switch (fmt->pixelformat) {
+	case V4L2_PIX_FMT_YUV422P:
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_YUV | C0_YUV_PLANAR | C0_YUVE_YVYU, C0_DF_MASK);
+		break;
+	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YVU420:
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_YUV | C0_YUV_420PL | C0_YUVE_YVYU, C0_DF_MASK);
+		break;
 	case V4L2_PIX_FMT_YUYV:
-	    mcam_reg_write_mask(cam, REG_CTRL0,
-			    C0_DF_YUV|C0_YUV_PACKED|C0_YUVE_YUYV,
-			    C0_DF_MASK);
-	    break;
-
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_UYVY, C0_DF_MASK);
+		break;
+	case V4L2_PIX_FMT_UYVY:
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV, C0_DF_MASK);
+		break;
+	case V4L2_PIX_FMT_JPEG:
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV, C0_DF_MASK);
+		break;
 	case V4L2_PIX_FMT_RGB444:
-	    mcam_reg_write_mask(cam, REG_CTRL0,
-			    C0_DF_RGB|C0_RGBF_444|C0_RGB4_XRGB,
-			    C0_DF_MASK);
-		/* Alpha value? */
-	    break;
-
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XRGB, C0_DF_MASK);
+		break;
 	case V4L2_PIX_FMT_RGB565:
-	    mcam_reg_write_mask(cam, REG_CTRL0,
-			    C0_DF_RGB|C0_RGBF_565|C0_RGB5_BGGR,
-			    C0_DF_MASK);
-	    break;
-
+		mcam_reg_write_mask(cam, REG_CTRL0,
+			C0_DF_RGB | C0_RGBF_565 | C0_RGB5_BGGR, C0_DF_MASK);
+		break;
 	default:
-	    cam_err(cam, "Unknown format %x\n", cam->pix_format.pixelformat);
-	    break;
+		cam_err(cam, "camera: unknown format: %c\n", fmt->pixelformat);
+		break;
 	}
+
 	/*
 	 * Make sure it knows we want to use hsync/vsync.
 	 */
-	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC,
-			C0_SIFM_MASK);
-
+	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK);
 	/*
 	 * This field controls the generation of EOF(DVP only)
 	 */
@@ -706,7 +798,6 @@  static void mcam_ctlr_image(struct mcam_camera *cam)
 	}
 }
 
-
 /*
  * Configure the controller for operation; caller holds the
  * device mutex.
@@ -979,11 +1070,32 @@  static void mcam_vb_buf_queue(struct vb2_buffer *vb)
 {
 	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
 	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
+	struct v4l2_pix_format *fmt = &cam->pix_format;
 	unsigned long flags;
 	int start;
+	dma_addr_t dma_handle;
+	u32 base_size = fmt->width * fmt->height;
 
 	spin_lock_irqsave(&cam->dev_lock, flags);
+	dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
+	BUG_ON(!dma_handle);
 	start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
+
+	if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV422P) {
+		mvb->yuv_p.y = dma_handle;
+		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
+		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 2;
+	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YUV420) {
+		mvb->yuv_p.y = dma_handle;
+		mvb->yuv_p.u = mvb->yuv_p.y + base_size;
+		mvb->yuv_p.v = mvb->yuv_p.u + base_size / 4;
+	} else if (cam->pix_format.pixelformat == V4L2_PIX_FMT_YVU420) {
+		mvb->yuv_p.y = dma_handle;
+		mvb->yuv_p.v = mvb->yuv_p.y + base_size;
+		mvb->yuv_p.u = mvb->yuv_p.v + base_size / 4;
+	} else
+		mvb->yuv_p.y = dma_handle;
+
 	list_add(&mvb->queue, &cam->buffers);
 	if (cam->state == S_STREAMING && test_bit(CF_SG_RESTART, &cam->flags))
 		mcam_sg_restart(cam);
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index 40368f6..3f75d7d 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -233,6 +233,12 @@  int mccic_resume(struct mcam_camera *cam);
 #define REG_Y0BAR	0x00
 #define REG_Y1BAR	0x04
 #define REG_Y2BAR	0x08
+#define REG_U0BAR	0x0c
+#define REG_U1BAR	0x10
+#define REG_U2BAR	0x14
+#define REG_V0BAR	0x18
+#define REG_V1BAR	0x1C
+#define REG_V2BAR	0x20
 
 /*
  * register definitions for MIPI support