RE: [PATCH 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,

>-----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


[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