Hi Guennadi, >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx] >Sent: Tuesday, December 04, 2012 2:57 PM >To: Libin Yang >Cc: corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx >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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
