| Message ID | 1309340946-5658-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Marek, On Wednesday 29 June 2011 11:49:06 Marek Szyprowski wrote: > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > the order of operations during stream on operation. Now the buffer are > first queued to the driver and then the start_streaming method is called. > This resolves the most common case when the driver needs to know buffer > addresses to enable dma engine and start streaming. For drivers that can > handle start_streaming without queued buffers (mem2mem and 'one shot' > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > introduced. Driver can set it to let videobuf2 know that it support this > mode. Is starting/stopping DMA engines that expensive on most hardware ? Several mails mentioned that drivers should keep one buffer around to avoid stopping the DMA engine in case of buffer underrun. The OMAP3 ISP driver just stops the ISP when it runs out of buffers, and restart it when a new buffer is queued. Switching the order of the start_streaming and __enqueue_in_driver calls would make my life more difficult on the OMAP3 because I will have to check if the queue is streaming in the qbuf callback. Your s5p-fimc driver has to check for that as well. I wonder if it really helps for other drivers. > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > vivi) to work properly with the changed order of operations and enables > use of the newly introduced flag. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > CC: Pawel Osciak <pawel@osciak.com> > --- > > drivers/media/video/mem2mem_testdev.c | 4 +- > drivers/media/video/s5p-fimc/fimc-capture.c | 65 > ++++++++++++++++---------- > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > drivers/media/video/videobuf2-core.c | 21 ++++----- > drivers/media/video/vivi.c | 2 +- > include/media/videobuf2-core.h | 11 +++-- > 6 files changed, 62 insertions(+), 45 deletions(-) > > > --- > > Hello, > > This patch introduces significant changes in the vb2 streamon operation, > so all vb2 clients need to be checked and updated. Right now I didn't > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > this patch can be merged, I will update it to include all the required > changes to these two drivers as well. > > Best regards > -- > Marek Szyprowski > Samsung Poland R&D Center [snip] > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c > index 2238a61..e740a44 100644 > --- a/drivers/media/video/vivi.c > +++ b/drivers/media/video/vivi.c > @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) > q = &dev->vb_vidq; > memset(q, 0, sizeof(dev->vb_vidq)); > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; Why do you remove VB2_USERPTR support from vivi ? > q->drv_priv = dev; > q->buf_struct_size = sizeof(struct vivi_buffer); > q->ops = &vivi_video_qops;
Hello, On Wednesday, June 29, 2011 12:45 PM Laurent Pinchart wrote: > On Wednesday 29 June 2011 11:49:06 Marek Szyprowski wrote: > > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > > the order of operations during stream on operation. Now the buffer are > > first queued to the driver and then the start_streaming method is called. > > This resolves the most common case when the driver needs to know buffer > > addresses to enable dma engine and start streaming. For drivers that can > > handle start_streaming without queued buffers (mem2mem and 'one shot' > > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > > introduced. Driver can set it to let videobuf2 know that it support this > > mode. > > Is starting/stopping DMA engines that expensive on most hardware ? > Several > mails mentioned that drivers should keep one buffer around to avoid > stopping > the DMA engine in case of buffer underrun. The OMAP3 ISP driver just stops > the > ISP when it runs out of buffers, and restart it when a new buffer is queued. > Switching the order of the start_streaming and __enqueue_in_driver calls > would > make my life more difficult on the OMAP3 because I will have to check if > the > queue is streaming in the qbuf callback. Your s5p-fimc driver has to check > for > that as well. I wonder if it really helps for other drivers. I'm not an expert, but it looks that most drivers for consumer cards doesn't stop/suspend dma engine until streamoff is called. I would also like to get some more feedback on this issue. > > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > > vivi) to work properly with the changed order of operations and enables > > use of the newly introduced flag. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > CC: Pawel Osciak <pawel@osciak.com> > > --- > > > > drivers/media/video/mem2mem_testdev.c | 4 +- > > drivers/media/video/s5p-fimc/fimc-capture.c | 65 > > ++++++++++++++++---------- > > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > > drivers/media/video/videobuf2-core.c | 21 ++++----- > > drivers/media/video/vivi.c | 2 +- > > include/media/videobuf2-core.h | 11 +++-- > > 6 files changed, 62 insertions(+), 45 deletions(-) > > > > > > --- > > > > Hello, > > > > This patch introduces significant changes in the vb2 streamon operation, > > so all vb2 clients need to be checked and updated. Right now I didn't > > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > > this patch can be merged, I will update it to include all the required > > changes to these two drivers as well. > > > > Best regards > > -- > > Marek Szyprowski > > Samsung Poland R&D Center > > [snip] > > > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c > > index 2238a61..e740a44 100644 > > --- a/drivers/media/video/vivi.c > > +++ b/drivers/media/video/vivi.c > > @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) > > q = &dev->vb_vidq; > > memset(q, 0, sizeof(dev->vb_vidq)); > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > > + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; > > Why do you remove VB2_USERPTR support from vivi ? Huh, this should go as a separate patch. vmalloc allocator still lacks support for user pointer mode so there is no point advertising that vivi supports it. > > > q->drv_priv = dev; > > q->buf_struct_size = sizeof(struct vivi_buffer); > > q->ops = &vivi_video_qops; Best regards
On Wednesday, June 29, 2011 12:44:48 Laurent Pinchart wrote: > Hi Marek, > > On Wednesday 29 June 2011 11:49:06 Marek Szyprowski wrote: > > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > > the order of operations during stream on operation. Now the buffer are > > first queued to the driver and then the start_streaming method is called. > > This resolves the most common case when the driver needs to know buffer > > addresses to enable dma engine and start streaming. For drivers that can > > handle start_streaming without queued buffers (mem2mem and 'one shot' > > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > > introduced. Driver can set it to let videobuf2 know that it support this > > mode. > > Is starting/stopping DMA engines that expensive on most hardware ? Several > mails mentioned that drivers should keep one buffer around to avoid stopping > the DMA engine in case of buffer underrun. The OMAP3 ISP driver just stops the > ISP when it runs out of buffers, and restart it when a new buffer is queued. Yes, this can be expensive. For video capture (e.g. from HDMI) you never want to stop capturing when you run out of buffers. Starting it up again will lead to a 1 or 2 frame delay, which is unacceptable for e.g. video conferencing. And when I start the DMA engine I'd like to know whether only one buffer is queued or if I have two or more. In the latter case I can setup both the 'current' and 'next' pointers in the DMA engine which will make the first frame available quicker (otherwise you will probably get an additional frame delay). > Switching the order of the start_streaming and __enqueue_in_driver calls would > make my life more difficult on the OMAP3 because I will have to check if the > queue is streaming in the qbuf callback. Your s5p-fimc driver has to check for > that as well. I wonder if it really helps for other drivers. Why not add a 'is_streaming' boolean argument to enqueue_in_driver? Regards, Hans > > > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > > vivi) to work properly with the changed order of operations and enables > > use of the newly introduced flag. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > CC: Pawel Osciak <pawel@osciak.com> > > --- > > > > drivers/media/video/mem2mem_testdev.c | 4 +- > > drivers/media/video/s5p-fimc/fimc-capture.c | 65 > > ++++++++++++++++---------- > > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > > drivers/media/video/videobuf2-core.c | 21 ++++----- > > drivers/media/video/vivi.c | 2 +- > > include/media/videobuf2-core.h | 11 +++-- > > 6 files changed, 62 insertions(+), 45 deletions(-) > > > > > > --- > > > > Hello, > > > > This patch introduces significant changes in the vb2 streamon operation, > > so all vb2 clients need to be checked and updated. Right now I didn't > > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > > this patch can be merged, I will update it to include all the required > > changes to these two drivers as well. > > > > Best regards > > -- > > Marek Szyprowski > > Samsung Poland R&D Center > > [snip] > > > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c > > index 2238a61..e740a44 100644 > > --- a/drivers/media/video/vivi.c > > +++ b/drivers/media/video/vivi.c > > @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) > > q = &dev->vb_vidq; > > memset(q, 0, sizeof(dev->vb_vidq)); > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > > + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; > > Why do you remove VB2_USERPTR support from vivi ? > > > q->drv_priv = dev; > > q->buf_struct_size = sizeof(struct vivi_buffer); > > q->ops = &vivi_video_qops; > > -- > Regards, > > Laurent Pinchart > -- > 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 > -- 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
On Wednesday, June 29, 2011 11:49:06 Marek Szyprowski wrote: > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > the order of operations during stream on operation. Now the buffer are > first queued to the driver and then the start_streaming method is called. > This resolves the most common case when the driver needs to know buffer > addresses to enable dma engine and start streaming. For drivers that can > handle start_streaming without queued buffers (mem2mem and 'one shot' > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > introduced. Driver can set it to let videobuf2 know that it support this > mode. > > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > vivi) to work properly with the changed order of operations and enables > use of the newly introduced flag. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > CC: Pawel Osciak <pawel@osciak.com> > --- > > drivers/media/video/mem2mem_testdev.c | 4 +- > drivers/media/video/s5p-fimc/fimc-capture.c | 65 ++++++++++++++++---------- > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > drivers/media/video/videobuf2-core.c | 21 ++++----- > drivers/media/video/vivi.c | 2 +- > include/media/videobuf2-core.h | 11 +++-- > 6 files changed, 62 insertions(+), 45 deletions(-) > > > --- > > Hello, > > This patch introduces significant changes in the vb2 streamon operation, > so all vb2 clients need to be checked and updated. Right now I didn't > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > this patch can be merged, I will update it to include all the required > changes to these two drivers as well. > > Best regards > -- > Marek Szyprowski > Samsung Poland R&D Center > <snip> > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 5517913..911e2eb 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1136,17 +1136,23 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) > } > > /* > - * Cannot start streaming on an OUTPUT device if no buffers have > - * been queued yet. > + * Cannot start streaming if driver requires queued buffers. > */ > - if (V4L2_TYPE_IS_OUTPUT(q->type)) { > + if (!(q->io_flags & VB2_STREAMON_WITHOUT_BUFFERS)) { > if (list_empty(&q->queued_list)) { > - dprintk(1, "streamon: no output buffers queued\n"); > + dprintk(1, "streamon: no buffers queued\n"); > return -EINVAL; > } > } > > /* > + * 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); > + > + /* > * Let driver notice that streaming state has been enabled. > */ > ret = call_qop(q, start_streaming, q); Am I missing something? What is the purpose of this flag? Why not let the driver check in start_streaming whether any buffers have been queued and return -EINVAL if there aren't any? Between setting a flag or just doing the test in start_streaming I would prefer just doing the test. To make it even easier you can perhaps add an extra argument to start_streaming with the number of buffers that are already queued. I can't help thinking that this is made more difficult than it really is. Regards, Hans > @@ -1157,13 +1163,6 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) > > 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); > - > dprintk(3, "Streamon successful\n"); > return 0; > } > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c > index 2238a61..e740a44 100644 > --- a/drivers/media/video/vivi.c > +++ b/drivers/media/video/vivi.c > @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) > q = &dev->vb_vidq; > memset(q, 0, sizeof(dev->vb_vidq)); > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; > q->drv_priv = dev; > q->buf_struct_size = sizeof(struct vivi_buffer); > q->ops = &vivi_video_qops; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index f87472a..cdc0558 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -84,12 +84,15 @@ struct vb2_plane { > * @VB2_USERPTR: driver supports USERPTR with streaming API > * @VB2_READ: driver supports read() style access > * @VB2_WRITE: driver supports write() style access > + * @VB2_STREAMON_WITHOUT_BUFFERS: driver supports stream_on() without buffers > + * queued > */ > enum vb2_io_modes { > - VB2_MMAP = (1 << 0), > - VB2_USERPTR = (1 << 1), > - VB2_READ = (1 << 2), > - VB2_WRITE = (1 << 3), > + VB2_MMAP = (1 << 0), > + VB2_USERPTR = (1 << 1), > + VB2_READ = (1 << 2), > + VB2_WRITE = (1 << 3), > + VB2_STREAMON_WITHOUT_BUFFERS = (1 << 16), > }; > > /** > -- > 1.7.1.569.g6f426 > > -- > 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 > > -- 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
Hello, On Wednesday, June 29, 2011 1:15 PM Hans Verkuil wrote: > On Wednesday, June 29, 2011 11:49:06 Marek Szyprowski wrote: > > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > > the order of operations during stream on operation. Now the buffer are > > first queued to the driver and then the start_streaming method is called. > > This resolves the most common case when the driver needs to know buffer > > addresses to enable dma engine and start streaming. For drivers that can > > handle start_streaming without queued buffers (mem2mem and 'one shot' > > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > > introduced. Driver can set it to let videobuf2 know that it support this > > mode. > > > > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > > vivi) to work properly with the changed order of operations and enables > > use of the newly introduced flag. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > CC: Pawel Osciak <pawel@osciak.com> > > --- > > > > drivers/media/video/mem2mem_testdev.c | 4 +- > > drivers/media/video/s5p-fimc/fimc-capture.c | 65 > ++++++++++++++++---------- > > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > > drivers/media/video/videobuf2-core.c | 21 ++++----- > > drivers/media/video/vivi.c | 2 +- > > include/media/videobuf2-core.h | 11 +++-- > > 6 files changed, 62 insertions(+), 45 deletions(-) > > > > > > --- > > > > Hello, > > > > This patch introduces significant changes in the vb2 streamon operation, > > so all vb2 clients need to be checked and updated. Right now I didn't > > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > > this patch can be merged, I will update it to include all the required > > changes to these two drivers as well. > > > > Best regards > > -- > > Marek Szyprowski > > Samsung Poland R&D Center > > > > <snip> > > > diff --git a/drivers/media/video/videobuf2-core.c > b/drivers/media/video/videobuf2-core.c > > index 5517913..911e2eb 100644 > > --- a/drivers/media/video/videobuf2-core.c > > +++ b/drivers/media/video/videobuf2-core.c > > @@ -1136,17 +1136,23 @@ int vb2_streamon(struct vb2_queue *q, enum > v4l2_buf_type type) > > } > > > > /* > > - * Cannot start streaming on an OUTPUT device if no buffers have > > - * been queued yet. > > + * Cannot start streaming if driver requires queued buffers. > > */ > > - if (V4L2_TYPE_IS_OUTPUT(q->type)) { > > + if (!(q->io_flags & VB2_STREAMON_WITHOUT_BUFFERS)) { > > if (list_empty(&q->queued_list)) { > > - dprintk(1, "streamon: no output buffers queued\n"); > > + dprintk(1, "streamon: no buffers queued\n"); > > return -EINVAL; > > } > > } > > > > /* > > + * 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); > > + > > + /* > > * Let driver notice that streaming state has been enabled. > > */ > > ret = call_qop(q, start_streaming, q); > > Am I missing something? What is the purpose of this flag? Why not let the > driver check in start_streaming whether any buffers have been queued and > return -EINVAL if there aren't any? Between setting a flag or just doing > the test in start_streaming I would prefer just doing the test. > > To make it even easier you can perhaps add an extra argument to > start_streaming with the number of buffers that are already queued. Ok, this sounds reasonable, it looks that I over-engineered it again... > I can't help thinking that this is made more difficult than it really is. Thanks for the idea! Best regards
Hello, On Wednesday, June 29, 2011 1:02 PM Hans Verkuil wrote: > On Wednesday, June 29, 2011 12:44:48 Laurent Pinchart wrote: > > Hi Marek, > > > > On Wednesday 29 June 2011 11:49:06 Marek Szyprowski wrote: > > > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > > > the order of operations during stream on operation. Now the buffer are > > > first queued to the driver and then the start_streaming method is > called. > > > This resolves the most common case when the driver needs to know buffer > > > addresses to enable dma engine and start streaming. For drivers that > can > > > handle start_streaming without queued buffers (mem2mem and 'one shot' > > > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > > > introduced. Driver can set it to let videobuf2 know that it support > this > > > mode. > > > > Is starting/stopping DMA engines that expensive on most hardware ? > Several > > mails mentioned that drivers should keep one buffer around to avoid > stopping > > the DMA engine in case of buffer underrun. The OMAP3 ISP driver just > stops > the > > ISP when it runs out of buffers, and restart it when a new buffer is > queued. > > Yes, this can be expensive. For video capture (e.g. from HDMI) you never > want > to stop capturing when you run out of buffers. Starting it up again will > lead > to a 1 or 2 frame delay, which is unacceptable for e.g. video conferencing. > > And when I start the DMA engine I'd like to know whether only one buffer is > queued or if I have two or more. In the latter case I can setup both the > 'current' and 'next' pointers in the DMA engine which will make the first > frame available quicker (otherwise you will probably get an additional > frame > delay). > > > Switching the order of the start_streaming and __enqueue_in_driver calls > would > > make my life more difficult on the OMAP3 because I will have to check if > the > > queue is streaming in the qbuf callback. Your s5p-fimc driver has to > check > for > > that as well. I wonder if it really helps for other drivers. > > Why not add a 'is_streaming' boolean argument to enqueue_in_driver? Again, thanks for the idea! Best regards
On Wed, 29 Jun 2011 11:49:06 +0200 Marek Szyprowski <m.szyprowski@samsung.com> wrote: > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > the order of operations during stream on operation. Now the buffer are > first queued to the driver and then the start_streaming method is called. Thanks for addressing this. But I do have to wonder if this flag is really necessary. The effort to set a "they want to start streaming" flag and start things for real in buf_queue() is not *that* great; if doing so avoids causing failures in applications which are following the rules, it seems worth it. > /* > + * 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); > + > + /* > * Let driver notice that streaming state has been enabled. > */ > ret = call_qop(q, start_streaming, q); I do still wonder why this is an issue - why not pass the buffers through to the driver at VIDIOC_QBUF time? I assume there must be a reason for doing things this way, I'd like to understand what it is. 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
Hello, On Wednesday, June 29, 2011 3:26 PM wrote: > On Wed, 29 Jun 2011 11:49:06 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > > the order of operations during stream on operation. Now the buffer are > > first queued to the driver and then the start_streaming method is called. > > Thanks for addressing this. But I do have to wonder if this flag is really > necessary. The effort to set a "they want to start streaming" flag and > start things for real in buf_queue() is not *that* great; if doing so > avoids causing failures in applications which are following the rules, it > seems worth it. > > > /* > > + * 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); > > + > > + /* > > * Let driver notice that streaming state has been enabled. > > */ > > ret = call_qop(q, start_streaming, q); > > I do still wonder why this is an issue - why not pass the buffers through > to the driver at VIDIOC_QBUF time? I assume there must be a reason for > doing things this way, I'd like to understand what it is. I want to delay giving the ownership of the buffers to the driver until it is certain that start_streaming method will be called. This way I achieve a well defined states of the queued buffers: 1. successful start_streaming() -> the driver is processing the queue buffers 2. unsuccessful start_streaming() -> the driver is responsible to discard all queued buffers 3. stop_streaming() called -> the driver has finished or discarded all queued buffers If we assume that the buffers are given to the driver in QBUF then we need to address somehow the following call sequence: 1. REQBUFS(n1) -> allocated n1 buffers 2. QBUF(buffer 1) -> gives buffer to the driver 3. QBUF(buffer 2) -> gives buffer to the driver 4. REQBUFS(n2) -> frees all buffers (driver still keeps 2 already queued buffers) and allocates n2 new buffers There is no way to tell the driver to discard the old buffers. Additional call to stop_streaming method might solve it, but I really don't like to abuse it here if delaying the ownership transfer just solves the issue. If you ask why there is a start_streaming callback at all then the reason is simple - I would like to have a single place in the driver, where the streaming is started (streaming might be started as a result of STREAMON ioctl, read() call or recently discussed poll()). Best regards
On Wed, 29 Jun 2011 16:10:45 +0200 Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > I do still wonder why this is an issue - why not pass the buffers through > > to the driver at VIDIOC_QBUF time? I assume there must be a reason for > > doing things this way, I'd like to understand what it is. > > I want to delay giving the ownership of the buffers to the driver until it > is certain that start_streaming method will be called. This way I achieve > a well defined states of the queued buffers: > > 1. successful start_streaming() -> the driver is processing the queue buffers > 2. unsuccessful start_streaming() -> the driver is responsible to discard all > queued buffers > 3. stop_streaming() called -> the driver has finished or discarded all queued > buffers So it's a buffer ownership thing. I wonder if there would be value in adding a buf_give_them_all_back_now() callback? You have an implicit change of buffer ownership now that seems easy for drivers to mess up. It might be better to send an explicit signal at such times and, perhaps, even require the driver to explicitly hand each buffer back to vb2? That would make the rules clear and give some flexibility - stopping and starting streaming without needing to start over with buffers, for example. Dunno, I'm just sort of babbling as I think; what's there now clearly works. 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
Hello, On Friday, July 01, 2011 12:18 AM Jonathan Corbet wrote: > On Wed, 29 Jun 2011 16:10:45 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > > > I do still wonder why this is an issue - why not pass the buffers > through > > > to the driver at VIDIOC_QBUF time? I assume there must be a reason for > > > doing things this way, I'd like to understand what it is. > > > > I want to delay giving the ownership of the buffers to the driver until > it > > is certain that start_streaming method will be called. This way I achieve > > a well defined states of the queued buffers: > > > > 1. successful start_streaming() -> the driver is processing the queue > buffers > > 2. unsuccessful start_streaming() -> the driver is responsible to discard > all > > queued buffers > > 3. stop_streaming() called -> the driver has finished or discarded all > queued > > buffers > > So it's a buffer ownership thing. I wonder if there would be value in > adding a buf_give_them_all_back_now() callback? You have an implicit > change of buffer ownership now that seems easy for drivers to mess up. It > might be better to send an explicit signal at such times and, perhaps, > even require the driver to explicitly hand each buffer back to vb2? That > would make the rules clear and give some flexibility - stopping and > starting streaming without needing to start over with buffers, for example. You are right that this will make the rules more clear, but wonder if we really need it now in videbuf2. The current V4L2 API states that all buffers are automatically discarded after calling STREAM_OFF, so it is not really possible to implement true pause-like functionality. Maybe we should schedule this for V4L3? ;) Best regards
Hi Jon and Marek, On Thu, Jun 30, 2011 at 15:18, Jonathan Corbet <corbet@lwn.net> wrote: > On Wed, 29 Jun 2011 16:10:45 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> wrote: > >> > I do still wonder why this is an issue - why not pass the buffers through >> > to the driver at VIDIOC_QBUF time? I assume there must be a reason for >> > doing things this way, I'd like to understand what it is. >> >> I want to delay giving the ownership of the buffers to the driver until it >> is certain that start_streaming method will be called. This way I achieve >> a well defined states of the queued buffers: >> >> 1. successful start_streaming() -> the driver is processing the queue buffers >> 2. unsuccessful start_streaming() -> the driver is responsible to discard all >> queued buffers >> 3. stop_streaming() called -> the driver has finished or discarded all queued >> buffers > > So it's a buffer ownership thing. I wonder if there would be value in > adding a buf_give_them_all_back_now() callback? You have an implicit > change of buffer ownership now that seems easy for drivers to mess up. It > might be better to send an explicit signal at such times and, perhaps, > even require the driver to explicitly hand each buffer back to vb2? That > would make the rules clear and give some flexibility - stopping and > starting streaming without needing to start over with buffers, for example. > > Dunno, I'm just sort of babbling as I think; what's there now clearly > works. > The original reason behind not passing buffers to the driver at QBUF time was very simple: a driver, when given a buffer by videobuf, was supposed to start using it right away. The drivers did not have to have a notion of streaming state, which was to be managed entirely in videobuf. This greatly simplified drivers, as their logic needed only to be: "on buf_queue() start processing immediately and give back the buffer as soon as finished". This approach was assuming that enabling/disabling a device (e.g. making it start or stop capturing) was fast and simple though. And it simplified cancelling/destroying the queue. So yes, it was an ownership thing (and still is). The reality seems to be proving different, the drivers want to know about streaming, as enabling and disabling their devices can be complicated and/or may be taking a significant amount of time (too long to be done on QBUF without slowing down things). On the other hand, V4L2 allows starting streaming without queuing any buffers (not for OUTPUT devices though, for obvious reasons). At first, videobuf would not allow STREAMON only for OUTPUT devices if no buffers were queued. Now it looks like most would prefer, by default, disallowing STREAMON with no buffers queued even for CAPTURE devices, unless a driver requests otherwise (the new flag in the above patch). The "buf_give_them_all_back_now()" callback is already there actually. It's stop_streaming(). That's why stop_streaming is required, while start_streaming is not. I'll try to clarify that in my incoming documentation patches. I like the general idea of this patch, making the behavior configurable by a driver. We can still have simple drivers that don't care about streaming state this way. Although I'm not sure which variant should be the default. I agree that a better idea would be to make driver check for it itself in start_streaming. Makes it simpler, and there would be no default either. But there is one catch: if we allow buf_queue() to be called without streaming on, all drivers will have to implement start_streaming, so it will have to become mandatory. Am I right Marek?
Hi Marek, Are you planning a RFCv2 for this? I've been implementing vb2 in an internal driver and this initialization order of vb2 is a bit of a pain to be honest. Regards, Hans On Wednesday, June 29, 2011 11:49:06 Marek Szyprowski wrote: > This patch introduces VB2_STREAMON_WITHOUT_BUFFERS io flag and changes > the order of operations during stream on operation. Now the buffer are > first queued to the driver and then the start_streaming method is called. > This resolves the most common case when the driver needs to know buffer > addresses to enable dma engine and start streaming. For drivers that can > handle start_streaming without queued buffers (mem2mem and 'one shot' > capture case) a new VB2_STREAMON_WITHOUT_BUFFERS io flag has been > introduced. Driver can set it to let videobuf2 know that it support this > mode. > > This patch also updates videobuf2 clients (s5p-fimc, mem2mem_testdev and > vivi) to work properly with the changed order of operations and enables > use of the newly introduced flag. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > CC: Pawel Osciak <pawel@osciak.com> > --- > > drivers/media/video/mem2mem_testdev.c | 4 +- > drivers/media/video/s5p-fimc/fimc-capture.c | 65 ++++++++++++++++---------- > drivers/media/video/s5p-fimc/fimc-core.c | 4 +- > drivers/media/video/videobuf2-core.c | 21 ++++----- > drivers/media/video/vivi.c | 2 +- > include/media/videobuf2-core.h | 11 +++-- > 6 files changed, 62 insertions(+), 45 deletions(-) > > > --- > > Hello, > > This patch introduces significant changes in the vb2 streamon operation, > so all vb2 clients need to be checked and updated. Right now I didn't > update mx3_camera and sh_mobile_ceu_camera drivers. Once we agree that > this patch can be merged, I will update it to include all the required > changes to these two drivers as well. > > Best regards > -- > Marek Szyprowski > Samsung Poland R&D Center > > diff --git a/drivers/media/video/mem2mem_testdev.c b/drivers/media/video/mem2mem_testdev.c > index b03d74e..65fb4ad 100644 > --- a/drivers/media/video/mem2mem_testdev.c > +++ b/drivers/media/video/mem2mem_testdev.c > @@ -808,7 +808,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds > > memset(src_vq, 0, sizeof(*src_vq)); > src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; > - src_vq->io_modes = VB2_MMAP; > + src_vq->io_modes = VB2_MMAP | VB2_STREAMON_WITHOUT_BUFFERS; > src_vq->drv_priv = ctx; > src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); > src_vq->ops = &m2mtest_qops; > @@ -820,7 +820,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds > > memset(dst_vq, 0, sizeof(*dst_vq)); > dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > - dst_vq->io_modes = VB2_MMAP; > + dst_vq->io_modes = VB2_MMAP | VB2_STREAMON_WITHOUT_BUFFERS; > dst_vq->drv_priv = ctx; > dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); > dst_vq->ops = &m2mtest_qops; > diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c > index d142b40..20a5bd4 100644 > --- a/drivers/media/video/s5p-fimc/fimc-capture.c > +++ b/drivers/media/video/s5p-fimc/fimc-capture.c > @@ -152,27 +152,11 @@ static int fimc_isp_subdev_init(struct fimc_dev *fimc, unsigned int index) > return ret; > } > > -static int fimc_stop_capture(struct fimc_dev *fimc) > +static void fimc_capture_state_cleanup(struct fimc_dev *fimc) > { > - unsigned long flags; > - struct fimc_vid_cap *cap; > + struct fimc_vid_cap *cap = &fimc->vid_cap; > struct fimc_vid_buffer *buf; > - > - cap = &fimc->vid_cap; > - > - if (!fimc_capture_active(fimc)) > - return 0; > - > - spin_lock_irqsave(&fimc->slock, flags); > - set_bit(ST_CAPT_SHUT, &fimc->state); > - fimc_deactivate_capture(fimc); > - spin_unlock_irqrestore(&fimc->slock, flags); > - > - wait_event_timeout(fimc->irq_queue, > - !test_bit(ST_CAPT_SHUT, &fimc->state), > - FIMC_SHUTDOWN_TIMEOUT); > - > - v4l2_subdev_call(cap->sd, video, s_stream, 0); > + unsigned long flags; > > spin_lock_irqsave(&fimc->slock, flags); > fimc->state &= ~(1 << ST_CAPT_RUN | 1 << ST_CAPT_PEND | > @@ -192,27 +176,50 @@ static int fimc_stop_capture(struct fimc_dev *fimc) > } > > spin_unlock_irqrestore(&fimc->slock, flags); > +} > + > +static int fimc_stop_capture(struct fimc_dev *fimc) > +{ > + struct fimc_vid_cap *cap = &fimc->vid_cap; > + unsigned long flags; > + > + if (!fimc_capture_active(fimc)) > + return 0; > + > + spin_lock_irqsave(&fimc->slock, flags); > + set_bit(ST_CAPT_SHUT, &fimc->state); > + fimc_deactivate_capture(fimc); > + spin_unlock_irqrestore(&fimc->slock, flags); > + > + wait_event_timeout(fimc->irq_queue, > + !test_bit(ST_CAPT_SHUT, &fimc->state), > + FIMC_SHUTDOWN_TIMEOUT); > > + v4l2_subdev_call(cap->sd, video, s_stream, 0); > + > + fimc_capture_state_cleanup(fimc); > dbg("state: 0x%lx", fimc->state); > return 0; > } > > + > static int start_streaming(struct vb2_queue *q) > { > struct fimc_ctx *ctx = q->drv_priv; > struct fimc_dev *fimc = ctx->fimc_dev; > struct s5p_fimc_isp_info *isp_info; > + int min_bufs; > int ret; > > fimc_hw_reset(fimc); > > ret = v4l2_subdev_call(fimc->vid_cap.sd, video, s_stream, 1); > if (ret && ret != -ENOIOCTLCMD) > - return ret; > + goto error; > > ret = fimc_prepare_config(ctx, ctx->state); > if (ret) > - return ret; > + goto error; > > isp_info = &fimc->pdata->isp_info[fimc->vid_cap.input_index]; > fimc_hw_set_camera_type(fimc, isp_info); > @@ -223,7 +230,7 @@ static int start_streaming(struct vb2_queue *q) > ret = fimc_set_scaler_info(ctx); > if (ret) { > err("Scaler setup error"); > - return ret; > + goto error; > } > fimc_hw_set_input_path(ctx); > fimc_hw_set_prescaler(ctx); > @@ -238,13 +245,20 @@ static int start_streaming(struct vb2_queue *q) > > INIT_LIST_HEAD(&fimc->vid_cap.pending_buf_q); > INIT_LIST_HEAD(&fimc->vid_cap.active_buf_q); > - fimc->vid_cap.active_buf_cnt = 0; > fimc->vid_cap.frame_count = 0; > fimc->vid_cap.buf_index = 0; > > set_bit(ST_CAPT_PEND, &fimc->state); > > + min_bufs = fimc->vid_cap.reqbufs_count > 1 ? 2 : 1; > + > + if (fimc->vid_cap.active_buf_cnt >= min_bufs) > + fimc_activate_capture(ctx); > + > return 0; > +error: > + fimc_capture_state_cleanup(fimc); > + return ret; > } > > static int stop_streaming(struct vb2_queue *q) > @@ -357,7 +371,8 @@ static void buffer_queue(struct vb2_buffer *vb) > > min_bufs = vid_cap->reqbufs_count > 1 ? 2 : 1; > > - if (vid_cap->active_buf_cnt >= min_bufs && > + if (vb2_is_streaming(vb->vb2_queue) && > + vid_cap->active_buf_cnt >= min_bufs && > !test_and_set_bit(ST_CAPT_STREAM, &fimc->state)) > fimc_activate_capture(ctx); > > @@ -878,7 +893,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc) > q = &fimc->vid_cap.vbq; > memset(q, 0, sizeof(*q)); > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > - q->io_modes = VB2_MMAP | VB2_USERPTR; > + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; > q->drv_priv = fimc->vid_cap.ctx; > q->ops = &fimc_capture_qops; > q->mem_ops = &vb2_dma_contig_memops; > diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c > index dc91a85..6a405c8 100644 > --- a/drivers/media/video/s5p-fimc/fimc-core.c > +++ b/drivers/media/video/s5p-fimc/fimc-core.c > @@ -1386,7 +1386,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, > > memset(src_vq, 0, sizeof(*src_vq)); > src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > - src_vq->io_modes = VB2_MMAP | VB2_USERPTR; > + src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; > src_vq->drv_priv = ctx; > src_vq->ops = &fimc_qops; > src_vq->mem_ops = &vb2_dma_contig_memops; > @@ -1398,7 +1398,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, > > memset(dst_vq, 0, sizeof(*dst_vq)); > dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > - dst_vq->io_modes = VB2_MMAP | VB2_USERPTR; > + dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; > dst_vq->drv_priv = ctx; > dst_vq->ops = &fimc_qops; > dst_vq->mem_ops = &vb2_dma_contig_memops; > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 5517913..911e2eb 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1136,17 +1136,23 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) > } > > /* > - * Cannot start streaming on an OUTPUT device if no buffers have > - * been queued yet. > + * Cannot start streaming if driver requires queued buffers. > */ > - if (V4L2_TYPE_IS_OUTPUT(q->type)) { > + if (!(q->io_flags & VB2_STREAMON_WITHOUT_BUFFERS)) { > if (list_empty(&q->queued_list)) { > - dprintk(1, "streamon: no output buffers queued\n"); > + dprintk(1, "streamon: no buffers queued\n"); > return -EINVAL; > } > } > > /* > + * 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); > + > + /* > * Let driver notice that streaming state has been enabled. > */ > ret = call_qop(q, start_streaming, q); > @@ -1157,13 +1163,6 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) > > 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); > - > dprintk(3, "Streamon successful\n"); > return 0; > } > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c > index 2238a61..e740a44 100644 > --- a/drivers/media/video/vivi.c > +++ b/drivers/media/video/vivi.c > @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) > q = &dev->vb_vidq; > memset(q, 0, sizeof(dev->vb_vidq)); > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; > q->drv_priv = dev; > q->buf_struct_size = sizeof(struct vivi_buffer); > q->ops = &vivi_video_qops; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index f87472a..cdc0558 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -84,12 +84,15 @@ struct vb2_plane { > * @VB2_USERPTR: driver supports USERPTR with streaming API > * @VB2_READ: driver supports read() style access > * @VB2_WRITE: driver supports write() style access > + * @VB2_STREAMON_WITHOUT_BUFFERS: driver supports stream_on() without buffers > + * queued > */ > enum vb2_io_modes { > - VB2_MMAP = (1 << 0), > - VB2_USERPTR = (1 << 1), > - VB2_READ = (1 << 2), > - VB2_WRITE = (1 << 3), > + VB2_MMAP = (1 << 0), > + VB2_USERPTR = (1 << 1), > + VB2_READ = (1 << 2), > + VB2_WRITE = (1 << 3), > + VB2_STREAMON_WITHOUT_BUFFERS = (1 << 16), > }; > > /** > -- > 1.7.1.569.g6f426 > -- 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
Hello, On Tuesday, August 23, 2011 12:11 PM Hans Verkuil wrote: > Are you planning a RFCv2 for this? > > I've been implementing vb2 in an internal driver and this initialization > order of vb2 is a bit of a pain to be honest. (snipped) Yes, I will post it till the end of the week. I'm sorry for the delay, I was a bit busy with updating CMA and dma-mapping patches... Best regards
On Tuesday, August 23, 2011 12:14:17 Marek Szyprowski wrote: > Hello, > > On Tuesday, August 23, 2011 12:11 PM Hans Verkuil wrote: > > > Are you planning a RFCv2 for this? > > > > I've been implementing vb2 in an internal driver and this initialization > > order of vb2 is a bit of a pain to be honest. > > (snipped) > > Yes, I will post it till the end of the week. I'm sorry for the delay, I was > a bit busy with updating CMA and dma-mapping patches... No problem, I just wanted to make sure it wasn't lost... I've found a few other issues as well, I'll post those separately. Regards, Hans > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/mem2mem_testdev.c b/drivers/media/video/mem2mem_testdev.c index b03d74e..65fb4ad 100644 --- a/drivers/media/video/mem2mem_testdev.c +++ b/drivers/media/video/mem2mem_testdev.c @@ -808,7 +808,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds memset(src_vq, 0, sizeof(*src_vq)); src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; - src_vq->io_modes = VB2_MMAP; + src_vq->io_modes = VB2_MMAP | VB2_STREAMON_WITHOUT_BUFFERS; src_vq->drv_priv = ctx; src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); src_vq->ops = &m2mtest_qops; @@ -820,7 +820,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds memset(dst_vq, 0, sizeof(*dst_vq)); dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - dst_vq->io_modes = VB2_MMAP; + dst_vq->io_modes = VB2_MMAP | VB2_STREAMON_WITHOUT_BUFFERS; dst_vq->drv_priv = ctx; dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); dst_vq->ops = &m2mtest_qops; diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c index d142b40..20a5bd4 100644 --- a/drivers/media/video/s5p-fimc/fimc-capture.c +++ b/drivers/media/video/s5p-fimc/fimc-capture.c @@ -152,27 +152,11 @@ static int fimc_isp_subdev_init(struct fimc_dev *fimc, unsigned int index) return ret; } -static int fimc_stop_capture(struct fimc_dev *fimc) +static void fimc_capture_state_cleanup(struct fimc_dev *fimc) { - unsigned long flags; - struct fimc_vid_cap *cap; + struct fimc_vid_cap *cap = &fimc->vid_cap; struct fimc_vid_buffer *buf; - - cap = &fimc->vid_cap; - - if (!fimc_capture_active(fimc)) - return 0; - - spin_lock_irqsave(&fimc->slock, flags); - set_bit(ST_CAPT_SHUT, &fimc->state); - fimc_deactivate_capture(fimc); - spin_unlock_irqrestore(&fimc->slock, flags); - - wait_event_timeout(fimc->irq_queue, - !test_bit(ST_CAPT_SHUT, &fimc->state), - FIMC_SHUTDOWN_TIMEOUT); - - v4l2_subdev_call(cap->sd, video, s_stream, 0); + unsigned long flags; spin_lock_irqsave(&fimc->slock, flags); fimc->state &= ~(1 << ST_CAPT_RUN | 1 << ST_CAPT_PEND | @@ -192,27 +176,50 @@ static int fimc_stop_capture(struct fimc_dev *fimc) } spin_unlock_irqrestore(&fimc->slock, flags); +} + +static int fimc_stop_capture(struct fimc_dev *fimc) +{ + struct fimc_vid_cap *cap = &fimc->vid_cap; + unsigned long flags; + + if (!fimc_capture_active(fimc)) + return 0; + + spin_lock_irqsave(&fimc->slock, flags); + set_bit(ST_CAPT_SHUT, &fimc->state); + fimc_deactivate_capture(fimc); + spin_unlock_irqrestore(&fimc->slock, flags); + + wait_event_timeout(fimc->irq_queue, + !test_bit(ST_CAPT_SHUT, &fimc->state), + FIMC_SHUTDOWN_TIMEOUT); + v4l2_subdev_call(cap->sd, video, s_stream, 0); + + fimc_capture_state_cleanup(fimc); dbg("state: 0x%lx", fimc->state); return 0; } + static int start_streaming(struct vb2_queue *q) { struct fimc_ctx *ctx = q->drv_priv; struct fimc_dev *fimc = ctx->fimc_dev; struct s5p_fimc_isp_info *isp_info; + int min_bufs; int ret; fimc_hw_reset(fimc); ret = v4l2_subdev_call(fimc->vid_cap.sd, video, s_stream, 1); if (ret && ret != -ENOIOCTLCMD) - return ret; + goto error; ret = fimc_prepare_config(ctx, ctx->state); if (ret) - return ret; + goto error; isp_info = &fimc->pdata->isp_info[fimc->vid_cap.input_index]; fimc_hw_set_camera_type(fimc, isp_info); @@ -223,7 +230,7 @@ static int start_streaming(struct vb2_queue *q) ret = fimc_set_scaler_info(ctx); if (ret) { err("Scaler setup error"); - return ret; + goto error; } fimc_hw_set_input_path(ctx); fimc_hw_set_prescaler(ctx); @@ -238,13 +245,20 @@ static int start_streaming(struct vb2_queue *q) INIT_LIST_HEAD(&fimc->vid_cap.pending_buf_q); INIT_LIST_HEAD(&fimc->vid_cap.active_buf_q); - fimc->vid_cap.active_buf_cnt = 0; fimc->vid_cap.frame_count = 0; fimc->vid_cap.buf_index = 0; set_bit(ST_CAPT_PEND, &fimc->state); + min_bufs = fimc->vid_cap.reqbufs_count > 1 ? 2 : 1; + + if (fimc->vid_cap.active_buf_cnt >= min_bufs) + fimc_activate_capture(ctx); + return 0; +error: + fimc_capture_state_cleanup(fimc); + return ret; } static int stop_streaming(struct vb2_queue *q) @@ -357,7 +371,8 @@ static void buffer_queue(struct vb2_buffer *vb) min_bufs = vid_cap->reqbufs_count > 1 ? 2 : 1; - if (vid_cap->active_buf_cnt >= min_bufs && + if (vb2_is_streaming(vb->vb2_queue) && + vid_cap->active_buf_cnt >= min_bufs && !test_and_set_bit(ST_CAPT_STREAM, &fimc->state)) fimc_activate_capture(ctx); @@ -878,7 +893,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc) q = &fimc->vid_cap.vbq; memset(q, 0, sizeof(*q)); q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; - q->io_modes = VB2_MMAP | VB2_USERPTR; + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; q->drv_priv = fimc->vid_cap.ctx; q->ops = &fimc_capture_qops; q->mem_ops = &vb2_dma_contig_memops; diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c index dc91a85..6a405c8 100644 --- a/drivers/media/video/s5p-fimc/fimc-core.c +++ b/drivers/media/video/s5p-fimc/fimc-core.c @@ -1386,7 +1386,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, memset(src_vq, 0, sizeof(*src_vq)); src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; - src_vq->io_modes = VB2_MMAP | VB2_USERPTR; + src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; src_vq->drv_priv = ctx; src_vq->ops = &fimc_qops; src_vq->mem_ops = &vb2_dma_contig_memops; @@ -1398,7 +1398,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, memset(dst_vq, 0, sizeof(*dst_vq)); dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; - dst_vq->io_modes = VB2_MMAP | VB2_USERPTR; + dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_STREAMON_WITHOUT_BUFFERS; dst_vq->drv_priv = ctx; dst_vq->ops = &fimc_qops; dst_vq->mem_ops = &vb2_dma_contig_memops; diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index 5517913..911e2eb 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1136,17 +1136,23 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) } /* - * Cannot start streaming on an OUTPUT device if no buffers have - * been queued yet. + * Cannot start streaming if driver requires queued buffers. */ - if (V4L2_TYPE_IS_OUTPUT(q->type)) { + if (!(q->io_flags & VB2_STREAMON_WITHOUT_BUFFERS)) { if (list_empty(&q->queued_list)) { - dprintk(1, "streamon: no output buffers queued\n"); + dprintk(1, "streamon: no buffers queued\n"); return -EINVAL; } } /* + * 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); + + /* * Let driver notice that streaming state has been enabled. */ ret = call_qop(q, start_streaming, q); @@ -1157,13 +1163,6 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type) 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); - dprintk(3, "Streamon successful\n"); return 0; } diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 2238a61..e740a44 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -1232,7 +1232,7 @@ static int __init vivi_create_instance(int inst) q = &dev->vb_vidq; memset(q, 0, sizeof(dev->vb_vidq)); q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; + q->io_modes = VB2_MMAP | VB2_READ | VB2_STREAMON_WITHOUT_BUFFERS; q->drv_priv = dev; q->buf_struct_size = sizeof(struct vivi_buffer); q->ops = &vivi_video_qops; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index f87472a..cdc0558 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -84,12 +84,15 @@ struct vb2_plane { * @VB2_USERPTR: driver supports USERPTR with streaming API * @VB2_READ: driver supports read() style access * @VB2_WRITE: driver supports write() style access + * @VB2_STREAMON_WITHOUT_BUFFERS: driver supports stream_on() without buffers + * queued */ enum vb2_io_modes { - VB2_MMAP = (1 << 0), - VB2_USERPTR = (1 << 1), - VB2_READ = (1 << 2), - VB2_WRITE = (1 << 3), + VB2_MMAP = (1 << 0), + VB2_USERPTR = (1 << 1), + VB2_READ = (1 << 2), + VB2_WRITE = (1 << 3), + VB2_STREAMON_WITHOUT_BUFFERS = (1 << 16), }; /**