[PATCH/RFC] media: vb2: change queue initialization order

[PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Hans Verkuil]

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
> 

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Hans Verkuil]

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

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Jonathan Corbet]

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

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Jonathan Corbet]

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

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Pawel Osciak]

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?

-- 
Best regards,
Pawel Osciak

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Hans Verkuil]

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
> 

RE: [PATCH/RFC] media: vb2: change queue initialization order

[Marek Szyprowski]

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
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH/RFC] media: vb2: change queue initialization order

[Hans Verkuil]

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