Thanks Albert Wang 86-21-61092656 >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx] >Sent: Tuesday, 27 November, 2012 19:45 >To: Albert Wang >Cc: corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx; 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@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 | 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
