RE: vb2: holding buffers until after start_streaming()
Hello, On Tuesday, June 21, 2011 7:14 PM Jonathan Corbet wrote: On Tue, 21 Jun 2011 18:07:03 +0200 Marek Szyprowski m.szyprow...@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 RD 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: vb2: holding buffers until after start_streaming()
On Wed, 22 Jun 2011 11:43:14 +0200 Marek Szyprowski m.szyprow...@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 -- 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: vb2: holding buffers until after start_streaming()
On Wednesday, June 22, 2011 14:38:55 Jonathan Corbet wrote: On Wed, 22 Jun 2011 11:43:14 +0200 Marek Szyprowski m.szyprow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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: vb2: holding buffers until after start_streaming()
Hello, On Monday, June 20, 2011 5:49 PM Jonathan Corbet wrote: On Mon, 20 Jun 2011 07:30:11 +0200 Marek Szyprowski m.szyprow...@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 RD 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: vb2: holding buffers until after start_streaming()
On Tue, 21 Jun 2011 18:07:03 +0200 Marek Szyprowski m.szyprow...@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 -- 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: vb2: holding buffers until after start_streaming()
On Mon, 20 Jun 2011 07:30:11 +0200 Marek Szyprowski m.szyprow...@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 -- 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: vb2: holding buffers until after start_streaming()
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 cor...@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
Re: vb2: holding buffers until after start_streaming()
Hi Jon,
On Fri, Jun 17, 2011 at 11:57, Jonathan Corbet cor...@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
--
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: vb2: holding buffers until after start_streaming()
Hello,
On Monday, June 20, 2011 3:28 AM Pawel Osciak wrote:
On Fri, Jun 17, 2011 at 11:57, Jonathan Corbet cor...@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 RD 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
vb2: holding buffers until after start_streaming()
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
--
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
