vb2: holding buffers until after start_streaming()

vb2: holding buffers until after start_streaming()

[Jonathan Corbet]

Here's another videobuf2 question...I've been trying to track down some
weird behavior, the roots of which were in the fact that start_streaming()
gets called even though no buffers have been queued.  This behavior is
quite explicit in the code:

	/*
	 * Let driver notice that streaming state has been enabled.
	 */
	ret = call_qop(q, start_streaming, q);
	if (ret) {
		dprintk(1, "streamon: driver refused to start streaming\n");
		return ret;
	}

	q->streaming = 1;

	/*
	 * If any buffers were queued before streamon,
	 * we can now pass them to driver for processing.
	 */
	list_for_each_entry(vb, &q->queued_list, queued_entry)
		__enqueue_in_driver(vb);

Pretty much every v4l2 capture application I've ever encountered passes all
of its buffers to VIDIOC_QBUF before starting streaming for a reason - it
makes little sense to start if there's nothing to stream to.  It's really
tempting to reorder that code, but...  it seems you must have done things
this way for a reason.  Why did you need to reorder the operations in this
way?

(Yes, my driver's current tendency to go oops when start_streaming() gets
called with no buffers is a bug, I'll fix it regardless).

Thanks,

jon

Re: vb2: holding buffers until after start_streaming()

[Pawel Osciak]

Hi Jon,

On Fri, Jun 17, 2011 at 11:57, Jonathan Corbet <corbet@lwn.net> wrote:
> Here's another videobuf2 question...I've been trying to track down some
> weird behavior, the roots of which were in the fact that start_streaming()
> gets called even though no buffers have been queued.  This behavior is
> quite explicit in the code:
>
>        /*
>         * Let driver notice that streaming state has been enabled.
>         */
>        ret = call_qop(q, start_streaming, q);
>        if (ret) {
>                dprintk(1, "streamon: driver refused to start streaming\n");
>                return ret;
>        }
>
>        q->streaming = 1;
>
>        /*
>         * If any buffers were queued before streamon,
>         * we can now pass them to driver for processing.
>         */
>        list_for_each_entry(vb, &q->queued_list, queued_entry)
>                __enqueue_in_driver(vb);
>
> Pretty much every v4l2 capture application I've ever encountered passes all
> of its buffers to VIDIOC_QBUF before starting streaming for a reason - it
> makes little sense to start if there's nothing to stream to.  It's really
> tempting to reorder that code, but...  it seems you must have done things
> this way for a reason.  Why did you need to reorder the operations in this
> way?
>

I don't see a reason why these couldn't be reordered (Marek should be
able to confirm, he wrote those lines). But this wouldn't fix
everything, as the V4L2 API permits streamon without queuing any
buffers first (for capture devices). So even reordered, it's possible
for start_streaming to be called without passing any buffers to the
driver first.

-- 
Best regards,
Pawel Osciak

RE: vb2: holding buffers until after start_streaming()

[Marek Szyprowski]

Hello,

On Monday, June 20, 2011 3:28 AM Pawel Osciak wrote:

> On Fri, Jun 17, 2011 at 11:57, Jonathan Corbet <corbet@lwn.net> wrote:
> > Here's another videobuf2 question...I've been trying to track down some
> > weird behavior, the roots of which were in the fact that
> start_streaming()
> > gets called even though no buffers have been queued.  This behavior is
> > quite explicit in the code:
> >
> >        /*
> >         * Let driver notice that streaming state has been enabled.
> >         */
> >        ret = call_qop(q, start_streaming, q);
> >        if (ret) {
> >                dprintk(1, "streamon: driver refused to start
> streaming\n");
> >                return ret;
> >        }
> >
> >        q->streaming = 1;
> >
> >        /*
> >         * If any buffers were queued before streamon,
> >         * we can now pass them to driver for processing.
> >         */
> >        list_for_each_entry(vb, &q->queued_list, queued_entry)
> >                __enqueue_in_driver(vb);
> >
> > Pretty much every v4l2 capture application I've ever encountered passes
> all
> > of its buffers to VIDIOC_QBUF before starting streaming for a reason - it
> > makes little sense to start if there's nothing to stream to.  It's really
> > tempting to reorder that code, but...  it seems you must have done things
> > this way for a reason.  Why did you need to reorder the operations in
> this
> > way?
> >
> 
> I don't see a reason why these couldn't be reordered (Marek should be
> able to confirm, he wrote those lines). But this wouldn't fix
> everything, as the V4L2 API permits streamon without queuing any
> buffers first (for capture devices). So even reordered, it's possible
> for start_streaming to be called without passing any buffers to the
> driver first.

The problem is the fact that you cannot guarantee the opposite order
in all cases. Even if you swap __enqueue_in_driver and 
call_qop(start_streaming), user might call respective ioctl in the
opposite order and you will end with start_streaming before 
__enqueue_in_driver. Calling VIDIOC_STREAMON without previous call
to VIDIOC_QBUF is legal from v4l2 api definition.

Because of that I decided to call start_streaming first, before the 
__enqueue_in_driver() to ensure the drivers will get their methods
called always in the same order, whatever used does. 

Start_streaming was designed to perform time consuming operations
like enabling the power, configuring the pipeline, setting up the
tuner, etc. Some of these can fail and it is really good to report
the failure asap.

If you cannot start your hardware (the dma engine) without queued
buffers then you probably need to move dma starting routine to the
first buffer_queue call. The problem is much more complex than it
initially looks. 

Please note that in videobuf2 buffer_queue method is allowed to sleep,
unlike it was designed in old videobuf.

Usually drivers require at least two buffers and always keep at 
least one in the dma engine, which overwrites it with incoming frames
until next buffer have been queued. However there are also devices
(like camera sensors) that might be used to capture only one single
frame or a few consecutive frames (for example a series of pictures).
They need to dequeue the last buffer once it got filled with video
data, so the design with overwriting the buffer makes no sense.
Right now it is really driver dependent and no generic solution 
exist.

We have been discussing it but no consensus has been made yet, so
right now I've decided to keep the current design. We probably needs
some additional flag somewhere to configure the driver either to
continuously overwrite last buffer until next one has been queued
or stop the dma engine and return the buffer to user. Once


Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: vb2: holding buffers until after start_streaming()

[Sylwester Nawrocki]

Hi Jon

On 06/17/2011 08:57 PM, Jonathan Corbet wrote:
> Here's another videobuf2 question...I've been trying to track down some
> weird behavior, the roots of which were in the fact that start_streaming()
> gets called even though no buffers have been queued.  This behavior is
> quite explicit in the code:
> 
> 	/*
> 	 * Let driver notice that streaming state has been enabled.
> 	 */
> 	ret = call_qop(q, start_streaming, q);
> 	if (ret) {
> 		dprintk(1, "streamon: driver refused to start streaming\n");
> 		return ret;
> 	}
> 
> 	q->streaming = 1;
> 
> 	/*
> 	 * If any buffers were queued before streamon,
> 	 * we can now pass them to driver for processing.
> 	 */
> 	list_for_each_entry(vb, &q->queued_list, queued_entry)
> 		__enqueue_in_driver(vb);
> 
> Pretty much every v4l2 capture application I've ever encountered passes all
> of its buffers to VIDIOC_QBUF before starting streaming for a reason - it
> makes little sense to start if there's nothing to stream to.  It's really
> tempting to reorder that code, but...  it seems you must have done things
> this way for a reason.  Why did you need to reorder the operations in this
> way?

AFAIR one of main reasons for doing the operations in that order was to
create consistent conditions for the drivers, regardless of the call sequence
in the user space.

You may find more information in this thread:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg29348.html


--
Regards,
Sylwester

Re: vb2: holding buffers until after start_streaming()

[Jonathan Corbet]

On Mon, 20 Jun 2011 07:30:11 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Because of that I decided to call start_streaming first, before the 
> __enqueue_in_driver() to ensure the drivers will get their methods
> called always in the same order, whatever used does. 

It still seems like the "wrong" order to me; it means that
start_streaming() can't actually start streaming.  But, as has been
pointed out, the driver can't count on the buffers being there in any
case.  This ordering does, at least, expose situations where the driver
author didn't think that buffers might not have been queued yet.

(BTW, lest people think I'm complaining too much, let it be said that vb2
is, indeed, a big improvement over its predecessor.)

Thanks,

jon

Re: vb2: holding buffers until after start_streaming()

[Hans Verkuil]

On Monday, June 20, 2011 07:30:11 Marek Szyprowski wrote:
> Hello,
> 
> On Monday, June 20, 2011 3:28 AM Pawel Osciak wrote:
> 
> > On Fri, Jun 17, 2011 at 11:57, Jonathan Corbet <corbet@lwn.net> wrote:
> > > Here's another videobuf2 question...I've been trying to track down some
> > > weird behavior, the roots of which were in the fact that
> > start_streaming()
> > > gets called even though no buffers have been queued.  This behavior is
> > > quite explicit in the code:
> > >
> > >        /*
> > >         * Let driver notice that streaming state has been enabled.
> > >         */
> > >        ret = call_qop(q, start_streaming, q);
> > >        if (ret) {
> > >                dprintk(1, "streamon: driver refused to start
> > streaming\n");
> > >                return ret;
> > >        }
> > >
> > >        q->streaming = 1;
> > >
> > >        /*
> > >         * If any buffers were queued before streamon,
> > >         * we can now pass them to driver for processing.
> > >         */
> > >        list_for_each_entry(vb, &q->queued_list, queued_entry)
> > >                __enqueue_in_driver(vb);
> > >
> > > Pretty much every v4l2 capture application I've ever encountered passes
> > all
> > > of its buffers to VIDIOC_QBUF before starting streaming for a reason - it
> > > makes little sense to start if there's nothing to stream to.  It's really
> > > tempting to reorder that code, but...  it seems you must have done things
> > > this way for a reason.  Why did you need to reorder the operations in
> > this
> > > way?
> > >
> > 
> > I don't see a reason why these couldn't be reordered (Marek should be
> > able to confirm, he wrote those lines). But this wouldn't fix
> > everything, as the V4L2 API permits streamon without queuing any
> > buffers first (for capture devices). So even reordered, it's possible
> > for start_streaming to be called without passing any buffers to the
> > driver first.
> 
> The problem is the fact that you cannot guarantee the opposite order
> in all cases. Even if you swap __enqueue_in_driver and 
> call_qop(start_streaming), user might call respective ioctl in the
> opposite order and you will end with start_streaming before 
> __enqueue_in_driver. Calling VIDIOC_STREAMON without previous call
> to VIDIOC_QBUF is legal from v4l2 api definition.

But not all drivers support this. So the api does allow for it, but I
don't believe it is mandatory (or if it is, then many, many drivers are
not compliant).

> Because of that I decided to call start_streaming first, before the 
> __enqueue_in_driver() to ensure the drivers will get their methods
> called always in the same order, whatever used does. 

The problem with doing this is that in order to start streaming several
drivers need to have the queued buffers available. For example, the davinci
capture drivers (vpif_capture.c) can start the DMA if there is only one
buffer queued, but that will overwrite that buffer until another one is
queued. If it has two buffers or more, then it can start the DMA in a more
optimal fashion. (As an aside, looking at that driver I think it actually
always starts the DMA as if there is only one buffer available, thus
introducing an unnecessarily extra frame delay.)

Anyway, what I'm trying to say is that some hardware needs to have the
queued buffers in order to start DMA in the best possible way.

> Start_streaming was designed to perform time consuming operations
> like enabling the power, configuring the pipeline, setting up the
> tuner, etc. Some of these can fail and it is really good to report
> the failure asap.

Reordering doesn't affect that.
 
> If you cannot start your hardware (the dma engine) without queued
> buffers then you probably need to move dma starting routine to the
> first buffer_queue call. The problem is much more complex than it
> initially looks. 

I don't believe that this is mandatory. And if the spec says so, then I think
that spec should be changed since it doesn't reflect reality.

> Please note that in videobuf2 buffer_queue method is allowed to sleep,
> unlike it was designed in old videobuf.

Hmmm, I hope the driver will remember to release and reacquire and locks
when it goes to sleep. Something to document.

> Usually drivers require at least two buffers and always keep at 
> least one in the dma engine, which overwrites it with incoming frames
> until next buffer have been queued. However there are also devices
> (like camera sensors) that might be used to capture only one single
> frame or a few consecutive frames (for example a series of pictures).
> They need to dequeue the last buffer once it got filled with video
> data, so the design with overwriting the buffer makes no sense.
> Right now it is really driver dependent and no generic solution 
> exist.

What's wrong with keeping this driver specific?

> We have been discussing it but no consensus has been made yet, so
> right now I've decided to keep the current design. We probably needs
> some additional flag somewhere to configure the driver either to
> continuously overwrite last buffer until next one has been queued
> or stop the dma engine and return the buffer to user. Once

Something is missing here :-)

Anyway, I believe that it should be up to the driver to decide whether it will
allow STREAMON with no buffers queued. But the start_streaming implementation
*does* need to be called after the ownership of any pre-queued buffers has been
passed from vb2 to the driver, so that the driver can use them to start DMA in
an optimal manner (this is true for both capture and display, BTW).

Regards,

	Hans

RE: vb2: holding buffers until after start_streaming()

[Marek Szyprowski]

Hello,

On Monday, June 20, 2011 5:49 PM Jonathan Corbet wrote:

> On Mon, 20 Jun 2011 07:30:11 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > Because of that I decided to call start_streaming first, before the
> > __enqueue_in_driver() to ensure the drivers will get their methods
> > called always in the same order, whatever used does.
> 
> It still seems like the "wrong" order to me; it means that
> start_streaming() can't actually start streaming.  But, as has been
> pointed out, the driver can't count on the buffers being there in any
> case.  This ordering does, at least, expose situations where the driver
> author didn't think that buffers might not have been queued yet.
> 
> (BTW, lest people think I'm complaining too much, let it be said that vb2
> is, indeed, a big improvement over its predecessor.)

I'm aware of this issue and I definitely don't threat your comments as
complaining. Right now there are just a few drivers that use vb2 so it
is quite easy to fix or change some design ideas.

I've thought a bit more about the current design and I must confess that
in fact it is suboptimal. Probably during vb2 development I've focused too
much on vivi and mem2mem devices which were used for testing the framework.

Usually for mem2mem case streamon() operations don't touch DMA engines,
so I've missed the point that DMA engine for typical capture or output
device should be activated there with some buffers already queued.

Now we also know that there are drivers that may need to start dma engine
in buffer_queue and stop it in the isr (before buffer_done). Capturing a 
single frame with camera sensor (taking a picture) is one of such
examples.

I have an idea to introduce a new flags to let device driver tell vb2
weather it supports 'streaming without buffers' or not. This way the
order of operations in vb2_streamon() function can be switched and vb2
can also return an error if one will try to enable streaming on device
that cannot do it without buffers pre-queued. This way most of typical
capture and output drivers will be happy. They will just use the 
'overwrite last frame' technique to guarantee that there is at least
one buffer for the dma engine all the time when streaming is enable. 
Mem2mem (and these special 'streaming without buffers' capable) drivers
will just set these flags and continue enabling/disabling dma engine 
per-frame basis.

I will try to post the patches soon.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: vb2: holding buffers until after start_streaming()

[Jonathan Corbet]

On Tue, 21 Jun 2011 18:07:03 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> I have an idea to introduce a new flags to let device driver tell vb2
> weather it supports 'streaming without buffers' or not. This way the
> order of operations in vb2_streamon() function can be switched and vb2
> can also return an error if one will try to enable streaming on device
> that cannot do it without buffers pre-queued.

Do you really need a flag?  If a driver absolutely cannot stream without
buffers queued (and can't be fixed to start streaming for real when the
buffers show up) it should just return -EINVAL from start_streaming() or
some such.  The driver must be aware of its limitations regardless, but
there's no need to push that awareness into vb2 as well.

(FWIW, I wouldn't switch the order of operations in vb2_streamon(); I
would just take out the "if (q->streaming)" test at the end of vb2_qbuf()
and pass the buffers through directly.  But maybe that's just me.)

Thanks,

jon

RE: vb2: holding buffers until after start_streaming()

[Marek Szyprowski]

Hello,

On Tuesday, June 21, 2011 7:14 PM Jonathan Corbet wrote:

> On Tue, 21 Jun 2011 18:07:03 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > I have an idea to introduce a new flags to let device driver tell vb2
> > weather it supports 'streaming without buffers' or not. This way the
> > order of operations in vb2_streamon() function can be switched and vb2
> > can also return an error if one will try to enable streaming on device
> > that cannot do it without buffers pre-queued.
> 
> Do you really need a flag?  If a driver absolutely cannot stream without
> buffers queued (and can't be fixed to start streaming for real when the
> buffers show up) it should just return -EINVAL from start_streaming() or
> some such.  The driver must be aware of its limitations regardless, but
> there's no need to push that awareness into vb2 as well.

The main idea behind vb2 was to move all common error handling code to
the framework and provide simple functions that can be used by the driver
directly without the need for additional checks.

> (FWIW, I wouldn't switch the order of operations in vb2_streamon(); I
> would just take out the "if (q->streaming)" test at the end of vb2_qbuf()
> and pass the buffers through directly.  But maybe that's just me.)

I want to keep the current version of vb2_qbuf() and change the order of
operations in streamon().

The only problem that still need to be resolved is what should happen with
the buffers if start_streaming() fails. The ownership for these buffers have
been already given to the driver, but they might have been in dirty state.
Probably vb2 should assume that the buffers are lost and reinitialize them.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: vb2: holding buffers until after start_streaming()

[Jonathan Corbet]

On Wed, 22 Jun 2011 11:43:14 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> > Do you really need a flag?  If a driver absolutely cannot stream without
> > buffers queued (and can't be fixed to start streaming for real when the
> > buffers show up) it should just return -EINVAL from start_streaming() or
> > some such.  The driver must be aware of its limitations regardless, but
> > there's no need to push that awareness into vb2 as well.  
> 
> The main idea behind vb2 was to move all common error handling code to
> the framework and provide simple functions that can be used by the driver
> directly without the need for additional checks.

For stuff that's truly common, that makes sense.  As soon as you start
adding driver-specific flags, things aren't quite so common anymore.  In
this case, the driver writer can just as easily check the state of the
buffer queue at streamon time, so I don't see what would be gained.
Whatever, doesn't matter that much.

> > (FWIW, I wouldn't switch the order of operations in vb2_streamon(); I
> > would just take out the "if (q->streaming)" test at the end of vb2_qbuf()
> > and pass the buffers through directly.  But maybe that's just me.)  
> 
> I want to keep the current version of vb2_qbuf() and change the order of
> operations in streamon().

I'm curious as to why?  As far as I can tell, there's no reason not to
pass the buffers straight through when you get them?

> The only problem that still need to be resolved is what should happen with
> the buffers if start_streaming() fails. The ownership for these buffers have
> been already given to the driver, but they might have been in dirty state.
> Probably vb2 should assume that the buffers are lost and reinitialize them.

You already grab them back at stop_streaming() time, right?  I'd think
that a failed start_streaming() should leave things in exactly the same
state as a start/stop_streaming() sequence.

Thanks,

jon

Re: vb2: holding buffers until after start_streaming()

[Hans Verkuil]

On Wednesday, June 22, 2011 14:38:55 Jonathan Corbet wrote:
> On Wed, 22 Jun 2011 11:43:14 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > > Do you really need a flag?  If a driver absolutely cannot stream without
> > > buffers queued (and can't be fixed to start streaming for real when the
> > > buffers show up) it should just return -EINVAL from start_streaming() or
> > > some such.  The driver must be aware of its limitations regardless, but
> > > there's no need to push that awareness into vb2 as well.  
> > 
> > The main idea behind vb2 was to move all common error handling code to
> > the framework and provide simple functions that can be used by the driver
> > directly without the need for additional checks.
> 
> For stuff that's truly common, that makes sense.  As soon as you start
> adding driver-specific flags, things aren't quite so common anymore.  In
> this case, the driver writer can just as easily check the state of the
> buffer queue at streamon time, so I don't see what would be gained.
> Whatever, doesn't matter that much.
> 
> > > (FWIW, I wouldn't switch the order of operations in vb2_streamon(); I
> > > would just take out the "if (q->streaming)" test at the end of vb2_qbuf()
> > > and pass the buffers through directly.  But maybe that's just me.)  
> > 
> > I want to keep the current version of vb2_qbuf() and change the order of
> > operations in streamon().
> 
> I'm curious as to why?  As far as I can tell, there's no reason not to
> pass the buffers straight through when you get them?

I'm curious as well. I don't see the problem with that.

> > The only problem that still need to be resolved is what should happen with
> > the buffers if start_streaming() fails. The ownership for these buffers have
> > been already given to the driver, but they might have been in dirty state.
> > Probably vb2 should assume that the buffers are lost and reinitialize them.
> 
> You already grab them back at stop_streaming() time, right?  I'd think
> that a failed start_streaming() should leave things in exactly the same
> state as a start/stop_streaming() sequence.

I think so too. vb2 can just call queue_cancel, I think.

Regards,

	Hans

> 
> Thanks,
> 
> jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>