RE: Allocating videobuf_buffer, but lists not being initialized
Hello, On Tuesday, November 16, 2010 8:38 AM Hans Verkuil wrote: > On Tuesday, November 16, 2010 02:10:39 Andrew Chew wrote: > > I'm looking at drivers/media/video/videobuf-dma-contig.c's > > __videobuf_alloc() routine. We call > kzalloc() to allocate the videobuf_buffer. However, I don't see where the > two lists (vb->stream and > vb->queue) that are a part of struct videobuf_buffer get initialized (with, > say, INIT_LIST_HEAD). > > Yuck. The videobuf framework doesn't initialize vb-stream at all. It relies on > list_add_tail to effectively initialize it for it. It works, but it is not > exactly clean programming :-( > > The vb->queue list has to be initialized in the driver. Never understood the > reason for that either. > > Marek, can you make sure that videobuf2 will initialize these lists correctly? > That is, vb2 should do this initialization instead of the driver. In the current version of vb2 there is no such list, so the driver has to keep list tracking entries inside its own structures. Usually v4l2 drivers embed video_buf structure inside some custom buffer structure to track some driver specific per-buffer stuff, so there is no problem in adding struct list_head there. This resolves all design problems like who is responsible of initializing it. > > This results in a warning in the V4L2 camera host driver that I'm > > developing when the buf_prepare > method gets called. I do a similar sanity check to the sh_mobile_ceu_camera > driver (WARN_ON(!list- > >empty(&vb->queue));) in my buf_prepare method, and see the warning. If I > >add INIT_LIST_HEAD to > __videobuf_alloc(), this warning goes away. > > > > Is this a known bug? > > Well, videobuf is one big bug. We hope that we can merge the videobuf > replacement > (called videobuf2, amazingly enough :-) ) for 2.6.38. Fingers crossed. > > So you might want to wait until vb2 arrives, depending on your schedule. I'm sorry for the delay, but I work hard to track some well hidden bug. I hope to post a new version of videobuf2 patches tomorrow. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: Allocating videobuf_buffer, but lists not being initialized
于 11/16/2010 03:37 PM, Hans Verkuil 写道: On Tuesday, November 16, 2010 02:10:39 Andrew Chew wrote: I'm looking at drivers/media/video/videobuf-dma-contig.c's __videobuf_alloc() routine. We call kzalloc() to allocate the videobuf_buffer. However, I don't see where the two lists (vb->stream and vb->queue) that are a part of struct videobuf_buffer get initialized (with, say, INIT_LIST_HEAD). Yuck. The videobuf framework doesn't initialize vb-stream at all. It relies on list_add_tail to effectively initialize it for it. It works, but it is not exactly clean programming :-( The vb->queue list has to be initialized in the driver. Never understood the reason for that either. Marek, can you make sure that videobuf2 will initialize these lists correctly? That is, vb2 should do this initialization instead of the driver. vb2 have init the list : INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); btw, "queued_list" re-name "grabbing_list" is better? -- 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: Allocating videobuf_buffer, but lists not being initialized
于 11/16/2010 03:40 PM, Hans Verkuil 写道: On Tuesday, November 16, 2010 06:29:53 Pawel Osciak wrote: On Mon, Nov 15, 2010 at 17:10, Andrew Chew wrote: I'm looking at drivers/media/video/videobuf-dma-contig.c's __videobuf_alloc() routine. We call kzalloc() to allocate the videobuf_buffer. However, I don't see where the two lists (vb->stream and vb->queue) that are a part of struct videobuf_buffer get initialized (with, say, INIT_LIST_HEAD). Those are not lists, but list entries. Those members of videobuf_buffer struct are used to put the buffer on one of the following lists: stream is a list entry for stream list in videobuf_queue, queue is used as list entry for driver's buffer queue. So? They still should be initialized properly. It's bad form to leave invalid pointers there. it just a list->entry, it has initialized by kzalloc at __videobuf_alloc_vb(), Regards, Hans -- 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: Allocating videobuf_buffer, but lists not being initialized
On Tuesday, November 16, 2010 08:37:32 Hans Verkuil wrote: > On Tuesday, November 16, 2010 02:10:39 Andrew Chew wrote: > > I'm looking at drivers/media/video/videobuf-dma-contig.c's > > __videobuf_alloc() routine. We call kzalloc() to allocate the > > videobuf_buffer. However, I don't see where the two lists (vb->stream and > > vb->queue) that are a part of struct videobuf_buffer get initialized (with, > > say, INIT_LIST_HEAD). > > Yuck. The videobuf framework doesn't initialize vb-stream at all. It relies on > list_add_tail to effectively initialize it for it. It works, but it is not > exactly clean programming :-( > > The vb->queue list has to be initialized in the driver. Never understood the > reason for that either. I'm actually not sure about that. I know I had problems in my driver so I had to initialize it myself. But it seems not all drivers do that. > Marek, can you make sure that videobuf2 will initialize these lists correctly? > That is, vb2 should do this initialization instead of the driver. lists -> list entries > > > This results in a warning in the V4L2 camera host driver that I'm > > developing when the buf_prepare method gets called. I do a similar sanity > > check to the sh_mobile_ceu_camera driver > > (WARN_ON(!list->empty(&vb->queue));) in my buf_prepare method, and see the > > warning. If I add INIT_LIST_HEAD to __videobuf_alloc(), this warning goes > > away. > > > > Is this a known bug? > > Well, videobuf is one big bug. We hope that we can merge the videobuf > replacement > (called videobuf2, amazingly enough :-) ) for 2.6.38. Fingers crossed. > > So you might want to wait until vb2 arrives, depending on your schedule. > > Regards, > > Hans > > -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: Allocating videobuf_buffer, but lists not being initialized
On Tuesday, November 16, 2010 06:29:53 Pawel Osciak wrote: > On Mon, Nov 15, 2010 at 17:10, Andrew Chew wrote: > > I'm looking at drivers/media/video/videobuf-dma-contig.c's > > __videobuf_alloc() routine. We call kzalloc() to allocate the > > videobuf_buffer. However, I don't see where the two lists (vb->stream and > > vb->queue) that are a part of struct videobuf_buffer get initialized (with, > > say, INIT_LIST_HEAD). > > > > Those are not lists, but list entries. Those members of > videobuf_buffer struct are used to put the buffer on one of the > following lists: stream is a list entry for stream list in > videobuf_queue, queue is used as list entry for driver's buffer queue. So? They still should be initialized properly. It's bad form to leave invalid pointers there. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: Allocating videobuf_buffer, but lists not being initialized
On Tuesday, November 16, 2010 02:10:39 Andrew Chew wrote: > I'm looking at drivers/media/video/videobuf-dma-contig.c's __videobuf_alloc() > routine. We call kzalloc() to allocate the videobuf_buffer. However, I > don't see where the two lists (vb->stream and vb->queue) that are a part of > struct videobuf_buffer get initialized (with, say, INIT_LIST_HEAD). Yuck. The videobuf framework doesn't initialize vb-stream at all. It relies on list_add_tail to effectively initialize it for it. It works, but it is not exactly clean programming :-( The vb->queue list has to be initialized in the driver. Never understood the reason for that either. Marek, can you make sure that videobuf2 will initialize these lists correctly? That is, vb2 should do this initialization instead of the driver. > This results in a warning in the V4L2 camera host driver that I'm developing > when the buf_prepare method gets called. I do a similar sanity check to the > sh_mobile_ceu_camera driver (WARN_ON(!list->empty(&vb->queue));) in my > buf_prepare method, and see the warning. If I add INIT_LIST_HEAD to > __videobuf_alloc(), this warning goes away. > > Is this a known bug? Well, videobuf is one big bug. We hope that we can merge the videobuf replacement (called videobuf2, amazingly enough :-) ) for 2.6.38. Fingers crossed. So you might want to wait until vb2 arrives, depending on your schedule. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: Allocating videobuf_buffer, but lists not being initialized
On Mon, Nov 15, 2010 at 17:10, Andrew Chew wrote: > I'm looking at drivers/media/video/videobuf-dma-contig.c's __videobuf_alloc() > routine. We call kzalloc() to allocate the videobuf_buffer. However, I > don't see where the two lists (vb->stream and vb->queue) that are a part of > struct videobuf_buffer get initialized (with, say, INIT_LIST_HEAD). > Those are not lists, but list entries. Those members of videobuf_buffer struct are used to put the buffer on one of the following lists: stream is a list entry for stream list in videobuf_queue, queue is used as list entry for driver's buffer queue. -- Best regards, Pawel Osciak -- 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
Allocating videobuf_buffer, but lists not being initialized
I'm looking at drivers/media/video/videobuf-dma-contig.c's __videobuf_alloc() routine. We call kzalloc() to allocate the videobuf_buffer. However, I don't see where the two lists (vb->stream and vb->queue) that are a part of struct videobuf_buffer get initialized (with, say, INIT_LIST_HEAD). This results in a warning in the V4L2 camera host driver that I'm developing when the buf_prepare method gets called. I do a similar sanity check to the sh_mobile_ceu_camera driver (WARN_ON(!list->empty(&vb->queue));) in my buf_prepare method, and see the warning. If I add INIT_LIST_HEAD to __videobuf_alloc(), this warning goes away. Is this a known bug? --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- -- 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
