Allocating videobuf_buffer, but lists not being initialized

Allocating videobuf_buffer, but lists not being initialized

[Andrew Chew]

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

Re: Allocating videobuf_buffer, but lists not being initialized

[Pawel Osciak]

On Mon, Nov 15, 2010 at 17:10, Andrew Chew <AChew@nvidia.com> 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

Re: Allocating videobuf_buffer, but lists not being initialized

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

> 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

Re: Allocating videobuf_buffer, but lists not being initialized

[Hans Verkuil]

On Tuesday, November 16, 2010 06:29:53 Pawel Osciak wrote:
> On Mon, Nov 15, 2010 at 17:10, Andrew Chew <AChew@nvidia.com> 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

Re: Allocating videobuf_buffer, but lists not being initialized

[Hans Verkuil]

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

Re: Allocating videobuf_buffer, but lists not being initialized

[Figo.zhang]

于 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<AChew@nvidia.com>  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
>

Re: Allocating videobuf_buffer, but lists not being initialized

[Figo.zhang]

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

RE: Allocating videobuf_buffer, but lists not being initialized

[Marek Szyprowski]

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