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

2013-01-09 Thread Libin Yang 
Hi Guennadi,

Below is the update for widthy, widthuv and imgsz_w setting.

-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, January 02, 2013 12:56 AM
To: Albert Wang
Cc: cor...@lwn.net; linux-media@vger.kernel.org; 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 lby...@marvell.com

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

 Signed-off-by: Albert Wang twan...@marvell.com
 Signed-off-by: Libin Yang lby...@marvell.com
 ---
  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.


First of all, the setting bytesperline in this file is wrong. It should be
like the setting in the mcam-core-soc.c in the later patch. It's my fault
missing modifying the bytesperline when adding the new formats.
Besides the bytesperline in mcam-core-soc.c is a little wrong.
I will update it in the new version of patch.

For imgsz_w setting, this is for the CCIC input data format, which is from
sensor, while 'switch (fmt-pixelformat)' is CCIC output data format.
It seems a little confusing using fmt-pixelformat here. Using
mcam_formats-mbus_code seems more reasonable. Anyway, 
each fmt-pixelformat must have one mcam_formats-mbus_code
correspondingly. 

Although, our spec says it supports YUV420 input. Our HW team told me
we only use YUV422 and the length is width * 2. So it seems some 
mbus_code is wrong set here.
It seems in this case such format will be never used as we can see
ov7670 does not support yuv420.

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

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

2013-01-04 Thread Libin Yang 
Hi Guennadi,

Please see my comments below.

-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, January 02, 2013 12:56 AM
To: Albert Wang
Cc: cor...@lwn.net; linux-media@vger.kernel.org; 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 lby...@marvell.com

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

 Signed-off-by: Albert Wang twan...@marvell.com
 Signed-off-by: Libin Yang lby...@marvell.com
 ---
  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

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

2013-01-01 Thread Guennadi Liakhovetski 
On Sat, 15 Dec 2012, Albert Wang wrote:

 From: Libin Yang lby...@marvell.com
 
 This patch adds the new formats support for marvell-ccic.
 
 Signed-off-by: Albert Wang twan...@marvell.com
 Signed-off-by: Libin Yang lby...@marvell.com
 ---
  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.

 + 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);
 -

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

2013-01-01 Thread Albert Wang 
Hi, Guennadi


-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, 02 January, 2013 00:56
To: Albert Wang
Cc: cor...@lwn.net; linux-media@vger.kernel.org; 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 lby...@marvell.com

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

 Signed-off-by: Albert Wang twan...@marvell.com
 Signed-off-by: Libin Yang lby...@marvell.com
 ---
  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.

[Albert Wang] It looks they are both 12 bit planar format, they have same 
imgsz_w.
Anyway, we will double check it after back to office.

 +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

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

2013-01-01 Thread Guennadi Liakhovetski 
Hi Albert

On Tue, 1 Jan 2013, Albert Wang wrote:

  +  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.
 
 [Albert Wang] It looks they are both 12 bit planar format, they have same 
 imgsz_w.
 Anyway, we will double check it after back to office.

_Both_ YUV420 and YVU420 - yes, but YUV422P is 16-bit.

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-12-16 Thread Jonathan Corbet 
On Sat, 15 Dec 2012 17:57:55 +0800
Albert Wang twan...@marvell.com wrote:

 From: Libin Yang lby...@marvell.com
 
 This patch adds the new formats support for marvell-ccic.

Once again, just one second-order comment:

 +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;
 +}

This seems like the kind of thing that would be useful in a number of
places; I'd be tempted to push it up a level and make it available to all
V4L2 drivers.  Of course, that means making it work for *all* formats,
which would be a pain.  

But, then, I can see some potential future pain if somebody adds a new
format and forgets to tweak this function here.  Rather than adding a new
switch, could you put a planar flag into struct mcam_format_struct
instead?  That would help to keep all this information together.

jon
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-12-16 Thread Albert Wang 
Hi, Jonathan



-Original Message-
From: Jonathan Corbet [mailto:cor...@lwn.net]
Sent: Monday, 17 December, 2012 00:17
To: Albert Wang
Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang
Subject: Re: [PATCH V3 06/15] [media] marvell-ccic: add new formats support for
marvell-ccic driver

On Sat, 15 Dec 2012 17:57:55 +0800
Albert Wang twan...@marvell.com wrote:

 From: Libin Yang lby...@marvell.com

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

Once again, just one second-order comment:

 +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;
 +}

This seems like the kind of thing that would be useful in a number of
places; I'd be tempted to push it up a level and make it available to all
V4L2 drivers.  Of course, that means making it work for *all* formats,
which would be a pain.

But, then, I can see some potential future pain if somebody adds a new
format and forgets to tweak this function here.  Rather than adding a new
switch, could you put a planar flag into struct mcam_format_struct
instead?  That would help to keep all this information together.

[Albert Wang] Yes, it looks make sense, we will think about it in next version.

jon


Thanks
Albert Wang
86-21-61092656
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-12-15 Thread Albert Wang 
From: Libin Yang lby...@marvell.com

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

Signed-off-by: Albert Wang twan...@marvell.com
Signed-off-by: Libin Yang lby...@marvell.com
---
 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
@@ -111,6 +111,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,
@@ -169,6 +193,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
@@ -180,6 +210,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)
@@ -470,6 +501,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
@@ -481,6 +524,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
 */
@@ -499,8 +544,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);
+   }
 }
 
 /*
@@ -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