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

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

 



Hi Guennadi,

Please see my comments below.

>-----Original Message-----
>From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx]
>Sent: Wednesday, January 02, 2013 12:56 AM
>To: Albert Wang
>Cc: corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx; Libin Yang
>Subject: Re: [PATCH V3 06/15] [media] marvell-ccic: add new formats support for
>marvell-ccic driver
>
>On Sat, 15 Dec 2012, Albert Wang wrote:
>
>> From: Libin Yang <lbyang@xxxxxxxxxxx>
>>
>> This patch adds the new formats support for marvell-ccic.
>>
>> Signed-off-by: Albert Wang <twang13@xxxxxxxxxxx>
>> Signed-off-by: Libin Yang <lbyang@xxxxxxxxxxx>
>> ---
>>  drivers/media/platform/marvell-ccic/mcam-core.c |  175 ++++++++++++++++++-----
>>  drivers/media/platform/marvell-ccic/mcam-core.h |    6 +
>>  2 files changed, 149 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
>b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 3cc1d0c..a679917 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>
>[snip]
>
>> @@ -658,49 +708,85 @@ 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;
>> +
>> +	switch (fmt->pixelformat) {
>> +	case V4L2_PIX_FMT_YUYV:
>> +	case V4L2_PIX_FMT_UYVY:
>> +		widthy = fmt->width * 2;
>> +		widthuv = 0;
>> +		break;
>> +	case V4L2_PIX_FMT_JPEG:
>> +		imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
>> +		widthy = fmt->bytesperline;
>> +		widthuv = 0;
>> +		break;
>> +	case V4L2_PIX_FMT_YUV422P:
>> +	case V4L2_PIX_FMT_YUV420:
>> +	case V4L2_PIX_FMT_YVU420:
>> +		imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
>> +		widthy = fmt->width;
>> +		widthuv = fmt->width / 2;
>
>I might be wrong, but the above doesn't look right to me. Firstly, YUV422P
>is a 4:2:2 format, whereas YUV420 and YVU420 are 4:2:0 formats, so, I
>would expect calculations for them to differ. Besides, bytesperline * 4 /
>3 doesn't look right for any of them. If this is what I think - total
>number of bytes per line, i.e., sizeimage / height, than shouldn't YAU422P
>have
>+		imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>and the other two
>+		imgsz_w = (fmt->bytesperline * 3 / 2) & IMGSZ_H_MASK;
>? But maybe I'm wrong, please, double-check and confirm.
>

Basically, I agree with you. We are checking with our HW team how the value is calculated out. And we will update here as soon as we get the answer.

>> +		break;
>> +	default:
>> +		widthy = fmt->bytesperline;
>> +		widthuv = 0;
>> +	}
>> +
>> +	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);
>> +		mcam_reg_write_mask(cam, REG_CTRL0,
>> +			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XRGB, C0_DF_MASK);
>>  		/* Alpha value? */
>> -	    break;
>> -
>> +		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: %#x\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)
>>  	 */
>> @@ -711,7 +797,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
>>  	}
>>  }
>>
>> -
>>  /*
>>   * Configure the controller for operation; caller holds the
>>   * device mutex.
>> @@ -984,11 +1069,37 @@ 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 pixel_count = 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);
>> +
>> +	switch (cam->pix_format.pixelformat) {
>> +	case V4L2_PIX_FMT_YUV422P:
>> +		mvb->yuv_p.y = dma_handle;
>
>The above line is common for all cases, perhaps just put it above switch?
>
>> +		mvb->yuv_p.u = mvb->yuv_p.y + pixel_count;
>> +		mvb->yuv_p.v = mvb->yuv_p.u + pixel_count / 2;
>> +		break;
>> +	case V4L2_PIX_FMT_YUV420:
>> +		mvb->yuv_p.y = dma_handle;
>> +		mvb->yuv_p.u = mvb->yuv_p.y + pixel_count;
>> +		mvb->yuv_p.v = mvb->yuv_p.u + pixel_count / 4;
>> +		break;
>> +	case V4L2_PIX_FMT_YVU420:
>> +		mvb->yuv_p.y = dma_handle;
>> +		mvb->yuv_p.v = mvb->yuv_p.y + pixel_count;
>> +		mvb->yuv_p.u = mvb->yuv_p.v + pixel_count / 4;
>> +		break;
>> +	default:
>> +		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);
>
>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@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