vb2: various small fixes/improvements

vb2: various small fixes/improvements

[Hans Verkuil]

This patch series contains a list of various vb2 fixes and improvements.

These patches were originally part of this RFC patch series:

http://www.spinics.net/lists/linux-media/msg73391.html

They are now rebased and reordered a bit. It's little stuff for the
most part, although the first patch touches on more drivers since it
changes the return type of stop_streaming to void. The return value was
always ignored by vb2 and you really cannot do anything sensible with it.
In general resource allocations can return an error, but freeing up resources
should not. That should always succeed.

Regards,

	Hans

[REVIEW PATCH 01/11] vb2: stop_streaming should return void

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The vb2 core ignores any return code from the stop_streaming op.
And there really isn't anything it can do anyway in case of an error.
So change the return type to void and update any drivers that implement it.

The int return gave drivers the idea that this operation could actually
fail, but that's really not the case.

The pwc amd sdr-msi3101 drivers both had this construction:

 	if (mutex_lock_interruptible(&s->v4l2_lock))
		return -ERESTARTSYS;

This has been updated to just call mutex_lock(). The stop_streaming op
expects this to really stop streaming and I very much doubt this will
work reliably if stop_streaming just returns without really stopping the
DMA.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/pci/sta2x11/sta2x11_vip.c                  | 3 +--
 drivers/media/platform/blackfin/bfin_capture.c           | 6 +-----
 drivers/media/platform/coda.c                            | 4 +---
 drivers/media/platform/exynos-gsc/gsc-m2m.c              | 4 +---
 drivers/media/platform/exynos4-is/fimc-capture.c         | 6 +++---
 drivers/media/platform/exynos4-is/fimc-lite.c            | 6 +++---
 drivers/media/platform/exynos4-is/fimc-m2m.c             | 3 +--
 drivers/media/platform/marvell-ccic/mcam-core.c          | 7 +++----
 drivers/media/platform/s3c-camif/camif-capture.c         | 4 ++--
 drivers/media/platform/s5p-jpeg/jpeg-core.c              | 4 +---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c             | 3 +--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c             | 3 +--
 drivers/media/platform/s5p-tv/mixer_video.c              | 3 +--
 drivers/media/platform/soc_camera/atmel-isi.c            | 6 ++----
 drivers/media/platform/soc_camera/mx2_camera.c           | 4 +---
 drivers/media/platform/soc_camera/mx3_camera.c           | 4 +---
 drivers/media/platform/soc_camera/rcar_vin.c             | 4 +---
 drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c | 4 ++--
 drivers/media/platform/vivi.c                            | 3 +--
 drivers/media/platform/vsp1/vsp1_video.c                 | 4 +---
 drivers/media/usb/em28xx/em28xx-v4l.h                    | 2 +-
 drivers/media/usb/em28xx/em28xx-video.c                  | 8 ++------
 drivers/media/usb/pwc/pwc-if.c                           | 7 ++-----
 drivers/media/usb/s2255/s2255drv.c                       | 5 ++---
 drivers/media/usb/stk1160/stk1160-v4l.c                  | 4 ++--
 drivers/media/usb/usbtv/usbtv-video.c                    | 9 +++------
 drivers/staging/media/davinci_vpfe/vpfe_video.c          | 3 +--
 drivers/staging/media/dt3155v4l/dt3155v4l.c              | 3 +--
 drivers/staging/media/go7007/go7007-v4l2.c               | 3 +--
 drivers/staging/media/msi3101/sdr-msi3101.c              | 7 ++-----
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c       | 3 +--
 drivers/staging/media/solo6x10/solo6x10-v4l2.c           | 3 +--
 include/media/videobuf2-core.h                           | 2 +-
 33 files changed, 49 insertions(+), 95 deletions(-)

diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
index bb11443..7559951 100644
--- a/drivers/media/pci/sta2x11/sta2x11_vip.c
+++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
@@ -357,7 +357,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 }
 
 /* abort streaming and wait for last buffer */
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct sta2x11_vip *vip = vb2_get_drv_priv(vq);
 	struct vip_buffer *vip_buf, *node;
@@ -374,7 +374,6 @@ static int stop_streaming(struct vb2_queue *vq)
 		list_del(&vip_buf->list);
 	}
 	spin_unlock(&vip->lock);
-	return 0;
 }
 
 static struct vb2_ops vip_video_qops = {
diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
index 200bec9..16f643c 100644
--- a/drivers/media/platform/blackfin/bfin_capture.c
+++ b/drivers/media/platform/blackfin/bfin_capture.c
@@ -427,15 +427,12 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 }
 
-static int bcap_stop_streaming(struct vb2_queue *vq)
+static void bcap_stop_streaming(struct vb2_queue *vq)
 {
 	struct bcap_device *bcap_dev = vb2_get_drv_priv(vq);
 	struct ppi_if *ppi = bcap_dev->ppi;
 	int ret;
 
-	if (!vb2_is_streaming(vq))
-		return 0;
-
 	bcap_dev->stop = true;
 	wait_for_completion(&bcap_dev->comp);
 	ppi->ops->stop(ppi);
@@ -452,7 +449,6 @@ static int bcap_stop_streaming(struct vb2_queue *vq)
 		list_del(&bcap_dev->cur_frm->list);
 		vb2_buffer_done(&bcap_dev->cur_frm->vb, VB2_BUF_STATE_ERROR);
 	}
-	return 0;
 }
 
 static struct vb2_ops bcap_video_qops = {
diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
index 3e5199e..d9b1a04 100644
--- a/drivers/media/platform/coda.c
+++ b/drivers/media/platform/coda.c
@@ -2269,7 +2269,7 @@ out:
 	return ret;
 }
 
-static int coda_stop_streaming(struct vb2_queue *q)
+static void coda_stop_streaming(struct vb2_queue *q)
 {
 	struct coda_ctx *ctx = vb2_get_drv_priv(q);
 	struct coda_dev *dev = ctx->dev;
@@ -2295,8 +2295,6 @@ static int coda_stop_streaming(struct vb2_queue *q)
 			ctx->bitstream.vaddr, ctx->bitstream.size);
 		ctx->runcounter = 0;
 	}
-
-	return 0;
 }
 
 static struct vb2_ops coda_qops = {
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index d0ea94f..e434f1f0 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -66,15 +66,13 @@ static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret > 0 ? 0 : ret;
 }
 
-static int gsc_m2m_stop_streaming(struct vb2_queue *q)
+static void gsc_m2m_stop_streaming(struct vb2_queue *q)
 {
 	struct gsc_ctx *ctx = q->drv_priv;
 
 	__gsc_m2m_job_abort(ctx);
 
 	pm_runtime_put(&ctx->gsc_dev->pdev->dev);
-
-	return 0;
 }
 
 void gsc_m2m_job_finish(struct gsc_ctx *ctx, int vb_state)
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 92ae812..3d2babd 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -294,15 +294,15 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 }
 
-static int stop_streaming(struct vb2_queue *q)
+static void stop_streaming(struct vb2_queue *q)
 {
 	struct fimc_ctx *ctx = q->drv_priv;
 	struct fimc_dev *fimc = ctx->fimc_dev;
 
 	if (!fimc_capture_active(fimc))
-		return -EINVAL;
+		return;
 
-	return fimc_stop_capture(fimc, false);
+	fimc_stop_capture(fimc, false);
 }
 
 int fimc_capture_suspend(struct fimc_dev *fimc)
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index 3ad660b..630aef5 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -350,14 +350,14 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 }
 
-static int stop_streaming(struct vb2_queue *q)
+static void stop_streaming(struct vb2_queue *q)
 {
 	struct fimc_lite *fimc = q->drv_priv;
 
 	if (!fimc_lite_active(fimc))
-		return -EINVAL;
+		return;
 
-	return fimc_lite_stop_capture(fimc, false);
+	fimc_lite_stop_capture(fimc, false);
 }
 
 static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 36971d9..d314155 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -85,7 +85,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret > 0 ? 0 : ret;
 }
 
-static int stop_streaming(struct vb2_queue *q)
+static void stop_streaming(struct vb2_queue *q)
 {
 	struct fimc_ctx *ctx = q->drv_priv;
 	int ret;
@@ -95,7 +95,6 @@ static int stop_streaming(struct vb2_queue *q)
 		fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
 
 	pm_runtime_put(&ctx->fimc_dev->pdev->dev);
-	return 0;
 }
 
 static void fimc_device_run(void *priv)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 8b34c48..be4b512 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1156,7 +1156,7 @@ static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return mcam_read_setup(cam);
 }
 
-static int mcam_vb_stop_streaming(struct vb2_queue *vq)
+static void mcam_vb_stop_streaming(struct vb2_queue *vq)
 {
 	struct mcam_camera *cam = vb2_get_drv_priv(vq);
 	unsigned long flags;
@@ -1164,10 +1164,10 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
 	if (cam->state == S_BUFWAIT) {
 		/* They never gave us buffers */
 		cam->state = S_IDLE;
-		return 0;
+		return;
 	}
 	if (cam->state != S_STREAMING)
-		return -EINVAL;
+		return;
 	mcam_ctlr_stop_dma(cam);
 	/*
 	 * Reset the CCIC PHY after stopping streaming,
@@ -1182,7 +1182,6 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
 	spin_lock_irqsave(&cam->dev_lock, flags);
 	INIT_LIST_HEAD(&cam->buffers);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
-	return 0;
 }
 
 
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 4e4d163..deba425 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -435,10 +435,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 }
 
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct camif_vp *vp = vb2_get_drv_priv(vq);
-	return camif_stop_capture(vp);
+	camif_stop_capture(vp);
 }
 
 static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 8a18972..368b3f6 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1670,13 +1670,11 @@ static int s5p_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret > 0 ? 0 : ret;
 }
 
-static int s5p_jpeg_stop_streaming(struct vb2_queue *q)
+static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
 {
 	struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
 
 	pm_runtime_put(ctx->jpeg->dev);
-
-	return 0;
 }
 
 static struct vb2_ops s5p_jpeg_qops = {
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 8faf969..58b7bba 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -1027,7 +1027,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 }
 
-static int s5p_mfc_stop_streaming(struct vb2_queue *q)
+static void s5p_mfc_stop_streaming(struct vb2_queue *q)
 {
 	unsigned long flags;
 	struct s5p_mfc_ctx *ctx = fh_to_ctx(q->drv_priv);
@@ -1071,7 +1071,6 @@ static int s5p_mfc_stop_streaming(struct vb2_queue *q)
 	}
 	if (aborted)
 		ctx->state = MFCINST_RUNNING;
-	return 0;
 }
 
 
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index df83cd1..458279e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1954,7 +1954,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 }
 
-static int s5p_mfc_stop_streaming(struct vb2_queue *q)
+static void s5p_mfc_stop_streaming(struct vb2_queue *q)
 {
 	unsigned long flags;
 	struct s5p_mfc_ctx *ctx = fh_to_ctx(q->drv_priv);
@@ -1983,7 +1983,6 @@ static int s5p_mfc_stop_streaming(struct vb2_queue *q)
 		ctx->src_queue_cnt = 0;
 	}
 	spin_unlock_irqrestore(&dev->irqlock, flags);
-	return 0;
 }
 
 static void s5p_mfc_buf_queue(struct vb2_buffer *vb)
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index a1ce55f..9f1e52f 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -985,7 +985,7 @@ static void mxr_watchdog(unsigned long arg)
 	spin_unlock_irqrestore(&layer->enq_slock, flags);
 }
 
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct mxr_layer *layer = vb2_get_drv_priv(vq);
 	struct mxr_device *mdev = layer->mdev;
@@ -1031,7 +1031,6 @@ static int stop_streaming(struct vb2_queue *vq)
 	mxr_streamer_put(mdev);
 	/* allow changes in output configuration */
 	mxr_output_put(mdev);
-	return 0;
 }
 
 static struct vb2_ops mxr_video_qops = {
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index f0b6c90..38c723a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -406,7 +406,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 }
 
 /* abort streaming and wait for last buffer */
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
@@ -433,7 +433,7 @@ static int stop_streaming(struct vb2_queue *vq)
 	if (time_after(jiffies, timeout)) {
 		dev_err(icd->parent,
 			"Timeout waiting for finishing codec request\n");
-		return -ETIMEDOUT;
+		return;
 	}
 
 	/* Disable interrupts */
@@ -444,8 +444,6 @@ static int stop_streaming(struct vb2_queue *vq)
 	ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
 	if (ret < 0)
 		dev_err(icd->parent, "Disable ISI timed out\n");
-
-	return ret;
 }
 
 static struct vb2_ops isi_video_qops = {
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 3e84480..b40bc2e 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -741,7 +741,7 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 }
 
-static int mx2_stop_streaming(struct vb2_queue *q)
+static void mx2_stop_streaming(struct vb2_queue *q)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
 	struct soc_camera_host *ici =
@@ -773,8 +773,6 @@ static int mx2_stop_streaming(struct vb2_queue *q)
 
 	dma_free_coherent(ici->v4l2_dev.dev,
 			  pcdev->discard_size, b, pcdev->discard_buffer_dma);
-
-	return 0;
 }
 
 static struct vb2_ops mx2_videobuf_ops = {
diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 9ed81ac..2bbf608 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -406,7 +406,7 @@ static int mx3_videobuf_init(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int mx3_stop_streaming(struct vb2_queue *q)
+static void mx3_stop_streaming(struct vb2_queue *q)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
@@ -605,8 +605,6 @@ static int mx3_camera_try_bus_param(struct soc_camera_device *icd,
 	} else if (ret != -ENOIOCTLCMD) {
 		return ret;
 	}
-
-	return 0;
 }
 
 static bool chan_filter(struct dma_chan *chan, void *arg)
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 0ff5cfa..e6287fb 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -505,7 +505,7 @@ static int rcar_vin_videobuf_init(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int rcar_vin_stop_streaming(struct vb2_queue *vq)
+static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
@@ -516,8 +516,6 @@ static int rcar_vin_stop_streaming(struct vb2_queue *vq)
 	list_for_each_safe(buf_head, tmp, &priv->capture)
 		list_del_init(buf_head);
 	spin_unlock_irq(&priv->lock);
-
-	return 0;
 }
 
 static struct vb2_ops rcar_vin_vb2_ops = {
diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index 3e75a46..20ad4a5 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -471,7 +471,7 @@ static int sh_mobile_ceu_videobuf_init(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
+static void sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
 {
 	struct soc_camera_device *icd = container_of(q, struct soc_camera_device, vb2_vidq);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
@@ -487,7 +487,7 @@ static int sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
 
 	spin_unlock_irq(&pcdev->lock);
 
-	return sh_mobile_ceu_soft_reset(pcdev);
+	sh_mobile_ceu_soft_reset(pcdev);
 }
 
 static struct vb2_ops sh_mobile_ceu_videobuf_ops = {
diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 3890f4f..d00bf3d 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -906,12 +906,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 }
 
 /* abort streaming and wait for last buffer */
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct vivi_dev *dev = vb2_get_drv_priv(vq);
 	dprintk(dev, 1, "%s\n", __func__);
 	vivi_stop_generating(dev);
-	return 0;
 }
 
 static void vivi_lock(struct vb2_queue *vq)
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index e41f07d..767d5c5 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -720,7 +720,7 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 }
 
-static int vsp1_video_stop_streaming(struct vb2_queue *vq)
+static void vsp1_video_stop_streaming(struct vb2_queue *vq)
 {
 	struct vsp1_video *video = vb2_get_drv_priv(vq);
 	struct vsp1_pipeline *pipe = to_vsp1_pipeline(&video->video.entity);
@@ -743,8 +743,6 @@ static int vsp1_video_stop_streaming(struct vb2_queue *vq)
 	spin_lock_irqsave(&video->irqlock, flags);
 	INIT_LIST_HEAD(&video->irqqueue);
 	spin_unlock_irqrestore(&video->irqlock, flags);
-
-	return 0;
 }
 
 static struct vb2_ops vsp1_video_queue_qops = {
diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h
index bce4386..432862c 100644
--- a/drivers/media/usb/em28xx/em28xx-v4l.h
+++ b/drivers/media/usb/em28xx/em28xx-v4l.h
@@ -16,5 +16,5 @@
 
 
 int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count);
-int em28xx_stop_vbi_streaming(struct vb2_queue *vq);
+void em28xx_stop_vbi_streaming(struct vb2_queue *vq);
 extern struct vb2_ops em28xx_vbi_qops;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 0856e5d..cdcd751 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -937,7 +937,7 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
 	return rc;
 }
 
-static int em28xx_stop_streaming(struct vb2_queue *vq)
+static void em28xx_stop_streaming(struct vb2_queue *vq)
 {
 	struct em28xx *dev = vb2_get_drv_priv(vq);
 	struct em28xx_dmaqueue *vidq = &dev->vidq;
@@ -961,11 +961,9 @@ static int em28xx_stop_streaming(struct vb2_queue *vq)
 	}
 	dev->usb_ctl.vid_buf = NULL;
 	spin_unlock_irqrestore(&dev->slock, flags);
-
-	return 0;
 }
 
-int em28xx_stop_vbi_streaming(struct vb2_queue *vq)
+void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
 {
 	struct em28xx *dev = vb2_get_drv_priv(vq);
 	struct em28xx_dmaqueue *vbiq = &dev->vbiq;
@@ -989,8 +987,6 @@ int em28xx_stop_vbi_streaming(struct vb2_queue *vq)
 	}
 	dev->usb_ctl.vbi_buf = NULL;
 	spin_unlock_irqrestore(&dev->slock, flags);
-
-	return 0;
 }
 
 static void
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 84a6720..a73b0bc 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -681,12 +681,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	return r;
 }
 
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct pwc_device *pdev = vb2_get_drv_priv(vq);
 
-	if (mutex_lock_interruptible(&pdev->v4l2_lock))
-		return -ERESTARTSYS;
+	mutex_lock(&pdev->v4l2_lock);
 	if (pdev->udev) {
 		pwc_set_leds(pdev, 0, 0);
 		pwc_camera_power(pdev, 0);
@@ -695,8 +694,6 @@ static int stop_streaming(struct vb2_queue *vq)
 
 	pwc_cleanup_queued_bufs(pdev);
 	mutex_unlock(&pdev->v4l2_lock);
-
-	return 0;
 }
 
 static struct vb2_ops pwc_vb_queue_ops = {
diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
index 4c7513a..60e4929 100644
--- a/drivers/media/usb/s2255/s2255drv.c
+++ b/drivers/media/usb/s2255/s2255drv.c
@@ -714,7 +714,7 @@ static void buffer_queue(struct vb2_buffer *vb)
 }
 
 static int start_streaming(struct vb2_queue *vq, unsigned int count);
-static int stop_streaming(struct vb2_queue *vq);
+static void stop_streaming(struct vb2_queue *vq);
 
 static struct vb2_ops s2255_video_qops = {
 	.queue_setup = queue_setup,
@@ -1109,7 +1109,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 }
 
 /* abort streaming and wait for last buffer */
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct s2255_vc *vc = vb2_get_drv_priv(vq);
 	struct s2255_buffer *buf, *node;
@@ -1123,7 +1123,6 @@ static int stop_streaming(struct vb2_queue *vq)
 			buf, buf->vb.v4l2_buf.index);
 	}
 	spin_unlock_irqrestore(&vc->qlock, flags);
-	return 0;
 }
 
 static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id i)
diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
index 37bc00f..46e8a50 100644
--- a/drivers/media/usb/stk1160/stk1160-v4l.c
+++ b/drivers/media/usb/stk1160/stk1160-v4l.c
@@ -583,10 +583,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 }
 
 /* abort streaming and wait for last buffer */
-static int stop_streaming(struct vb2_queue *vq)
+static void stop_streaming(struct vb2_queue *vq)
 {
 	struct stk1160 *dev = vb2_get_drv_priv(vq);
-	return stk1160_stop_streaming(dev);
+	stk1160_stop_streaming(dev);
 }
 
 static struct vb2_ops stk1160_video_qops = {
diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 01ed1ec8..d2f24ad 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -634,15 +634,12 @@ static int usbtv_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return usbtv_start(usbtv);
 }
 
-static int usbtv_stop_streaming(struct vb2_queue *vq)
+static void usbtv_stop_streaming(struct vb2_queue *vq)
 {
 	struct usbtv *usbtv = vb2_get_drv_priv(vq);
 
-	if (usbtv->udev == NULL)
-		return -ENODEV;
-
-	usbtv_stop(usbtv);
-	return 0;
+	if (usbtv->udev)
+		usbtv_stop(usbtv);
 }
 
 struct vb2_ops usbtv_vb2_ops = {
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index 8c101cb..baeceee 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -1242,7 +1242,7 @@ static int vpfe_buffer_init(struct vb2_buffer *vb)
 }
 
 /* abort streaming and wait for last buffer */
-static int vpfe_stop_streaming(struct vb2_queue *vq)
+static void vpfe_stop_streaming(struct vb2_queue *vq)
 {
 	struct vpfe_fh *fh = vb2_get_drv_priv(vq);
 	struct vpfe_video_device *video = fh->video;
@@ -1256,7 +1256,6 @@ static int vpfe_stop_streaming(struct vb2_queue *vq)
 		list_del(&video->next_frm->list);
 		vb2_buffer_done(&video->next_frm->vb, VB2_BUF_STATE_ERROR);
 	}
-	return 0;
 }
 
 static void vpfe_buf_cleanup(struct vb2_buffer *vb)
diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
index e235787..3b97a05 100644
--- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
+++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
@@ -263,7 +263,7 @@ dt3155_buf_prepare(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int
+static void
 dt3155_stop_streaming(struct vb2_queue *q)
 {
 	struct dt3155_priv *pd = vb2_get_drv_priv(q);
@@ -277,7 +277,6 @@ dt3155_stop_streaming(struct vb2_queue *q)
 	}
 	spin_unlock_irq(&pd->lock);
 	msleep(45); /* irq hendler will stop the hardware */
-	return 0;
 }
 
 static void
diff --git a/drivers/staging/media/go7007/go7007-v4l2.c b/drivers/staging/media/go7007/go7007-v4l2.c
index a349878..818acdde 100644
--- a/drivers/staging/media/go7007/go7007-v4l2.c
+++ b/drivers/staging/media/go7007/go7007-v4l2.c
@@ -515,7 +515,7 @@ static int go7007_start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret;
 }
 
-static int go7007_stop_streaming(struct vb2_queue *q)
+static void go7007_stop_streaming(struct vb2_queue *q)
 {
 	struct go7007 *go = vb2_get_drv_priv(q);
 	unsigned long flags;
@@ -537,7 +537,6 @@ static int go7007_stop_streaming(struct vb2_queue *q)
 	/* Turn on Capture LED */
 	if (go->board_id == GO7007_BOARDID_ADS_USBAV_709)
 		go7007_write_addr(go, 0x3c82, 0x000d);
-	return 0;
 }
 
 static struct vb2_ops go7007_video_qops = {
diff --git a/drivers/staging/media/msi3101/sdr-msi3101.c b/drivers/staging/media/msi3101/sdr-msi3101.c
index 04ff29e..2136e60 100644
--- a/drivers/staging/media/msi3101/sdr-msi3101.c
+++ b/drivers/staging/media/msi3101/sdr-msi3101.c
@@ -1577,13 +1577,12 @@ static int msi3101_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return ret;
 }
 
-static int msi3101_stop_streaming(struct vb2_queue *vq)
+static void msi3101_stop_streaming(struct vb2_queue *vq)
 {
 	struct msi3101_state *s = vb2_get_drv_priv(vq);
 	dev_dbg(&s->udev->dev, "%s:\n", __func__);
 
-	if (mutex_lock_interruptible(&s->v4l2_lock))
-		return -ERESTARTSYS;
+	mutex_lock(&s->v4l2_lock);
 
 	if (s->udev)
 		msi3101_isoc_cleanup(s);
@@ -1595,8 +1594,6 @@ static int msi3101_stop_streaming(struct vb2_queue *vq)
 	msi3101_ctrl_msg(s, CMD_STOP_STREAMING, 0);
 
 	mutex_unlock(&s->v4l2_lock);
-
-	return 0;
 }
 
 static struct vb2_ops msi3101_vb2_ops = {
diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index edcabcd..e056476 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -741,14 +741,13 @@ static int solo_enc_start_streaming(struct vb2_queue *q, unsigned int count)
 	return solo_ring_start(solo_enc->solo_dev);
 }
 
-static int solo_enc_stop_streaming(struct vb2_queue *q)
+static void solo_enc_stop_streaming(struct vb2_queue *q)
 {
 	struct solo_enc_dev *solo_enc = vb2_get_drv_priv(q);
 
 	solo_enc_off(solo_enc);
 	INIT_LIST_HEAD(&solo_enc->vidq_active);
 	solo_ring_stop(solo_enc->solo_dev);
-	return 0;
 }
 
 static struct vb2_ops solo_enc_video_qops = {
diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2.c b/drivers/staging/media/solo6x10/solo6x10-v4l2.c
index 1815f76..5d0100e 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2.c
@@ -336,13 +336,12 @@ static int solo_start_streaming(struct vb2_queue *q, unsigned int count)
 	return solo_start_thread(solo_dev);
 }
 
-static int solo_stop_streaming(struct vb2_queue *q)
+static void solo_stop_streaming(struct vb2_queue *q)
 {
 	struct solo_dev *solo_dev = vb2_get_drv_priv(q);
 
 	solo_stop_thread(solo_dev);
 	INIT_LIST_HEAD(&solo_dev->vidq_active);
-	return 0;
 }
 
 static void solo_buf_queue(struct vb2_buffer *vb)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index af46211..3b57851 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -323,7 +323,7 @@ struct vb2_ops {
 	void (*buf_cleanup)(struct vb2_buffer *vb);
 
 	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
-	int (*stop_streaming)(struct vb2_queue *q);
+	void (*stop_streaming)(struct vb2_queue *q);
 
 	void (*buf_queue)(struct vb2_buffer *vb);
 };
-- 
1.9.0

Re: [REVIEW PATCH 01/11] vb2: stop_streaming should return void

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The vb2 core ignores any return code from the stop_streaming op.
> And there really isn't anything it can do anyway in case of an error.
> So change the return type to void and update any drivers that implement it.
>
> The int return gave drivers the idea that this operation could actually
> fail, but that's really not the case.
>
> The pwc amd sdr-msi3101 drivers both had this construction:
>
>         if (mutex_lock_interruptible(&s->v4l2_lock))
>                 return -ERESTARTSYS;
>
> This has been updated to just call mutex_lock(). The stop_streaming op
> expects this to really stop streaming and I very much doubt this will
> work reliably if stop_streaming just returns without really stopping the
> DMA.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/pci/sta2x11/sta2x11_vip.c                  | 3 +--
>  drivers/media/platform/blackfin/bfin_capture.c           | 6 +-----
>  drivers/media/platform/coda.c                            | 4 +---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c              | 4 +---
>  drivers/media/platform/exynos4-is/fimc-capture.c         | 6 +++---
>  drivers/media/platform/exynos4-is/fimc-lite.c            | 6 +++---
>  drivers/media/platform/exynos4-is/fimc-m2m.c             | 3 +--
>  drivers/media/platform/marvell-ccic/mcam-core.c          | 7 +++----
>  drivers/media/platform/s3c-camif/camif-capture.c         | 4 ++--
>  drivers/media/platform/s5p-jpeg/jpeg-core.c              | 4 +---
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c             | 3 +--
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c             | 3 +--
>  drivers/media/platform/s5p-tv/mixer_video.c              | 3 +--
>  drivers/media/platform/soc_camera/atmel-isi.c            | 6 ++----
>  drivers/media/platform/soc_camera/mx2_camera.c           | 4 +---
>  drivers/media/platform/soc_camera/mx3_camera.c           | 4 +---
>  drivers/media/platform/soc_camera/rcar_vin.c             | 4 +---
>  drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c | 4 ++--
>  drivers/media/platform/vivi.c                            | 3 +--
>  drivers/media/platform/vsp1/vsp1_video.c                 | 4 +---
>  drivers/media/usb/em28xx/em28xx-v4l.h                    | 2 +-
>  drivers/media/usb/em28xx/em28xx-video.c                  | 8 ++------
>  drivers/media/usb/pwc/pwc-if.c                           | 7 ++-----
>  drivers/media/usb/s2255/s2255drv.c                       | 5 ++---
>  drivers/media/usb/stk1160/stk1160-v4l.c                  | 4 ++--
>  drivers/media/usb/usbtv/usbtv-video.c                    | 9 +++------
>  drivers/staging/media/davinci_vpfe/vpfe_video.c          | 3 +--
>  drivers/staging/media/dt3155v4l/dt3155v4l.c              | 3 +--
>  drivers/staging/media/go7007/go7007-v4l2.c               | 3 +--
>  drivers/staging/media/msi3101/sdr-msi3101.c              | 7 ++-----
>  drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c       | 3 +--
>  drivers/staging/media/solo6x10/solo6x10-v4l2.c           | 3 +--
>  include/media/videobuf2-core.h                           | 2 +-
>  33 files changed, 49 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
> index bb11443..7559951 100644
> --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> @@ -357,7 +357,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct sta2x11_vip *vip = vb2_get_drv_priv(vq);
>         struct vip_buffer *vip_buf, *node;
> @@ -374,7 +374,6 @@ static int stop_streaming(struct vb2_queue *vq)
>                 list_del(&vip_buf->list);
>         }
>         spin_unlock(&vip->lock);
> -       return 0;
>  }
>
>  static struct vb2_ops vip_video_qops = {
> diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
> index 200bec9..16f643c 100644
> --- a/drivers/media/platform/blackfin/bfin_capture.c
> +++ b/drivers/media/platform/blackfin/bfin_capture.c
> @@ -427,15 +427,12 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return 0;
>  }
>
> -static int bcap_stop_streaming(struct vb2_queue *vq)
> +static void bcap_stop_streaming(struct vb2_queue *vq)
>  {
>         struct bcap_device *bcap_dev = vb2_get_drv_priv(vq);
>         struct ppi_if *ppi = bcap_dev->ppi;
>         int ret;
>
> -       if (!vb2_is_streaming(vq))
> -               return 0;
> -
>         bcap_dev->stop = true;
>         wait_for_completion(&bcap_dev->comp);
>         ppi->ops->stop(ppi);
> @@ -452,7 +449,6 @@ static int bcap_stop_streaming(struct vb2_queue *vq)
>                 list_del(&bcap_dev->cur_frm->list);
>                 vb2_buffer_done(&bcap_dev->cur_frm->vb, VB2_BUF_STATE_ERROR);
>         }
> -       return 0;
>  }
>
>  static struct vb2_ops bcap_video_qops = {
> diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
> index 3e5199e..d9b1a04 100644
> --- a/drivers/media/platform/coda.c
> +++ b/drivers/media/platform/coda.c
> @@ -2269,7 +2269,7 @@ out:
>         return ret;
>  }
>
> -static int coda_stop_streaming(struct vb2_queue *q)
> +static void coda_stop_streaming(struct vb2_queue *q)
>  {
>         struct coda_ctx *ctx = vb2_get_drv_priv(q);
>         struct coda_dev *dev = ctx->dev;
> @@ -2295,8 +2295,6 @@ static int coda_stop_streaming(struct vb2_queue *q)
>                         ctx->bitstream.vaddr, ctx->bitstream.size);
>                 ctx->runcounter = 0;
>         }
> -
> -       return 0;
>  }
>
>  static struct vb2_ops coda_qops = {
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index d0ea94f..e434f1f0 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -66,15 +66,13 @@ static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
>         return ret > 0 ? 0 : ret;
>  }
>
> -static int gsc_m2m_stop_streaming(struct vb2_queue *q)
> +static void gsc_m2m_stop_streaming(struct vb2_queue *q)
>  {
>         struct gsc_ctx *ctx = q->drv_priv;
>
>         __gsc_m2m_job_abort(ctx);
>
>         pm_runtime_put(&ctx->gsc_dev->pdev->dev);
> -
> -       return 0;
>  }
>
>  void gsc_m2m_job_finish(struct gsc_ctx *ctx, int vb_state)
> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
> index 92ae812..3d2babd 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -294,15 +294,15 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>  }
>
> -static int stop_streaming(struct vb2_queue *q)
> +static void stop_streaming(struct vb2_queue *q)
>  {
>         struct fimc_ctx *ctx = q->drv_priv;
>         struct fimc_dev *fimc = ctx->fimc_dev;
>
>         if (!fimc_capture_active(fimc))
> -               return -EINVAL;
> +               return;
>
> -       return fimc_stop_capture(fimc, false);
> +       fimc_stop_capture(fimc, false);
>  }
>
>  int fimc_capture_suspend(struct fimc_dev *fimc)
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> index 3ad660b..630aef5 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -350,14 +350,14 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>  }
>
> -static int stop_streaming(struct vb2_queue *q)
> +static void stop_streaming(struct vb2_queue *q)
>  {
>         struct fimc_lite *fimc = q->drv_priv;
>
>         if (!fimc_lite_active(fimc))
> -               return -EINVAL;
> +               return;
>
> -       return fimc_lite_stop_capture(fimc, false);
> +       fimc_lite_stop_capture(fimc, false);
>  }
>
>  static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
> diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
> index 36971d9..d314155 100644
> --- a/drivers/media/platform/exynos4-is/fimc-m2m.c
> +++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
> @@ -85,7 +85,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
>         return ret > 0 ? 0 : ret;
>  }
>
> -static int stop_streaming(struct vb2_queue *q)
> +static void stop_streaming(struct vb2_queue *q)
>  {
>         struct fimc_ctx *ctx = q->drv_priv;
>         int ret;
> @@ -95,7 +95,6 @@ static int stop_streaming(struct vb2_queue *q)
>                 fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
>
>         pm_runtime_put(&ctx->fimc_dev->pdev->dev);
> -       return 0;
>  }
>
>  static void fimc_device_run(void *priv)
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
> index 8b34c48..be4b512 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -1156,7 +1156,7 @@ static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return mcam_read_setup(cam);
>  }
>
> -static int mcam_vb_stop_streaming(struct vb2_queue *vq)
> +static void mcam_vb_stop_streaming(struct vb2_queue *vq)
>  {
>         struct mcam_camera *cam = vb2_get_drv_priv(vq);
>         unsigned long flags;
> @@ -1164,10 +1164,10 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
>         if (cam->state == S_BUFWAIT) {
>                 /* They never gave us buffers */
>                 cam->state = S_IDLE;
> -               return 0;
> +               return;
>         }
>         if (cam->state != S_STREAMING)
> -               return -EINVAL;
> +               return;
>         mcam_ctlr_stop_dma(cam);
>         /*
>          * Reset the CCIC PHY after stopping streaming,
> @@ -1182,7 +1182,6 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
>         spin_lock_irqsave(&cam->dev_lock, flags);
>         INIT_LIST_HEAD(&cam->buffers);
>         spin_unlock_irqrestore(&cam->dev_lock, flags);
> -       return 0;
>  }
>
>
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
> index 4e4d163..deba425 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -435,10 +435,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>         return 0;
>  }
>
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct camif_vp *vp = vb2_get_drv_priv(vq);
> -       return camif_stop_capture(vp);
> +       camif_stop_capture(vp);
>  }
>
>  static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 8a18972..368b3f6 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1670,13 +1670,11 @@ static int s5p_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
>         return ret > 0 ? 0 : ret;
>  }
>
> -static int s5p_jpeg_stop_streaming(struct vb2_queue *q)
> +static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
>  {
>         struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
>
>         pm_runtime_put(ctx->jpeg->dev);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops s5p_jpeg_qops = {
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 8faf969..58b7bba 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -1027,7 +1027,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>  }
>
> -static int s5p_mfc_stop_streaming(struct vb2_queue *q)
> +static void s5p_mfc_stop_streaming(struct vb2_queue *q)
>  {
>         unsigned long flags;
>         struct s5p_mfc_ctx *ctx = fh_to_ctx(q->drv_priv);
> @@ -1071,7 +1071,6 @@ static int s5p_mfc_stop_streaming(struct vb2_queue *q)
>         }
>         if (aborted)
>                 ctx->state = MFCINST_RUNNING;
> -       return 0;
>  }
>
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index df83cd1..458279e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1954,7 +1954,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>  }
>
> -static int s5p_mfc_stop_streaming(struct vb2_queue *q)
> +static void s5p_mfc_stop_streaming(struct vb2_queue *q)
>  {
>         unsigned long flags;
>         struct s5p_mfc_ctx *ctx = fh_to_ctx(q->drv_priv);
> @@ -1983,7 +1983,6 @@ static int s5p_mfc_stop_streaming(struct vb2_queue *q)
>                 ctx->src_queue_cnt = 0;
>         }
>         spin_unlock_irqrestore(&dev->irqlock, flags);
> -       return 0;
>  }
>
>  static void s5p_mfc_buf_queue(struct vb2_buffer *vb)
> diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
> index a1ce55f..9f1e52f 100644
> --- a/drivers/media/platform/s5p-tv/mixer_video.c
> +++ b/drivers/media/platform/s5p-tv/mixer_video.c
> @@ -985,7 +985,7 @@ static void mxr_watchdog(unsigned long arg)
>         spin_unlock_irqrestore(&layer->enq_slock, flags);
>  }
>
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct mxr_layer *layer = vb2_get_drv_priv(vq);
>         struct mxr_device *mdev = layer->mdev;
> @@ -1031,7 +1031,6 @@ static int stop_streaming(struct vb2_queue *vq)
>         mxr_streamer_put(mdev);
>         /* allow changes in output configuration */
>         mxr_output_put(mdev);
> -       return 0;
>  }
>
>  static struct vb2_ops mxr_video_qops = {
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index f0b6c90..38c723a 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -406,7 +406,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>         struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> @@ -433,7 +433,7 @@ static int stop_streaming(struct vb2_queue *vq)
>         if (time_after(jiffies, timeout)) {
>                 dev_err(icd->parent,
>                         "Timeout waiting for finishing codec request\n");
> -               return -ETIMEDOUT;
> +               return;
>         }
>
>         /* Disable interrupts */
> @@ -444,8 +444,6 @@ static int stop_streaming(struct vb2_queue *vq)
>         ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>         if (ret < 0)
>                 dev_err(icd->parent, "Disable ISI timed out\n");
> -
> -       return ret;
>  }
>
>  static struct vb2_ops isi_video_qops = {
> diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
> index 3e84480..b40bc2e 100644
> --- a/drivers/media/platform/soc_camera/mx2_camera.c
> +++ b/drivers/media/platform/soc_camera/mx2_camera.c
> @@ -741,7 +741,7 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>  }
>
> -static int mx2_stop_streaming(struct vb2_queue *q)
> +static void mx2_stop_streaming(struct vb2_queue *q)
>  {
>         struct soc_camera_device *icd = soc_camera_from_vb2q(q);
>         struct soc_camera_host *ici =
> @@ -773,8 +773,6 @@ static int mx2_stop_streaming(struct vb2_queue *q)
>
>         dma_free_coherent(ici->v4l2_dev.dev,
>                           pcdev->discard_size, b, pcdev->discard_buffer_dma);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops mx2_videobuf_ops = {
> diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
> index 9ed81ac..2bbf608 100644
> --- a/drivers/media/platform/soc_camera/mx3_camera.c
> +++ b/drivers/media/platform/soc_camera/mx3_camera.c
> @@ -406,7 +406,7 @@ static int mx3_videobuf_init(struct vb2_buffer *vb)
>         return 0;
>  }
>
> -static int mx3_stop_streaming(struct vb2_queue *q)
> +static void mx3_stop_streaming(struct vb2_queue *q)
>  {
>         struct soc_camera_device *icd = soc_camera_from_vb2q(q);
>         struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> @@ -605,8 +605,6 @@ static int mx3_camera_try_bus_param(struct soc_camera_device *icd,
>         } else if (ret != -ENOIOCTLCMD) {
>                 return ret;
>         }
> -
> -       return 0;
>  }
>
>  static bool chan_filter(struct dma_chan *chan, void *arg)
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 0ff5cfa..e6287fb 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -505,7 +505,7 @@ static int rcar_vin_videobuf_init(struct vb2_buffer *vb)
>         return 0;
>  }
>
> -static int rcar_vin_stop_streaming(struct vb2_queue *vq)
> +static void rcar_vin_stop_streaming(struct vb2_queue *vq)
>  {
>         struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>         struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> @@ -516,8 +516,6 @@ static int rcar_vin_stop_streaming(struct vb2_queue *vq)
>         list_for_each_safe(buf_head, tmp, &priv->capture)
>                 list_del_init(buf_head);
>         spin_unlock_irq(&priv->lock);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops rcar_vin_vb2_ops = {
> diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
> index 3e75a46..20ad4a5 100644
> --- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
> +++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
> @@ -471,7 +471,7 @@ static int sh_mobile_ceu_videobuf_init(struct vb2_buffer *vb)
>         return 0;
>  }
>
> -static int sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
> +static void sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
>  {
>         struct soc_camera_device *icd = container_of(q, struct soc_camera_device, vb2_vidq);
>         struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> @@ -487,7 +487,7 @@ static int sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
>
>         spin_unlock_irq(&pcdev->lock);
>
> -       return sh_mobile_ceu_soft_reset(pcdev);
> +       sh_mobile_ceu_soft_reset(pcdev);
>  }
>
>  static struct vb2_ops sh_mobile_ceu_videobuf_ops = {
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 3890f4f..d00bf3d 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -906,12 +906,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct vivi_dev *dev = vb2_get_drv_priv(vq);
>         dprintk(dev, 1, "%s\n", __func__);
>         vivi_stop_generating(dev);
> -       return 0;
>  }
>
>  static void vivi_lock(struct vb2_queue *vq)
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index e41f07d..767d5c5 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -720,7 +720,7 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return 0;
>  }
>
> -static int vsp1_video_stop_streaming(struct vb2_queue *vq)
> +static void vsp1_video_stop_streaming(struct vb2_queue *vq)
>  {
>         struct vsp1_video *video = vb2_get_drv_priv(vq);
>         struct vsp1_pipeline *pipe = to_vsp1_pipeline(&video->video.entity);
> @@ -743,8 +743,6 @@ static int vsp1_video_stop_streaming(struct vb2_queue *vq)
>         spin_lock_irqsave(&video->irqlock, flags);
>         INIT_LIST_HEAD(&video->irqqueue);
>         spin_unlock_irqrestore(&video->irqlock, flags);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops vsp1_video_queue_qops = {
> diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h
> index bce4386..432862c 100644
> --- a/drivers/media/usb/em28xx/em28xx-v4l.h
> +++ b/drivers/media/usb/em28xx/em28xx-v4l.h
> @@ -16,5 +16,5 @@
>
>
>  int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count);
> -int em28xx_stop_vbi_streaming(struct vb2_queue *vq);
> +void em28xx_stop_vbi_streaming(struct vb2_queue *vq);
>  extern struct vb2_ops em28xx_vbi_qops;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 0856e5d..cdcd751 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -937,7 +937,7 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
>         return rc;
>  }
>
> -static int em28xx_stop_streaming(struct vb2_queue *vq)
> +static void em28xx_stop_streaming(struct vb2_queue *vq)
>  {
>         struct em28xx *dev = vb2_get_drv_priv(vq);
>         struct em28xx_dmaqueue *vidq = &dev->vidq;
> @@ -961,11 +961,9 @@ static int em28xx_stop_streaming(struct vb2_queue *vq)
>         }
>         dev->usb_ctl.vid_buf = NULL;
>         spin_unlock_irqrestore(&dev->slock, flags);
> -
> -       return 0;
>  }
>
> -int em28xx_stop_vbi_streaming(struct vb2_queue *vq)
> +void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>  {
>         struct em28xx *dev = vb2_get_drv_priv(vq);
>         struct em28xx_dmaqueue *vbiq = &dev->vbiq;
> @@ -989,8 +987,6 @@ int em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>         }
>         dev->usb_ctl.vbi_buf = NULL;
>         spin_unlock_irqrestore(&dev->slock, flags);
> -
> -       return 0;
>  }
>
>  static void
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 84a6720..a73b0bc 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -681,12 +681,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>         return r;
>  }
>
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct pwc_device *pdev = vb2_get_drv_priv(vq);
>
> -       if (mutex_lock_interruptible(&pdev->v4l2_lock))
> -               return -ERESTARTSYS;
> +       mutex_lock(&pdev->v4l2_lock);
>         if (pdev->udev) {
>                 pwc_set_leds(pdev, 0, 0);
>                 pwc_camera_power(pdev, 0);
> @@ -695,8 +694,6 @@ static int stop_streaming(struct vb2_queue *vq)
>
>         pwc_cleanup_queued_bufs(pdev);
>         mutex_unlock(&pdev->v4l2_lock);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops pwc_vb_queue_ops = {
> diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
> index 4c7513a..60e4929 100644
> --- a/drivers/media/usb/s2255/s2255drv.c
> +++ b/drivers/media/usb/s2255/s2255drv.c
> @@ -714,7 +714,7 @@ static void buffer_queue(struct vb2_buffer *vb)
>  }
>
>  static int start_streaming(struct vb2_queue *vq, unsigned int count);
> -static int stop_streaming(struct vb2_queue *vq);
> +static void stop_streaming(struct vb2_queue *vq);
>
>  static struct vb2_ops s2255_video_qops = {
>         .queue_setup = queue_setup,
> @@ -1109,7 +1109,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct s2255_vc *vc = vb2_get_drv_priv(vq);
>         struct s2255_buffer *buf, *node;
> @@ -1123,7 +1123,6 @@ static int stop_streaming(struct vb2_queue *vq)
>                         buf, buf->vb.v4l2_buf.index);
>         }
>         spin_unlock_irqrestore(&vc->qlock, flags);
> -       return 0;
>  }
>
>  static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id i)
> diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
> index 37bc00f..46e8a50 100644
> --- a/drivers/media/usb/stk1160/stk1160-v4l.c
> +++ b/drivers/media/usb/stk1160/stk1160-v4l.c
> @@ -583,10 +583,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int stop_streaming(struct vb2_queue *vq)
> +static void stop_streaming(struct vb2_queue *vq)
>  {
>         struct stk1160 *dev = vb2_get_drv_priv(vq);
> -       return stk1160_stop_streaming(dev);
> +       stk1160_stop_streaming(dev);
>  }
>
>  static struct vb2_ops stk1160_video_qops = {
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 01ed1ec8..d2f24ad 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -634,15 +634,12 @@ static int usbtv_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return usbtv_start(usbtv);
>  }
>
> -static int usbtv_stop_streaming(struct vb2_queue *vq)
> +static void usbtv_stop_streaming(struct vb2_queue *vq)
>  {
>         struct usbtv *usbtv = vb2_get_drv_priv(vq);
>
> -       if (usbtv->udev == NULL)
> -               return -ENODEV;
> -
> -       usbtv_stop(usbtv);
> -       return 0;
> +       if (usbtv->udev)
> +               usbtv_stop(usbtv);
>  }
>
>  struct vb2_ops usbtv_vb2_ops = {
> diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
> index 8c101cb..baeceee 100644
> --- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
> +++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
> @@ -1242,7 +1242,7 @@ static int vpfe_buffer_init(struct vb2_buffer *vb)
>  }
>
>  /* abort streaming and wait for last buffer */
> -static int vpfe_stop_streaming(struct vb2_queue *vq)
> +static void vpfe_stop_streaming(struct vb2_queue *vq)
>  {
>         struct vpfe_fh *fh = vb2_get_drv_priv(vq);
>         struct vpfe_video_device *video = fh->video;
> @@ -1256,7 +1256,6 @@ static int vpfe_stop_streaming(struct vb2_queue *vq)
>                 list_del(&video->next_frm->list);
>                 vb2_buffer_done(&video->next_frm->vb, VB2_BUF_STATE_ERROR);
>         }
> -       return 0;
>  }
>
>  static void vpfe_buf_cleanup(struct vb2_buffer *vb)
> diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> index e235787..3b97a05 100644
> --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
> +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> @@ -263,7 +263,7 @@ dt3155_buf_prepare(struct vb2_buffer *vb)
>         return 0;
>  }
>
> -static int
> +static void
>  dt3155_stop_streaming(struct vb2_queue *q)
>  {
>         struct dt3155_priv *pd = vb2_get_drv_priv(q);
> @@ -277,7 +277,6 @@ dt3155_stop_streaming(struct vb2_queue *q)
>         }
>         spin_unlock_irq(&pd->lock);
>         msleep(45); /* irq hendler will stop the hardware */
> -       return 0;
>  }
>
>  static void
> diff --git a/drivers/staging/media/go7007/go7007-v4l2.c b/drivers/staging/media/go7007/go7007-v4l2.c
> index a349878..818acdde 100644
> --- a/drivers/staging/media/go7007/go7007-v4l2.c
> +++ b/drivers/staging/media/go7007/go7007-v4l2.c
> @@ -515,7 +515,7 @@ static int go7007_start_streaming(struct vb2_queue *q, unsigned int count)
>         return ret;
>  }
>
> -static int go7007_stop_streaming(struct vb2_queue *q)
> +static void go7007_stop_streaming(struct vb2_queue *q)
>  {
>         struct go7007 *go = vb2_get_drv_priv(q);
>         unsigned long flags;
> @@ -537,7 +537,6 @@ static int go7007_stop_streaming(struct vb2_queue *q)
>         /* Turn on Capture LED */
>         if (go->board_id == GO7007_BOARDID_ADS_USBAV_709)
>                 go7007_write_addr(go, 0x3c82, 0x000d);
> -       return 0;
>  }
>
>  static struct vb2_ops go7007_video_qops = {
> diff --git a/drivers/staging/media/msi3101/sdr-msi3101.c b/drivers/staging/media/msi3101/sdr-msi3101.c
> index 04ff29e..2136e60 100644
> --- a/drivers/staging/media/msi3101/sdr-msi3101.c
> +++ b/drivers/staging/media/msi3101/sdr-msi3101.c
> @@ -1577,13 +1577,12 @@ static int msi3101_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return ret;
>  }
>
> -static int msi3101_stop_streaming(struct vb2_queue *vq)
> +static void msi3101_stop_streaming(struct vb2_queue *vq)
>  {
>         struct msi3101_state *s = vb2_get_drv_priv(vq);
>         dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> -       if (mutex_lock_interruptible(&s->v4l2_lock))
> -               return -ERESTARTSYS;
> +       mutex_lock(&s->v4l2_lock);
>
>         if (s->udev)
>                 msi3101_isoc_cleanup(s);
> @@ -1595,8 +1594,6 @@ static int msi3101_stop_streaming(struct vb2_queue *vq)
>         msi3101_ctrl_msg(s, CMD_STOP_STREAMING, 0);
>
>         mutex_unlock(&s->v4l2_lock);
> -
> -       return 0;
>  }
>
>  static struct vb2_ops msi3101_vb2_ops = {
> diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
> index edcabcd..e056476 100644
> --- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
> +++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
> @@ -741,14 +741,13 @@ static int solo_enc_start_streaming(struct vb2_queue *q, unsigned int count)
>         return solo_ring_start(solo_enc->solo_dev);
>  }
>
> -static int solo_enc_stop_streaming(struct vb2_queue *q)
> +static void solo_enc_stop_streaming(struct vb2_queue *q)
>  {
>         struct solo_enc_dev *solo_enc = vb2_get_drv_priv(q);
>
>         solo_enc_off(solo_enc);
>         INIT_LIST_HEAD(&solo_enc->vidq_active);
>         solo_ring_stop(solo_enc->solo_dev);
> -       return 0;
>  }
>
>  static struct vb2_ops solo_enc_video_qops = {
> diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2.c b/drivers/staging/media/solo6x10/solo6x10-v4l2.c
> index 1815f76..5d0100e 100644
> --- a/drivers/staging/media/solo6x10/solo6x10-v4l2.c
> +++ b/drivers/staging/media/solo6x10/solo6x10-v4l2.c
> @@ -336,13 +336,12 @@ static int solo_start_streaming(struct vb2_queue *q, unsigned int count)
>         return solo_start_thread(solo_dev);
>  }
>
> -static int solo_stop_streaming(struct vb2_queue *q)
> +static void solo_stop_streaming(struct vb2_queue *q)
>  {
>         struct solo_dev *solo_dev = vb2_get_drv_priv(q);
>
>         solo_stop_thread(solo_dev);
>         INIT_LIST_HEAD(&solo_dev->vidq_active);
> -       return 0;
>  }
>
>  static void solo_buf_queue(struct vb2_buffer *vb)
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index af46211..3b57851 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -323,7 +323,7 @@ struct vb2_ops {
>         void (*buf_cleanup)(struct vb2_buffer *vb);
>
>         int (*start_streaming)(struct vb2_queue *q, unsigned int count);
> -       int (*stop_streaming)(struct vb2_queue *q);
> +       void (*stop_streaming)(struct vb2_queue *q);
>
>         void (*buf_queue)(struct vb2_buffer *vb);
>  };
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

[REVIEW PATCH 02/11] vb2: fix handling of data_offset and v4l2_plane.reserved[]

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The videobuf2-core did not zero the reserved array of v4l2_plane as it
should.

More serious is the fact that data_offset was not handled correctly:

- for capture devices it was never zeroed, which meant that it was
  uninitialized. Unless the driver sets it it was a completely random
  number.

- __qbuf_dmabuf had a completely incorrect length check that included
  data_offset.

- in the single-planar case data_offset was never correctly set to 0.
  The single-planar API doesn't support data_offset, so setting it
  to 0 is the right thing to do.

All these issues were found with v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f9059bb..1a09442 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1141,6 +1141,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 	unsigned int plane;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
+		for (plane = 0; plane < vb->num_planes; ++plane) {
+			memset(v4l2_planes[plane].reserved, 0,
+			       sizeof(v4l2_planes[plane].reserved));
+			v4l2_planes[plane].data_offset = 0;
+		}
+
 		/* Fill in driver-provided information for OUTPUT types */
 		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
 			/*
@@ -1169,8 +1175,6 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 					b->m.planes[plane].m.fd;
 				v4l2_planes[plane].length =
 					b->m.planes[plane].length;
-				v4l2_planes[plane].data_offset =
-					b->m.planes[plane].data_offset;
 			}
 		}
 	} else {
@@ -1180,10 +1184,10 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 		 * In videobuf we use our internal V4l2_planes struct for
 		 * single-planar buffers as well, for simplicity.
 		 */
-		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+		if (V4L2_TYPE_IS_OUTPUT(b->type))
 			v4l2_planes[0].bytesused = b->bytesused;
-			v4l2_planes[0].data_offset = 0;
-		}
+		/* Single-planar buffers never use data_offset */
+		v4l2_planes[0].data_offset = 0;
 
 		if (b->memory == V4L2_MEMORY_USERPTR) {
 			v4l2_planes[0].m.userptr = b->m.userptr;
@@ -1193,9 +1197,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 		if (b->memory == V4L2_MEMORY_DMABUF) {
 			v4l2_planes[0].m.fd = b->m.fd;
 			v4l2_planes[0].length = b->length;
-			v4l2_planes[0].data_offset = 0;
 		}
-
 	}
 
 	/* Zero flags that the vb2 core handles */
@@ -1374,8 +1376,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		if (planes[plane].length == 0)
 			planes[plane].length = dbuf->size;
 
-		if (planes[plane].length < planes[plane].data_offset +
-		    q->plane_sizes[plane]) {
+		if (planes[plane].length < q->plane_sizes[plane]) {
 			dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
 				plane);
 			ret = -EINVAL;
-- 
1.9.0

Re: [REVIEW PATCH 02/11] vb2: fix handling of data_offset and v4l2_plane.reserved[]

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The videobuf2-core did not zero the reserved array of v4l2_plane as it
> should.
>
> More serious is the fact that data_offset was not handled correctly:
>
> - for capture devices it was never zeroed, which meant that it was
>   uninitialized. Unless the driver sets it it was a completely random
>   number.
>
> - __qbuf_dmabuf had a completely incorrect length check that included
>   data_offset.
>
> - in the single-planar case data_offset was never correctly set to 0.
>   The single-planar API doesn't support data_offset, so setting it
>   to 0 is the right thing to do.
>
> All these issues were found with v4l2-compliance.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index f9059bb..1a09442 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1141,6 +1141,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>         unsigned int plane;
>
>         if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> +               for (plane = 0; plane < vb->num_planes; ++plane) {
> +                       memset(v4l2_planes[plane].reserved, 0,
> +                              sizeof(v4l2_planes[plane].reserved));
> +                       v4l2_planes[plane].data_offset = 0;
> +               }
> +

Perhaps we should just memset the whole v4l2_planes array to 0 over
all elements (ARRAY_SIZE)?
Also I would extract this above the if and zero out everything for
both multi and singleplanar.
You shouldn't need to zero it out below then.

>                 /* Fill in driver-provided information for OUTPUT types */
>                 if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>                         /*
> @@ -1169,8 +1175,6 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>                                         b->m.planes[plane].m.fd;
>                                 v4l2_planes[plane].length =
>                                         b->m.planes[plane].length;
> -                               v4l2_planes[plane].data_offset =
> -                                       b->m.planes[plane].data_offset;
>                         }
>                 }
>         } else {
> @@ -1180,10 +1184,10 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>                  * In videobuf we use our internal V4l2_planes struct for
>                  * single-planar buffers as well, for simplicity.
>                  */
> -               if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> +               if (V4L2_TYPE_IS_OUTPUT(b->type))
>                         v4l2_planes[0].bytesused = b->bytesused;
> -                       v4l2_planes[0].data_offset = 0;
> -               }
> +               /* Single-planar buffers never use data_offset */
> +               v4l2_planes[0].data_offset = 0;
>
>                 if (b->memory == V4L2_MEMORY_USERPTR) {
>                         v4l2_planes[0].m.userptr = b->m.userptr;
> @@ -1193,9 +1197,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>                 if (b->memory == V4L2_MEMORY_DMABUF) {
>                         v4l2_planes[0].m.fd = b->m.fd;
>                         v4l2_planes[0].length = b->length;
> -                       v4l2_planes[0].data_offset = 0;
>                 }
> -
>         }
>
>         /* Zero flags that the vb2 core handles */
> @@ -1374,8 +1376,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 if (planes[plane].length == 0)
>                         planes[plane].length = dbuf->size;
>
> -               if (planes[plane].length < planes[plane].data_offset +
> -                   q->plane_sizes[plane]) {
> +               if (planes[plane].length < q->plane_sizes[plane]) {

Good catch!

>                         dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
>                                 plane);
>                         ret = -EINVAL;
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

Re: [REVIEW PATCH 02/11] vb2: fix handling of data_offset and v4l2_plane.reserved[]

[Hans Verkuil]

On 04/07/2014 07:11 AM, Pawel Osciak wrote:
> On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The videobuf2-core did not zero the reserved array of v4l2_plane as it
>> should.
>>
>> More serious is the fact that data_offset was not handled correctly:
>>
>> - for capture devices it was never zeroed, which meant that it was
>>   uninitialized. Unless the driver sets it it was a completely random
>>   number.
>>
>> - __qbuf_dmabuf had a completely incorrect length check that included
>>   data_offset.
>>
>> - in the single-planar case data_offset was never correctly set to 0.
>>   The single-planar API doesn't support data_offset, so setting it
>>   to 0 is the right thing to do.
>>
>> All these issues were found with v4l2-compliance.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index f9059bb..1a09442 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1141,6 +1141,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>         unsigned int plane;
>>
>>         if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>> +               for (plane = 0; plane < vb->num_planes; ++plane) {
>> +                       memset(v4l2_planes[plane].reserved, 0,
>> +                              sizeof(v4l2_planes[plane].reserved));
>> +                       v4l2_planes[plane].data_offset = 0;
>> +               }
>> +
> 
> Perhaps we should just memset the whole v4l2_planes array to 0 over
> all elements (ARRAY_SIZE)?

You can't do that here since in the mmap case the v4l2_planes array has already
fields filled in. However, by doing a memset in __qbuf_userptr and __qbuf_dmabuf,
before calling __fill_vb2_buffer, this can be solved neatly and correctly.

> Also I would extract this above the if and zero out everything for
> both multi and singleplanar.
> You shouldn't need to zero it out below then.
> 
>>                 /* Fill in driver-provided information for OUTPUT types */
>>                 if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>>                         /*
>> @@ -1169,8 +1175,6 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>                                         b->m.planes[plane].m.fd;
>>                                 v4l2_planes[plane].length =
>>                                         b->m.planes[plane].length;
>> -                               v4l2_planes[plane].data_offset =
>> -                                       b->m.planes[plane].data_offset;

I've added an explicit explanation for this change to the commit log as
well:

- in __fill_vb2_buffer in the DMABUF case the data_offset field was
  unconditionally copied from v4l2_buffer to v4l2_plane when this
  should only happen in the output case.

Regards,

	Hans

>>                         }
>>                 }
>>         } else {
>> @@ -1180,10 +1184,10 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>                  * In videobuf we use our internal V4l2_planes struct for
>>                  * single-planar buffers as well, for simplicity.
>>                  */
>> -               if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>> +               if (V4L2_TYPE_IS_OUTPUT(b->type))
>>                         v4l2_planes[0].bytesused = b->bytesused;
>> -                       v4l2_planes[0].data_offset = 0;
>> -               }
>> +               /* Single-planar buffers never use data_offset */
>> +               v4l2_planes[0].data_offset = 0;
>>
>>                 if (b->memory == V4L2_MEMORY_USERPTR) {
>>                         v4l2_planes[0].m.userptr = b->m.userptr;
>> @@ -1193,9 +1197,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>                 if (b->memory == V4L2_MEMORY_DMABUF) {
>>                         v4l2_planes[0].m.fd = b->m.fd;
>>                         v4l2_planes[0].length = b->length;
>> -                       v4l2_planes[0].data_offset = 0;
>>                 }
>> -
>>         }
>>
>>         /* Zero flags that the vb2 core handles */
>> @@ -1374,8 +1376,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                 if (planes[plane].length == 0)
>>                         planes[plane].length = dbuf->size;
>>
>> -               if (planes[plane].length < planes[plane].data_offset +
>> -                   q->plane_sizes[plane]) {
>> +               if (planes[plane].length < q->plane_sizes[plane]) {
> 
> Good catch!
> 
>>                         dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
>>                                 plane);
>>                         ret = -EINVAL;
>> --
>> 1.9.0
>>
> 
> 
> 

[REVIEW PATCH 03/11] vb2: if bytesused is 0, then fill with output buffer length

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The application should really always fill in bytesused for output
buffers, unfortunately the vb2 framework never checked for that.

So for single planar formats replace a bytesused of 0 by the length
of the buffer, and for multiplanar format do the same if bytesused is
0 for ALL planes.

This seems to be what the user really intended if v4l2_buffer was
just memset to 0.

I'm afraid that just checking for this and returning an error would
break too many applications. Quite a few drivers never check for bytesused
at all and just use the buffer length instead.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 1a09442..83e78e9 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1145,19 +1145,35 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 			memset(v4l2_planes[plane].reserved, 0,
 			       sizeof(v4l2_planes[plane].reserved));
 			v4l2_planes[plane].data_offset = 0;
+			v4l2_planes[plane].bytesused = 0;
 		}
 
 		/* Fill in driver-provided information for OUTPUT types */
 		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+			bool bytesused_is_used;
+
+			/* Check if bytesused == 0 for all planes */
+			for (plane = 0; plane < vb->num_planes; ++plane)
+				if (b->m.planes[plane].bytesused)
+					break;
+			bytesused_is_used = plane < vb->num_planes;
+
 			/*
 			 * Will have to go up to b->length when API starts
 			 * accepting variable number of planes.
+			 *
+			 * If bytesused_is_used is false, then fall back to the
+			 * full buffer size. In that case userspace clearly
+			 * never bothered to set it and it's a safe assumption
+			 * that they really meant to use the full plane sizes.
 			 */
 			for (plane = 0; plane < vb->num_planes; ++plane) {
-				v4l2_planes[plane].bytesused =
-					b->m.planes[plane].bytesused;
-				v4l2_planes[plane].data_offset =
-					b->m.planes[plane].data_offset;
+				struct v4l2_plane *pdst = &v4l2_planes[plane];
+				struct v4l2_plane *psrc = &b->m.planes[plane];
+
+				pdst->bytesused = bytesused_is_used ?
+					psrc->bytesused : psrc->length;
+				pdst->data_offset = psrc->data_offset;
 			}
 		}
 
@@ -1183,9 +1199,15 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 		 * so fill in relevant v4l2_buffer struct fields instead.
 		 * In videobuf we use our internal V4l2_planes struct for
 		 * single-planar buffers as well, for simplicity.
+		 *
+		 * If bytesused == 0, then fall back to the full buffer size
+		 * as that's a sensible default.
 		 */
 		if (V4L2_TYPE_IS_OUTPUT(b->type))
-			v4l2_planes[0].bytesused = b->bytesused;
+			v4l2_planes[0].bytesused =
+				b->bytesused ? b->bytesused : b->length;
+		else
+			v4l2_planes[0].bytesused = 0;
 		/* Single-planar buffers never use data_offset */
 		v4l2_planes[0].data_offset = 0;
 
-- 
1.9.0

Re: [REVIEW PATCH 03/11] vb2: if bytesused is 0, then fill with output buffer length

[Pawel Osciak]

I'm thinking, that if we are doing this, perhaps we should just update
the API to allow this case, i.e. say that if the bytesused is not set
for any planes, length will be used by default?
This would be backwards-compatible.

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The application should really always fill in bytesused for output
> buffers, unfortunately the vb2 framework never checked for that.
>
> So for single planar formats replace a bytesused of 0 by the length
> of the buffer, and for multiplanar format do the same if bytesused is
> 0 for ALL planes.
>
> This seems to be what the user really intended if v4l2_buffer was
> just memset to 0.
>
> I'm afraid that just checking for this and returning an error would
> break too many applications. Quite a few drivers never check for bytesused
> at all and just use the buffer length instead.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 1a09442..83e78e9 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1145,19 +1145,35 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>                         memset(v4l2_planes[plane].reserved, 0,
>                                sizeof(v4l2_planes[plane].reserved));
>                         v4l2_planes[plane].data_offset = 0;
> +                       v4l2_planes[plane].bytesused = 0;
>                 }
>
>                 /* Fill in driver-provided information for OUTPUT types */
>                 if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> +                       bool bytesused_is_used;
> +
> +                       /* Check if bytesused == 0 for all planes */
> +                       for (plane = 0; plane < vb->num_planes; ++plane)
> +                               if (b->m.planes[plane].bytesused)
> +                                       break;
> +                       bytesused_is_used = plane < vb->num_planes;
> +
>                         /*
>                          * Will have to go up to b->length when API starts
>                          * accepting variable number of planes.
> +                        *
> +                        * If bytesused_is_used is false, then fall back to the
> +                        * full buffer size. In that case userspace clearly
> +                        * never bothered to set it and it's a safe assumption
> +                        * that they really meant to use the full plane sizes.
>                          */
>                         for (plane = 0; plane < vb->num_planes; ++plane) {
> -                               v4l2_planes[plane].bytesused =
> -                                       b->m.planes[plane].bytesused;
> -                               v4l2_planes[plane].data_offset =
> -                                       b->m.planes[plane].data_offset;
> +                               struct v4l2_plane *pdst = &v4l2_planes[plane];
> +                               struct v4l2_plane *psrc = &b->m.planes[plane];
> +
> +                               pdst->bytesused = bytesused_is_used ?
> +                                       psrc->bytesused : psrc->length;
> +                               pdst->data_offset = psrc->data_offset;
>                         }
>                 }
>
> @@ -1183,9 +1199,15 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>                  * so fill in relevant v4l2_buffer struct fields instead.
>                  * In videobuf we use our internal V4l2_planes struct for
>                  * single-planar buffers as well, for simplicity.
> +                *
> +                * If bytesused == 0, then fall back to the full buffer size
> +                * as that's a sensible default.
>                  */
>                 if (V4L2_TYPE_IS_OUTPUT(b->type))
> -                       v4l2_planes[0].bytesused = b->bytesused;
> +                       v4l2_planes[0].bytesused =
> +                               b->bytesused ? b->bytesused : b->length;
> +               else
> +                       v4l2_planes[0].bytesused = 0;
>                 /* Single-planar buffers never use data_offset */
>                 v4l2_planes[0].data_offset = 0;
>
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

Re: [REVIEW PATCH 03/11] vb2: if bytesused is 0, then fill with output buffer length

[Hans Verkuil]

On 04/07/2014 09:20 AM, Pawel Osciak wrote:
> I'm thinking, that if we are doing this, perhaps we should just update
> the API to allow this case, i.e. say that if the bytesused is not set

With 'not set' you mean 'is 0', right?

> for any planes, length will be used by default?
> This would be backwards-compatible.

I agree with that. I'll update the doc.

Regards,

	Hans

> 
> On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The application should really always fill in bytesused for output
>> buffers, unfortunately the vb2 framework never checked for that.
>>
>> So for single planar formats replace a bytesused of 0 by the length
>> of the buffer, and for multiplanar format do the same if bytesused is
>> 0 for ALL planes.
>>
>> This seems to be what the user really intended if v4l2_buffer was
>> just memset to 0.
>>
>> I'm afraid that just checking for this and returning an error would
>> break too many applications. Quite a few drivers never check for bytesused
>> at all and just use the buffer length instead.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Acked-by: Pawel Osciak <pawel@osciak.com>
> 
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 32 +++++++++++++++++++++++++++-----
>>  1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 1a09442..83e78e9 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1145,19 +1145,35 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>                         memset(v4l2_planes[plane].reserved, 0,
>>                                sizeof(v4l2_planes[plane].reserved));
>>                         v4l2_planes[plane].data_offset = 0;
>> +                       v4l2_planes[plane].bytesused = 0;
>>                 }
>>
>>                 /* Fill in driver-provided information for OUTPUT types */
>>                 if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>> +                       bool bytesused_is_used;
>> +
>> +                       /* Check if bytesused == 0 for all planes */
>> +                       for (plane = 0; plane < vb->num_planes; ++plane)
>> +                               if (b->m.planes[plane].bytesused)
>> +                                       break;
>> +                       bytesused_is_used = plane < vb->num_planes;
>> +
>>                         /*
>>                          * Will have to go up to b->length when API starts
>>                          * accepting variable number of planes.
>> +                        *
>> +                        * If bytesused_is_used is false, then fall back to the
>> +                        * full buffer size. In that case userspace clearly
>> +                        * never bothered to set it and it's a safe assumption
>> +                        * that they really meant to use the full plane sizes.
>>                          */
>>                         for (plane = 0; plane < vb->num_planes; ++plane) {
>> -                               v4l2_planes[plane].bytesused =
>> -                                       b->m.planes[plane].bytesused;
>> -                               v4l2_planes[plane].data_offset =
>> -                                       b->m.planes[plane].data_offset;
>> +                               struct v4l2_plane *pdst = &v4l2_planes[plane];
>> +                               struct v4l2_plane *psrc = &b->m.planes[plane];
>> +
>> +                               pdst->bytesused = bytesused_is_used ?
>> +                                       psrc->bytesused : psrc->length;
>> +                               pdst->data_offset = psrc->data_offset;
>>                         }
>>                 }
>>
>> @@ -1183,9 +1199,15 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>                  * so fill in relevant v4l2_buffer struct fields instead.
>>                  * In videobuf we use our internal V4l2_planes struct for
>>                  * single-planar buffers as well, for simplicity.
>> +                *
>> +                * If bytesused == 0, then fall back to the full buffer size
>> +                * as that's a sensible default.
>>                  */
>>                 if (V4L2_TYPE_IS_OUTPUT(b->type))
>> -                       v4l2_planes[0].bytesused = b->bytesused;
>> +                       v4l2_planes[0].bytesused =
>> +                               b->bytesused ? b->bytesused : b->length;
>> +               else
>> +                       v4l2_planes[0].bytesused = 0;
>>                 /* Single-planar buffers never use data_offset */
>>                 v4l2_planes[0].data_offset = 0;
>>
>> --
>> 1.9.0
>>
> 
> 
> 

Re: [REVIEW PATCH 03/11] vb2: if bytesused is 0, then fill with output buffer length

[Pawel Osciak]

On Mon, Apr 7, 2014 at 4:39 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 04/07/2014 09:20 AM, Pawel Osciak wrote:
>> I'm thinking, that if we are doing this, perhaps we should just update
>> the API to allow this case, i.e. say that if the bytesused is not set
>
> With 'not set' you mean 'is 0', right?

Yes, correct.

>
>> for any planes, length will be used by default?
>> This would be backwards-compatible.
>
> I agree with that. I'll update the doc.
>
> Regards,
>
>         Hans
>

[REVIEW PATCH 04/11] vb2: use correct prefix

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Many dprintk's in vb2 use a hardcoded prefix with the function name. In
many cases that is now outdated. Replace prefixes by the function name using
__func__. At least now I know if I see a 'qbuf:' prefix whether that refers
to the mmap, userptr or dmabuf variant.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 102 ++++++++++++++++---------------
 1 file changed, 54 insertions(+), 48 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 83e78e9..71be247 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -371,7 +371,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 		if (q->bufs[buffer] == NULL)
 			continue;
 		if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
-			dprintk(1, "reqbufs: preparing buffers, cannot free\n");
+			dprintk(1, "%s: preparing buffers, cannot free\n",
+					__func__);
 			return -EAGAIN;
 		}
 	}
@@ -656,12 +657,12 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	int ret;
 
 	if (b->type != q->type) {
-		dprintk(1, "querybuf: wrong buffer type\n");
+		dprintk(1, "%s: wrong buffer type\n", __func__);
 		return -EINVAL;
 	}
 
 	if (b->index >= q->num_buffers) {
-		dprintk(1, "querybuf: buffer index out of range\n");
+		dprintk(1, "%s: buffer index out of range\n", __func__);
 		return -EINVAL;
 	}
 	vb = q->bufs[b->index];
@@ -721,12 +722,12 @@ static int __verify_memory_type(struct vb2_queue *q,
 {
 	if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR &&
 	    memory != V4L2_MEMORY_DMABUF) {
-		dprintk(1, "reqbufs: unsupported memory type\n");
+		dprintk(1, "%s: unsupported memory type\n", __func__);
 		return -EINVAL;
 	}
 
 	if (type != q->type) {
-		dprintk(1, "reqbufs: requested type is incorrect\n");
+		dprintk(1, "%s: requested type is incorrect\n", __func__);
 		return -EINVAL;
 	}
 
@@ -735,17 +736,20 @@ static int __verify_memory_type(struct vb2_queue *q,
 	 * are available.
 	 */
 	if (memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
-		dprintk(1, "reqbufs: MMAP for current setup unsupported\n");
+		dprintk(1, "%s: MMAP for current setup unsupported\n",
+				__func__);
 		return -EINVAL;
 	}
 
 	if (memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
-		dprintk(1, "reqbufs: USERPTR for current setup unsupported\n");
+		dprintk(1, "%s: USERPTR for current setup unsupported\n",
+				__func__);
 		return -EINVAL;
 	}
 
 	if (memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
-		dprintk(1, "reqbufs: DMABUF for current setup unsupported\n");
+		dprintk(1, "%s: DMABUF for current setup unsupported\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -755,7 +759,7 @@ static int __verify_memory_type(struct vb2_queue *q,
 	 * do the memory and type validation.
 	 */
 	if (q->fileio) {
-		dprintk(1, "reqbufs: file io in progress\n");
+		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
 	return 0;
@@ -790,7 +794,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	int ret;
 
 	if (q->streaming) {
-		dprintk(1, "reqbufs: streaming active\n");
+		dprintk(1, "%s: streaming active\n", __func__);
 		return -EBUSY;
 	}
 
@@ -800,7 +804,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 		 * are not in use and can be freed.
 		 */
 		if (q->memory == V4L2_MEMORY_MMAP && __buffers_in_use(q)) {
-			dprintk(1, "reqbufs: memory in use, cannot free\n");
+			dprintk(1, "%s: memory in use, cannot free\n", __func__);
 			return -EBUSY;
 		}
 
@@ -1272,15 +1276,14 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		    && vb->v4l2_planes[plane].length == planes[plane].length)
 			continue;
 
-		dprintk(3, "qbuf: userspace address for plane %d changed, "
-				"reacquiring memory\n", plane);
+		dprintk(3, "%s: userspace address for plane %d changed, reacquiring memory\n",
+			__func__, plane);
 
 		/* Check if the provided plane buffer is large enough */
 		if (planes[plane].length < q->plane_sizes[plane]) {
-			dprintk(1, "qbuf: provided buffer size %u is less than "
-						"setup size %u for plane %d\n",
-						planes[plane].length,
-						q->plane_sizes[plane], plane);
+			dprintk(1, "%s: provided buffer size %u is less than setup size %u for plane %d\n",
+				__func__, planes[plane].length,
+				q->plane_sizes[plane], plane);
 			ret = -EINVAL;
 			goto err;
 		}
@@ -1302,8 +1305,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 				      planes[plane].m.userptr,
 				      planes[plane].length, write);
 		if (IS_ERR_OR_NULL(mem_priv)) {
-			dprintk(1, "qbuf: failed acquiring userspace "
-						"memory for plane %d\n", plane);
+			dprintk(1, "%s: failed acquiring userspace memory for plane %d\n",
+				__func__, plane);
 			fail_memop(vb, get_userptr);
 			ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
 			goto err;
@@ -1326,7 +1329,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		 */
 		ret = call_vb_qop(vb, buf_init, vb);
 		if (ret) {
-			dprintk(1, "qbuf: buffer initialization failed\n");
+			dprintk(1, "%s: buffer initialization failed\n",
+				__func__);
 			fail_vb_qop(vb, buf_init);
 			goto err;
 		}
@@ -1334,7 +1338,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 
 	ret = call_vb_qop(vb, buf_prepare, vb);
 	if (ret) {
-		dprintk(1, "qbuf: buffer preparation failed\n");
+		dprintk(1, "%s: buffer preparation failed\n", __func__);
 		fail_vb_qop(vb, buf_prepare);
 		call_vb_qop(vb, buf_cleanup, vb);
 		goto err;
@@ -1388,8 +1392,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
 
 		if (IS_ERR_OR_NULL(dbuf)) {
-			dprintk(1, "qbuf: invalid dmabuf fd for plane %d\n",
-				plane);
+			dprintk(1, "%s: invalid dmabuf fd for plane %d\n",
+				__func__, plane);
 			ret = -EINVAL;
 			goto err;
 		}
@@ -1399,8 +1403,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 			planes[plane].length = dbuf->size;
 
 		if (planes[plane].length < q->plane_sizes[plane]) {
-			dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
-				plane);
+			dprintk(1, "%s: invalid dmabuf length for plane %d\n",
+				__func__, plane);
 			ret = -EINVAL;
 			goto err;
 		}
@@ -1412,7 +1416,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 			continue;
 		}
 
-		dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
+		dprintk(1, "%s: buffer for plane %d changed\n",
+			__func__, plane);
 
 		if (!reacquired) {
 			reacquired = true;
@@ -1427,7 +1432,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
 			dbuf, planes[plane].length, write);
 		if (IS_ERR(mem_priv)) {
-			dprintk(1, "qbuf: failed to attach dmabuf\n");
+			dprintk(1, "%s: failed to attach dmabuf\n", __func__);
 			fail_memop(vb, attach_dmabuf);
 			ret = PTR_ERR(mem_priv);
 			dma_buf_put(dbuf);
@@ -1445,8 +1450,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
 		if (ret) {
-			dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
-				plane);
+			dprintk(1, "%s: failed to map dmabuf for plane %d\n",
+				__func__, plane);
 			fail_memop(vb, map_dmabuf);
 			goto err;
 		}
@@ -1467,7 +1472,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		 */
 		ret = call_vb_qop(vb, buf_init, vb);
 		if (ret) {
-			dprintk(1, "qbuf: buffer initialization failed\n");
+			dprintk(1, "%s: buffer initialization failed\n",
+				__func__);
 			fail_vb_qop(vb, buf_init);
 			goto err;
 		}
@@ -1475,7 +1481,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 
 	ret = call_vb_qop(vb, buf_prepare, vb);
 	if (ret) {
-		dprintk(1, "qbuf: buffer preparation failed\n");
+		dprintk(1, "%s: buffer preparation failed\n", __func__);
 		fail_vb_qop(vb, buf_prepare);
 		call_vb_qop(vb, buf_cleanup, vb);
 		goto err;
@@ -1671,7 +1677,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
 		return 0;
 
 	fail_qop(q, start_streaming);
-	dprintk(1, "qbuf: driver refused to start streaming\n");
+	dprintk(1, "%s: driver refused to start streaming\n", __func__);
 	if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
 		unsigned i;
 
@@ -1709,7 +1715,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	case VB2_BUF_STATE_PREPARED:
 		break;
 	case VB2_BUF_STATE_PREPARING:
-		dprintk(1, "qbuf: buffer still being prepared\n");
+		dprintk(1, "%s: buffer still being prepared\n", __func__);
 		return -EINVAL;
 	default:
 		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
@@ -1945,7 +1951,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	int ret;
 
 	if (b->type != q->type) {
-		dprintk(1, "dqbuf: invalid buffer type\n");
+		dprintk(1, "%s: invalid buffer type\n", __func__);
 		return -EINVAL;
 	}
 	ret = __vb2_get_done_vb(q, &vb, b, nonblocking);
@@ -1954,13 +1960,13 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_DONE:
-		dprintk(3, "dqbuf: Returning done buffer\n");
+		dprintk(3, "%s: Returning done buffer\n", __func__);
 		break;
 	case VB2_BUF_STATE_ERROR:
-		dprintk(3, "dqbuf: Returning done buffer with errors\n");
+		dprintk(3, "%s: Returning done buffer with errors\n", __func__);
 		break;
 	default:
-		dprintk(1, "dqbuf: Invalid buffer state\n");
+		dprintk(1, "%s: Invalid buffer state\n", __func__);
 		return -EINVAL;
 	}
 
@@ -2004,7 +2010,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 {
 	if (q->fileio) {
-		dprintk(1, "dqbuf: file io in progress\n");
+		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
 	return vb2_internal_dqbuf(q, b, nonblocking);
@@ -2076,7 +2082,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 	int ret;
 
 	if (type != q->type) {
-		dprintk(1, "streamon: invalid stream type\n");
+		dprintk(1, "%s: invalid stream type\n", __func__);
 		return -EINVAL;
 	}
 
@@ -2086,17 +2092,17 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 	}
 
 	if (!q->num_buffers) {
-		dprintk(1, "streamon: no buffers have been allocated\n");
+		dprintk(1, "%s: no buffers have been allocated\n", __func__);
 		return -EINVAL;
 	}
 
 	if (!q->num_buffers) {
-		dprintk(1, "streamon: no buffers have been allocated\n");
+		dprintk(1, "%s: no buffers have been allocated\n", __func__);
 		return -EINVAL;
 	}
 	if (q->num_buffers < q->min_buffers_needed) {
-		dprintk(1, "streamon: need at least %u allocated buffers\n",
-				q->min_buffers_needed);
+		dprintk(1, "%s: need at least %u allocated buffers\n",
+				__func__, q->min_buffers_needed);
 		return -EINVAL;
 	}
 
@@ -2134,7 +2140,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	if (q->fileio) {
-		dprintk(1, "streamon: file io in progress\n");
+		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
 	return vb2_internal_streamon(q, type);
@@ -2144,7 +2150,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon);
 static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	if (type != q->type) {
-		dprintk(1, "streamoff: invalid stream type\n");
+		dprintk(1, "%s: invalid stream type\n", __func__);
 		return -EINVAL;
 	}
 
@@ -2181,7 +2187,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	if (q->fileio) {
-		dprintk(1, "streamoff: file io in progress\n");
+		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
 	return vb2_internal_streamoff(q, type);
@@ -2249,7 +2255,7 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
 	}
 
 	if (eb->type != q->type) {
-		dprintk(1, "qbuf: invalid buffer type\n");
+		dprintk(1, "%s: invalid buffer type\n", __func__);
 		return -EINVAL;
 	}
 
@@ -2863,7 +2869,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->b.index = index;
 		fileio->b.bytesused = buf->pos;
 		ret = vb2_internal_qbuf(q, &fileio->b);
-		dprintk(5, "file io: vb2_dbuf result: %d\n", ret);
+		dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
 		if (ret)
 			return ret;
 
-- 
1.9.0

Re: [REVIEW PATCH 04/11] vb2: use correct prefix

[Pawel Osciak]

The idea behind not using __func__ was that it was much more
informative when debugging to see a "reqbufs" prefix instead of, for
example "__verify_memory_type". But since some of the functions are
shared across multiple ioctl impls now (e.g. __verify_memory_type is
used by both reqbufs and createbufs), as much as I would prefer to
keep this convention, it'd probably be too much to maintain.

But if we want to do this, we should move "__func__" to dprintk()
definition, instead of adding it to the call sites.

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Many dprintk's in vb2 use a hardcoded prefix with the function name. In
> many cases that is now outdated. Replace prefixes by the function name using
> __func__. At least now I know if I see a 'qbuf:' prefix whether that refers
> to the mmap, userptr or dmabuf variant.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 102 ++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 83e78e9..71be247 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -371,7 +371,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>                 if (q->bufs[buffer] == NULL)
>                         continue;
>                 if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
> -                       dprintk(1, "reqbufs: preparing buffers, cannot free\n");
> +                       dprintk(1, "%s: preparing buffers, cannot free\n",
> +                                       __func__);
>                         return -EAGAIN;
>                 }
>         }
> @@ -656,12 +657,12 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>         int ret;
>
>         if (b->type != q->type) {
> -               dprintk(1, "querybuf: wrong buffer type\n");
> +               dprintk(1, "%s: wrong buffer type\n", __func__);
>                 return -EINVAL;
>         }
>
>         if (b->index >= q->num_buffers) {
> -               dprintk(1, "querybuf: buffer index out of range\n");
> +               dprintk(1, "%s: buffer index out of range\n", __func__);
>                 return -EINVAL;
>         }
>         vb = q->bufs[b->index];
> @@ -721,12 +722,12 @@ static int __verify_memory_type(struct vb2_queue *q,
>  {
>         if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR &&
>             memory != V4L2_MEMORY_DMABUF) {
> -               dprintk(1, "reqbufs: unsupported memory type\n");
> +               dprintk(1, "%s: unsupported memory type\n", __func__);
>                 return -EINVAL;
>         }
>
>         if (type != q->type) {
> -               dprintk(1, "reqbufs: requested type is incorrect\n");
> +               dprintk(1, "%s: requested type is incorrect\n", __func__);
>                 return -EINVAL;
>         }
>
> @@ -735,17 +736,20 @@ static int __verify_memory_type(struct vb2_queue *q,
>          * are available.
>          */
>         if (memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> -               dprintk(1, "reqbufs: MMAP for current setup unsupported\n");
> +               dprintk(1, "%s: MMAP for current setup unsupported\n",
> +                               __func__);
>                 return -EINVAL;
>         }
>
>         if (memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> -               dprintk(1, "reqbufs: USERPTR for current setup unsupported\n");
> +               dprintk(1, "%s: USERPTR for current setup unsupported\n",
> +                               __func__);
>                 return -EINVAL;
>         }
>
>         if (memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
> -               dprintk(1, "reqbufs: DMABUF for current setup unsupported\n");
> +               dprintk(1, "%s: DMABUF for current setup unsupported\n",
> +                               __func__);
>                 return -EINVAL;
>         }
>
> @@ -755,7 +759,7 @@ static int __verify_memory_type(struct vb2_queue *q,
>          * do the memory and type validation.
>          */
>         if (q->fileio) {
> -               dprintk(1, "reqbufs: file io in progress\n");
> +               dprintk(1, "%s: file io in progress\n", __func__);
>                 return -EBUSY;
>         }
>         return 0;
> @@ -790,7 +794,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>         int ret;
>
>         if (q->streaming) {
> -               dprintk(1, "reqbufs: streaming active\n");
> +               dprintk(1, "%s: streaming active\n", __func__);
>                 return -EBUSY;
>         }
>
> @@ -800,7 +804,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>                  * are not in use and can be freed.
>                  */
>                 if (q->memory == V4L2_MEMORY_MMAP && __buffers_in_use(q)) {
> -                       dprintk(1, "reqbufs: memory in use, cannot free\n");
> +                       dprintk(1, "%s: memory in use, cannot free\n", __func__);
>                         return -EBUSY;
>                 }
>
> @@ -1272,15 +1276,14 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                     && vb->v4l2_planes[plane].length == planes[plane].length)
>                         continue;
>
> -               dprintk(3, "qbuf: userspace address for plane %d changed, "
> -                               "reacquiring memory\n", plane);
> +               dprintk(3, "%s: userspace address for plane %d changed, reacquiring memory\n",
> +                       __func__, plane);
>
>                 /* Check if the provided plane buffer is large enough */
>                 if (planes[plane].length < q->plane_sizes[plane]) {
> -                       dprintk(1, "qbuf: provided buffer size %u is less than "
> -                                               "setup size %u for plane %d\n",
> -                                               planes[plane].length,
> -                                               q->plane_sizes[plane], plane);
> +                       dprintk(1, "%s: provided buffer size %u is less than setup size %u for plane %d\n",
> +                               __func__, planes[plane].length,
> +                               q->plane_sizes[plane], plane);
>                         ret = -EINVAL;
>                         goto err;
>                 }
> @@ -1302,8 +1305,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                                       planes[plane].m.userptr,
>                                       planes[plane].length, write);
>                 if (IS_ERR_OR_NULL(mem_priv)) {
> -                       dprintk(1, "qbuf: failed acquiring userspace "
> -                                               "memory for plane %d\n", plane);
> +                       dprintk(1, "%s: failed acquiring userspace memory for plane %d\n",
> +                               __func__, plane);
>                         fail_memop(vb, get_userptr);
>                         ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
>                         goto err;
> @@ -1326,7 +1329,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                  */
>                 ret = call_vb_qop(vb, buf_init, vb);
>                 if (ret) {
> -                       dprintk(1, "qbuf: buffer initialization failed\n");
> +                       dprintk(1, "%s: buffer initialization failed\n",
> +                               __func__);
>                         fail_vb_qop(vb, buf_init);
>                         goto err;
>                 }
> @@ -1334,7 +1338,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>
>         ret = call_vb_qop(vb, buf_prepare, vb);
>         if (ret) {
> -               dprintk(1, "qbuf: buffer preparation failed\n");
> +               dprintk(1, "%s: buffer preparation failed\n", __func__);
>                 fail_vb_qop(vb, buf_prepare);
>                 call_vb_qop(vb, buf_cleanup, vb);
>                 goto err;
> @@ -1388,8 +1392,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>
>                 if (IS_ERR_OR_NULL(dbuf)) {
> -                       dprintk(1, "qbuf: invalid dmabuf fd for plane %d\n",
> -                               plane);
> +                       dprintk(1, "%s: invalid dmabuf fd for plane %d\n",
> +                               __func__, plane);
>                         ret = -EINVAL;
>                         goto err;
>                 }
> @@ -1399,8 +1403,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                         planes[plane].length = dbuf->size;
>
>                 if (planes[plane].length < q->plane_sizes[plane]) {
> -                       dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
> -                               plane);
> +                       dprintk(1, "%s: invalid dmabuf length for plane %d\n",
> +                               __func__, plane);
>                         ret = -EINVAL;
>                         goto err;
>                 }
> @@ -1412,7 +1416,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                         continue;
>                 }
>
> -               dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
> +               dprintk(1, "%s: buffer for plane %d changed\n",
> +                       __func__, plane);
>
>                 if (!reacquired) {
>                         reacquired = true;
> @@ -1427,7 +1432,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
>                         dbuf, planes[plane].length, write);
>                 if (IS_ERR(mem_priv)) {
> -                       dprintk(1, "qbuf: failed to attach dmabuf\n");
> +                       dprintk(1, "%s: failed to attach dmabuf\n", __func__);
>                         fail_memop(vb, attach_dmabuf);
>                         ret = PTR_ERR(mem_priv);
>                         dma_buf_put(dbuf);
> @@ -1445,8 +1450,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>         for (plane = 0; plane < vb->num_planes; ++plane) {
>                 ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>                 if (ret) {
> -                       dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
> -                               plane);
> +                       dprintk(1, "%s: failed to map dmabuf for plane %d\n",
> +                               __func__, plane);
>                         fail_memop(vb, map_dmabuf);
>                         goto err;
>                 }
> @@ -1467,7 +1472,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                  */
>                 ret = call_vb_qop(vb, buf_init, vb);
>                 if (ret) {
> -                       dprintk(1, "qbuf: buffer initialization failed\n");
> +                       dprintk(1, "%s: buffer initialization failed\n",
> +                               __func__);
>                         fail_vb_qop(vb, buf_init);
>                         goto err;
>                 }
> @@ -1475,7 +1481,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>
>         ret = call_vb_qop(vb, buf_prepare, vb);
>         if (ret) {
> -               dprintk(1, "qbuf: buffer preparation failed\n");
> +               dprintk(1, "%s: buffer preparation failed\n", __func__);
>                 fail_vb_qop(vb, buf_prepare);
>                 call_vb_qop(vb, buf_cleanup, vb);
>                 goto err;
> @@ -1671,7 +1677,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>                 return 0;
>
>         fail_qop(q, start_streaming);
> -       dprintk(1, "qbuf: driver refused to start streaming\n");
> +       dprintk(1, "%s: driver refused to start streaming\n", __func__);
>         if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
>                 unsigned i;
>
> @@ -1709,7 +1715,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>         case VB2_BUF_STATE_PREPARED:
>                 break;
>         case VB2_BUF_STATE_PREPARING:
> -               dprintk(1, "qbuf: buffer still being prepared\n");
> +               dprintk(1, "%s: buffer still being prepared\n", __func__);
>                 return -EINVAL;
>         default:
>                 dprintk(1, "%s(): invalid buffer state %d\n", __func__,
> @@ -1945,7 +1951,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>         int ret;
>
>         if (b->type != q->type) {
> -               dprintk(1, "dqbuf: invalid buffer type\n");
> +               dprintk(1, "%s: invalid buffer type\n", __func__);
>                 return -EINVAL;
>         }
>         ret = __vb2_get_done_vb(q, &vb, b, nonblocking);
> @@ -1954,13 +1960,13 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>
>         switch (vb->state) {
>         case VB2_BUF_STATE_DONE:
> -               dprintk(3, "dqbuf: Returning done buffer\n");
> +               dprintk(3, "%s: Returning done buffer\n", __func__);
>                 break;
>         case VB2_BUF_STATE_ERROR:
> -               dprintk(3, "dqbuf: Returning done buffer with errors\n");
> +               dprintk(3, "%s: Returning done buffer with errors\n", __func__);
>                 break;
>         default:
> -               dprintk(1, "dqbuf: Invalid buffer state\n");
> +               dprintk(1, "%s: Invalid buffer state\n", __func__);
>                 return -EINVAL;
>         }
>
> @@ -2004,7 +2010,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>  int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
>  {
>         if (q->fileio) {
> -               dprintk(1, "dqbuf: file io in progress\n");
> +               dprintk(1, "%s: file io in progress\n", __func__);
>                 return -EBUSY;
>         }
>         return vb2_internal_dqbuf(q, b, nonblocking);
> @@ -2076,7 +2082,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>         int ret;
>
>         if (type != q->type) {
> -               dprintk(1, "streamon: invalid stream type\n");
> +               dprintk(1, "%s: invalid stream type\n", __func__);
>                 return -EINVAL;
>         }
>
> @@ -2086,17 +2092,17 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>         }
>
>         if (!q->num_buffers) {
> -               dprintk(1, "streamon: no buffers have been allocated\n");
> +               dprintk(1, "%s: no buffers have been allocated\n", __func__);
>                 return -EINVAL;
>         }
>
>         if (!q->num_buffers) {
> -               dprintk(1, "streamon: no buffers have been allocated\n");
> +               dprintk(1, "%s: no buffers have been allocated\n", __func__);
>                 return -EINVAL;
>         }
>         if (q->num_buffers < q->min_buffers_needed) {
> -               dprintk(1, "streamon: need at least %u allocated buffers\n",
> -                               q->min_buffers_needed);
> +               dprintk(1, "%s: need at least %u allocated buffers\n",
> +                               __func__, q->min_buffers_needed);
>                 return -EINVAL;
>         }
>
> @@ -2134,7 +2140,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  {
>         if (q->fileio) {
> -               dprintk(1, "streamon: file io in progress\n");
> +               dprintk(1, "%s: file io in progress\n", __func__);
>                 return -EBUSY;
>         }
>         return vb2_internal_streamon(q, type);
> @@ -2144,7 +2150,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon);
>  static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>  {
>         if (type != q->type) {
> -               dprintk(1, "streamoff: invalid stream type\n");
> +               dprintk(1, "%s: invalid stream type\n", __func__);
>                 return -EINVAL;
>         }
>
> @@ -2181,7 +2187,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>  {
>         if (q->fileio) {
> -               dprintk(1, "streamoff: file io in progress\n");
> +               dprintk(1, "%s: file io in progress\n", __func__);
>                 return -EBUSY;
>         }
>         return vb2_internal_streamoff(q, type);
> @@ -2249,7 +2255,7 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
>         }
>
>         if (eb->type != q->type) {
> -               dprintk(1, "qbuf: invalid buffer type\n");
> +               dprintk(1, "%s: invalid buffer type\n", __func__);
>                 return -EINVAL;
>         }
>
> @@ -2863,7 +2869,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>                 fileio->b.index = index;
>                 fileio->b.bytesused = buf->pos;
>                 ret = vb2_internal_qbuf(q, &fileio->b);
> -               dprintk(5, "file io: vb2_dbuf result: %d\n", ret);
> +               dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
>                 if (ret)
>                         return ret;
>
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

Re: [REVIEW PATCH 04/11] vb2: use correct prefix

[Hans Verkuil]

On 04/07/2014 09:30 AM, Pawel Osciak wrote:
> The idea behind not using __func__ was that it was much more
> informative when debugging to see a "reqbufs" prefix instead of, for
> example "__verify_memory_type". But since some of the functions are
> shared across multiple ioctl impls now (e.g. __verify_memory_type is
> used by both reqbufs and createbufs), as much as I would prefer to
> keep this convention, it'd probably be too much to maintain.
> 
> But if we want to do this, we should move "__func__" to dprintk()
> definition, instead of adding it to the call sites.

Good idea. I'll do that.

Regards,

	Hans

> 
> On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Many dprintk's in vb2 use a hardcoded prefix with the function name. In
>> many cases that is now outdated. Replace prefixes by the function name using
>> __func__. At least now I know if I see a 'qbuf:' prefix whether that refers
>> to the mmap, userptr or dmabuf variant.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 102 ++++++++++++++++---------------
>>  1 file changed, 54 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 83e78e9..71be247 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -371,7 +371,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>>                 if (q->bufs[buffer] == NULL)
>>                         continue;
>>                 if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
>> -                       dprintk(1, "reqbufs: preparing buffers, cannot free\n");
>> +                       dprintk(1, "%s: preparing buffers, cannot free\n",
>> +                                       __func__);
>>                         return -EAGAIN;
>>                 }
>>         }
>> @@ -656,12 +657,12 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>         int ret;
>>
>>         if (b->type != q->type) {
>> -               dprintk(1, "querybuf: wrong buffer type\n");
>> +               dprintk(1, "%s: wrong buffer type\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>>         if (b->index >= q->num_buffers) {
>> -               dprintk(1, "querybuf: buffer index out of range\n");
>> +               dprintk(1, "%s: buffer index out of range\n", __func__);
>>                 return -EINVAL;
>>         }
>>         vb = q->bufs[b->index];
>> @@ -721,12 +722,12 @@ static int __verify_memory_type(struct vb2_queue *q,
>>  {
>>         if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR &&
>>             memory != V4L2_MEMORY_DMABUF) {
>> -               dprintk(1, "reqbufs: unsupported memory type\n");
>> +               dprintk(1, "%s: unsupported memory type\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>>         if (type != q->type) {
>> -               dprintk(1, "reqbufs: requested type is incorrect\n");
>> +               dprintk(1, "%s: requested type is incorrect\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -735,17 +736,20 @@ static int __verify_memory_type(struct vb2_queue *q,
>>          * are available.
>>          */
>>         if (memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
>> -               dprintk(1, "reqbufs: MMAP for current setup unsupported\n");
>> +               dprintk(1, "%s: MMAP for current setup unsupported\n",
>> +                               __func__);
>>                 return -EINVAL;
>>         }
>>
>>         if (memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
>> -               dprintk(1, "reqbufs: USERPTR for current setup unsupported\n");
>> +               dprintk(1, "%s: USERPTR for current setup unsupported\n",
>> +                               __func__);
>>                 return -EINVAL;
>>         }
>>
>>         if (memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
>> -               dprintk(1, "reqbufs: DMABUF for current setup unsupported\n");
>> +               dprintk(1, "%s: DMABUF for current setup unsupported\n",
>> +                               __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -755,7 +759,7 @@ static int __verify_memory_type(struct vb2_queue *q,
>>          * do the memory and type validation.
>>          */
>>         if (q->fileio) {
>> -               dprintk(1, "reqbufs: file io in progress\n");
>> +               dprintk(1, "%s: file io in progress\n", __func__);
>>                 return -EBUSY;
>>         }
>>         return 0;
>> @@ -790,7 +794,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>>         int ret;
>>
>>         if (q->streaming) {
>> -               dprintk(1, "reqbufs: streaming active\n");
>> +               dprintk(1, "%s: streaming active\n", __func__);
>>                 return -EBUSY;
>>         }
>>
>> @@ -800,7 +804,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>>                  * are not in use and can be freed.
>>                  */
>>                 if (q->memory == V4L2_MEMORY_MMAP && __buffers_in_use(q)) {
>> -                       dprintk(1, "reqbufs: memory in use, cannot free\n");
>> +                       dprintk(1, "%s: memory in use, cannot free\n", __func__);
>>                         return -EBUSY;
>>                 }
>>
>> @@ -1272,15 +1276,14 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                     && vb->v4l2_planes[plane].length == planes[plane].length)
>>                         continue;
>>
>> -               dprintk(3, "qbuf: userspace address for plane %d changed, "
>> -                               "reacquiring memory\n", plane);
>> +               dprintk(3, "%s: userspace address for plane %d changed, reacquiring memory\n",
>> +                       __func__, plane);
>>
>>                 /* Check if the provided plane buffer is large enough */
>>                 if (planes[plane].length < q->plane_sizes[plane]) {
>> -                       dprintk(1, "qbuf: provided buffer size %u is less than "
>> -                                               "setup size %u for plane %d\n",
>> -                                               planes[plane].length,
>> -                                               q->plane_sizes[plane], plane);
>> +                       dprintk(1, "%s: provided buffer size %u is less than setup size %u for plane %d\n",
>> +                               __func__, planes[plane].length,
>> +                               q->plane_sizes[plane], plane);
>>                         ret = -EINVAL;
>>                         goto err;
>>                 }
>> @@ -1302,8 +1305,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                                       planes[plane].m.userptr,
>>                                       planes[plane].length, write);
>>                 if (IS_ERR_OR_NULL(mem_priv)) {
>> -                       dprintk(1, "qbuf: failed acquiring userspace "
>> -                                               "memory for plane %d\n", plane);
>> +                       dprintk(1, "%s: failed acquiring userspace memory for plane %d\n",
>> +                               __func__, plane);
>>                         fail_memop(vb, get_userptr);
>>                         ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
>>                         goto err;
>> @@ -1326,7 +1329,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                  */
>>                 ret = call_vb_qop(vb, buf_init, vb);
>>                 if (ret) {
>> -                       dprintk(1, "qbuf: buffer initialization failed\n");
>> +                       dprintk(1, "%s: buffer initialization failed\n",
>> +                               __func__);
>>                         fail_vb_qop(vb, buf_init);
>>                         goto err;
>>                 }
>> @@ -1334,7 +1338,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>
>>         ret = call_vb_qop(vb, buf_prepare, vb);
>>         if (ret) {
>> -               dprintk(1, "qbuf: buffer preparation failed\n");
>> +               dprintk(1, "%s: buffer preparation failed\n", __func__);
>>                 fail_vb_qop(vb, buf_prepare);
>>                 call_vb_qop(vb, buf_cleanup, vb);
>>                 goto err;
>> @@ -1388,8 +1392,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                 struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>>
>>                 if (IS_ERR_OR_NULL(dbuf)) {
>> -                       dprintk(1, "qbuf: invalid dmabuf fd for plane %d\n",
>> -                               plane);
>> +                       dprintk(1, "%s: invalid dmabuf fd for plane %d\n",
>> +                               __func__, plane);
>>                         ret = -EINVAL;
>>                         goto err;
>>                 }
>> @@ -1399,8 +1403,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                         planes[plane].length = dbuf->size;
>>
>>                 if (planes[plane].length < q->plane_sizes[plane]) {
>> -                       dprintk(1, "qbuf: invalid dmabuf length for plane %d\n",
>> -                               plane);
>> +                       dprintk(1, "%s: invalid dmabuf length for plane %d\n",
>> +                               __func__, plane);
>>                         ret = -EINVAL;
>>                         goto err;
>>                 }
>> @@ -1412,7 +1416,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                         continue;
>>                 }
>>
>> -               dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
>> +               dprintk(1, "%s: buffer for plane %d changed\n",
>> +                       __func__, plane);
>>
>>                 if (!reacquired) {
>>                         reacquired = true;
>> @@ -1427,7 +1432,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                 mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
>>                         dbuf, planes[plane].length, write);
>>                 if (IS_ERR(mem_priv)) {
>> -                       dprintk(1, "qbuf: failed to attach dmabuf\n");
>> +                       dprintk(1, "%s: failed to attach dmabuf\n", __func__);
>>                         fail_memop(vb, attach_dmabuf);
>>                         ret = PTR_ERR(mem_priv);
>>                         dma_buf_put(dbuf);
>> @@ -1445,8 +1450,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>         for (plane = 0; plane < vb->num_planes; ++plane) {
>>                 ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>>                 if (ret) {
>> -                       dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
>> -                               plane);
>> +                       dprintk(1, "%s: failed to map dmabuf for plane %d\n",
>> +                               __func__, plane);
>>                         fail_memop(vb, map_dmabuf);
>>                         goto err;
>>                 }
>> @@ -1467,7 +1472,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>                  */
>>                 ret = call_vb_qop(vb, buf_init, vb);
>>                 if (ret) {
>> -                       dprintk(1, "qbuf: buffer initialization failed\n");
>> +                       dprintk(1, "%s: buffer initialization failed\n",
>> +                               __func__);
>>                         fail_vb_qop(vb, buf_init);
>>                         goto err;
>>                 }
>> @@ -1475,7 +1481,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>
>>         ret = call_vb_qop(vb, buf_prepare, vb);
>>         if (ret) {
>> -               dprintk(1, "qbuf: buffer preparation failed\n");
>> +               dprintk(1, "%s: buffer preparation failed\n", __func__);
>>                 fail_vb_qop(vb, buf_prepare);
>>                 call_vb_qop(vb, buf_cleanup, vb);
>>                 goto err;
>> @@ -1671,7 +1677,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>                 return 0;
>>
>>         fail_qop(q, start_streaming);
>> -       dprintk(1, "qbuf: driver refused to start streaming\n");
>> +       dprintk(1, "%s: driver refused to start streaming\n", __func__);
>>         if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
>>                 unsigned i;
>>
>> @@ -1709,7 +1715,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>         case VB2_BUF_STATE_PREPARED:
>>                 break;
>>         case VB2_BUF_STATE_PREPARING:
>> -               dprintk(1, "qbuf: buffer still being prepared\n");
>> +               dprintk(1, "%s: buffer still being prepared\n", __func__);
>>                 return -EINVAL;
>>         default:
>>                 dprintk(1, "%s(): invalid buffer state %d\n", __func__,
>> @@ -1945,7 +1951,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>>         int ret;
>>
>>         if (b->type != q->type) {
>> -               dprintk(1, "dqbuf: invalid buffer type\n");
>> +               dprintk(1, "%s: invalid buffer type\n", __func__);
>>                 return -EINVAL;
>>         }
>>         ret = __vb2_get_done_vb(q, &vb, b, nonblocking);
>> @@ -1954,13 +1960,13 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>>
>>         switch (vb->state) {
>>         case VB2_BUF_STATE_DONE:
>> -               dprintk(3, "dqbuf: Returning done buffer\n");
>> +               dprintk(3, "%s: Returning done buffer\n", __func__);
>>                 break;
>>         case VB2_BUF_STATE_ERROR:
>> -               dprintk(3, "dqbuf: Returning done buffer with errors\n");
>> +               dprintk(3, "%s: Returning done buffer with errors\n", __func__);
>>                 break;
>>         default:
>> -               dprintk(1, "dqbuf: Invalid buffer state\n");
>> +               dprintk(1, "%s: Invalid buffer state\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -2004,7 +2010,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>>  int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
>>  {
>>         if (q->fileio) {
>> -               dprintk(1, "dqbuf: file io in progress\n");
>> +               dprintk(1, "%s: file io in progress\n", __func__);
>>                 return -EBUSY;
>>         }
>>         return vb2_internal_dqbuf(q, b, nonblocking);
>> @@ -2076,7 +2082,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>>         int ret;
>>
>>         if (type != q->type) {
>> -               dprintk(1, "streamon: invalid stream type\n");
>> +               dprintk(1, "%s: invalid stream type\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -2086,17 +2092,17 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>>         }
>>
>>         if (!q->num_buffers) {
>> -               dprintk(1, "streamon: no buffers have been allocated\n");
>> +               dprintk(1, "%s: no buffers have been allocated\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>>         if (!q->num_buffers) {
>> -               dprintk(1, "streamon: no buffers have been allocated\n");
>> +               dprintk(1, "%s: no buffers have been allocated\n", __func__);
>>                 return -EINVAL;
>>         }
>>         if (q->num_buffers < q->min_buffers_needed) {
>> -               dprintk(1, "streamon: need at least %u allocated buffers\n",
>> -                               q->min_buffers_needed);
>> +               dprintk(1, "%s: need at least %u allocated buffers\n",
>> +                               __func__, q->min_buffers_needed);
>>                 return -EINVAL;
>>         }
>>
>> @@ -2134,7 +2140,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>>  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>>  {
>>         if (q->fileio) {
>> -               dprintk(1, "streamon: file io in progress\n");
>> +               dprintk(1, "%s: file io in progress\n", __func__);
>>                 return -EBUSY;
>>         }
>>         return vb2_internal_streamon(q, type);
>> @@ -2144,7 +2150,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon);
>>  static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>>  {
>>         if (type != q->type) {
>> -               dprintk(1, "streamoff: invalid stream type\n");
>> +               dprintk(1, "%s: invalid stream type\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -2181,7 +2187,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>>  {
>>         if (q->fileio) {
>> -               dprintk(1, "streamoff: file io in progress\n");
>> +               dprintk(1, "%s: file io in progress\n", __func__);
>>                 return -EBUSY;
>>         }
>>         return vb2_internal_streamoff(q, type);
>> @@ -2249,7 +2255,7 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
>>         }
>>
>>         if (eb->type != q->type) {
>> -               dprintk(1, "qbuf: invalid buffer type\n");
>> +               dprintk(1, "%s: invalid buffer type\n", __func__);
>>                 return -EINVAL;
>>         }
>>
>> @@ -2863,7 +2869,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>>                 fileio->b.index = index;
>>                 fileio->b.bytesused = buf->pos;
>>                 ret = vb2_internal_qbuf(q, &fileio->b);
>> -               dprintk(5, "file io: vb2_dbuf result: %d\n", ret);
>> +               dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
>>                 if (ret)
>>                         return ret;
>>
>> --
>> 1.9.0
>>
> 
> 
> 

[REVIEW PATCH 05/11] vb2: move __qbuf_mmap before __qbuf_userptr

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

__qbuf_mmap was sort of hidden in between the much larger __qbuf_userptr
and __qbuf_dmabuf functions. Move it before __qbuf_userptr which is
also conform the usual order these memory models are implemented: first
mmap, then userptr, then dmabuf.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 71be247..e38b45e 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1254,6 +1254,20 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 }
 
 /**
+ * __qbuf_mmap() - handle qbuf of an MMAP buffer
+ */
+static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+{
+	int ret;
+
+	__fill_vb2_buffer(vb, b, vb->v4l2_planes);
+	ret = call_vb_qop(vb, buf_prepare, vb);
+	if (ret)
+		fail_vb_qop(vb, buf_prepare);
+	return ret;
+}
+
+/**
  * __qbuf_userptr() - handle qbuf of a USERPTR buffer
  */
 static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
@@ -1359,20 +1373,6 @@ err:
 }
 
 /**
- * __qbuf_mmap() - handle qbuf of an MMAP buffer
- */
-static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
-{
-	int ret;
-
-	__fill_vb2_buffer(vb, b, vb->v4l2_planes);
-	ret = call_vb_qop(vb, buf_prepare, vb);
-	if (ret)
-		fail_vb_qop(vb, buf_prepare);
-	return ret;
-}
-
-/**
  * __qbuf_dmabuf() - handle qbuf of a DMABUF buffer
  */
 static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
-- 
1.9.0

Re: [REVIEW PATCH 05/11] vb2: move __qbuf_mmap before __qbuf_userptr

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> __qbuf_mmap was sort of hidden in between the much larger __qbuf_userptr
> and __qbuf_dmabuf functions. Move it before __qbuf_userptr which is
> also conform the usual order these memory models are implemented: first
> mmap, then userptr, then dmabuf.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 71be247..e38b45e 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1254,6 +1254,20 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  }
>
>  /**
> + * __qbuf_mmap() - handle qbuf of an MMAP buffer
> + */
> +static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> +{
> +       int ret;
> +
> +       __fill_vb2_buffer(vb, b, vb->v4l2_planes);
> +       ret = call_vb_qop(vb, buf_prepare, vb);
> +       if (ret)
> +               fail_vb_qop(vb, buf_prepare);
> +       return ret;
> +}
> +
> +/**
>   * __qbuf_userptr() - handle qbuf of a USERPTR buffer
>   */
>  static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> @@ -1359,20 +1373,6 @@ err:
>  }
>
>  /**
> - * __qbuf_mmap() - handle qbuf of an MMAP buffer
> - */
> -static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> -{
> -       int ret;
> -
> -       __fill_vb2_buffer(vb, b, vb->v4l2_planes);
> -       ret = call_vb_qop(vb, buf_prepare, vb);
> -       if (ret)
> -               fail_vb_qop(vb, buf_prepare);
> -       return ret;
> -}
> -
> -/**
>   * __qbuf_dmabuf() - handle qbuf of a DMABUF buffer
>   */
>  static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

[REVIEW PATCH 06/11] vb2: set timestamp when using write()

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When using write() to write data to an output video node the vb2 core
should set timestamps if V4L2_BUF_FLAG_TIMESTAMP_COPY is set. Nobody
else is able to provide this information with the write() operation.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index e38b45e..afd1268 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -22,6 +22,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-common.h>
 #include <media/videobuf2-core.h>
 
 static int debug;
@@ -2767,6 +2768,9 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 {
 	struct vb2_fileio_data *fileio;
 	struct vb2_fileio_buf *buf;
+	bool set_timestamp = !read &&
+		(q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) ==
+		V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	int ret, index;
 
 	dprintk(3, "file io: mode %s, offset %ld, count %zd, %sblocking\n",
@@ -2868,6 +2872,8 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->b.memory = q->memory;
 		fileio->b.index = index;
 		fileio->b.bytesused = buf->pos;
+		if (set_timestamp)
+			v4l2_get_timestamp(&fileio->b.timestamp);
 		ret = vb2_internal_qbuf(q, &fileio->b);
 		dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
 		if (ret)
-- 
1.9.0

Re: [REVIEW PATCH 06/11] vb2: set timestamp when using write()

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> When using write() to write data to an output video node the vb2 core
> should set timestamps if V4L2_BUF_FLAG_TIMESTAMP_COPY is set. Nobody

I'm confused. Shouldn't we be saving the existing timestamp from the buffer if
V4L2_BUF_FLAG_TIMESTAMP_COPY is true, instead of getting it from
v4l2_get_timestamp()?

> else is able to provide this information with the write() operation.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index e38b45e..afd1268 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -22,6 +22,7 @@
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-common.h>
>  #include <media/videobuf2-core.h>
>
>  static int debug;
> @@ -2767,6 +2768,9 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>  {
>         struct vb2_fileio_data *fileio;
>         struct vb2_fileio_buf *buf;
> +       bool set_timestamp = !read &&
> +               (q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) ==
> +               V4L2_BUF_FLAG_TIMESTAMP_COPY;
>         int ret, index;
>
>         dprintk(3, "file io: mode %s, offset %ld, count %zd, %sblocking\n",
> @@ -2868,6 +2872,8 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>                 fileio->b.memory = q->memory;
>                 fileio->b.index = index;
>                 fileio->b.bytesused = buf->pos;
> +               if (set_timestamp)
> +                       v4l2_get_timestamp(&fileio->b.timestamp);
>                 ret = vb2_internal_qbuf(q, &fileio->b);
>                 dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
>                 if (ret)
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

Re: [REVIEW PATCH 06/11] vb2: set timestamp when using write()

[Hans Verkuil]

On 04/07/2014 10:32 AM, Pawel Osciak wrote:
> On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> When using write() to write data to an output video node the vb2 core
>> should set timestamps if V4L2_BUF_FLAG_TIMESTAMP_COPY is set. Nobody
> 
> I'm confused. Shouldn't we be saving the existing timestamp from the buffer if
> V4L2_BUF_FLAG_TIMESTAMP_COPY is true, instead of getting it from
> v4l2_get_timestamp()?

When using the write() file operation the application has no way of setting the
timestamp. So it is uninitialized and the reader on the other side receives an
uninitialized (or 0, I'm not sure) timestamp. So __vb2_perform_fileio() has to
fill in a valid timestamp instead.

It's a corner case.

Regards,

	Hans

> 
>> else is able to provide this information with the write() operation.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index e38b45e..afd1268 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -22,6 +22,7 @@
>>  #include <media/v4l2-dev.h>
>>  #include <media/v4l2-fh.h>
>>  #include <media/v4l2-event.h>
>> +#include <media/v4l2-common.h>
>>  #include <media/videobuf2-core.h>
>>
>>  static int debug;
>> @@ -2767,6 +2768,9 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>>  {
>>         struct vb2_fileio_data *fileio;
>>         struct vb2_fileio_buf *buf;
>> +       bool set_timestamp = !read &&
>> +               (q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) ==
>> +               V4L2_BUF_FLAG_TIMESTAMP_COPY;
>>         int ret, index;
>>
>>         dprintk(3, "file io: mode %s, offset %ld, count %zd, %sblocking\n",
>> @@ -2868,6 +2872,8 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>>                 fileio->b.memory = q->memory;
>>                 fileio->b.index = index;
>>                 fileio->b.bytesused = buf->pos;
>> +               if (set_timestamp)
>> +                       v4l2_get_timestamp(&fileio->b.timestamp);
>>                 ret = vb2_internal_qbuf(q, &fileio->b);
>>                 dprintk(5, "file io: vb2_internal_qbuf result: %d\n", ret);
>>                 if (ret)
>> --
>> 1.9.0
>>
> 
> 
> 

[REVIEW PATCH 07/11] vb2: reject output buffers with V4L2_FIELD_ALTERNATE

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This is not allowed by the spec and does in fact not make any sense.
Return -EINVAL if this is the case.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index afd1268..8984187 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1526,6 +1526,15 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 			__func__, ret);
 		return ret;
 	}
+	if (V4L2_TYPE_IS_OUTPUT(q->type) && b->field == V4L2_FIELD_ALTERNATE) {
+		/*
+		 * If field is ALTERNATE, then we return an error.
+		 * If the format's field is ALTERNATE, then the buffer's field
+		 * should be either TOP or BOTTOM, but using ALTERNATE here as
+		 * well makes no sense.
+		 */
+		return -EINVAL;
+	}
 
 	vb->state = VB2_BUF_STATE_PREPARING;
 	vb->v4l2_buf.timestamp.tv_sec = 0;
-- 
1.9.0

Re: [REVIEW PATCH 07/11] vb2: reject output buffers with V4L2_FIELD_ALTERNATE

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> This is not allowed by the spec and does in fact not make any sense.
> Return -EINVAL if this is the case.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index afd1268..8984187 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1526,6 +1526,15 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                         __func__, ret);
>                 return ret;
>         }
> +       if (V4L2_TYPE_IS_OUTPUT(q->type) && b->field == V4L2_FIELD_ALTERNATE) {

Checking for field first would probably eliminate the additional
OUTPUT check most of the time.
I'd swap them.

> +               /*
> +                * If field is ALTERNATE, then we return an error.

I'd drop this line, doesn't really add anything.

> +                * If the format's field is ALTERNATE, then the buffer's field
> +                * should be either TOP or BOTTOM, but using ALTERNATE here as
> +                * well makes no sense.

This doesn't really explain why this is an error and is confusing,
since we don't check TOP/BOTTOM
anyway. I think it would be better to say why ALTERNATE doesn't make
sense instead.

> +                */
> +               return -EINVAL;
> +       }
>
>         vb->state = VB2_BUF_STATE_PREPARING;
>         vb->v4l2_buf.timestamp.tv_sec = 0;
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

[REVIEW PATCH 08/11] vb2: simplify a confusing condition.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

q->start_streaming_called is always true, so the WARN_ON check against
it being false can be dropped.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 8984187..2ae316b 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1099,9 +1099,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	if (!q->start_streaming_called) {
 		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
 			state = VB2_BUF_STATE_QUEUED;
-	} else if (!WARN_ON(!q->start_streaming_called)) {
-		if (WARN_ON(state != VB2_BUF_STATE_DONE &&
-			    state != VB2_BUF_STATE_ERROR))
+	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
+			   state != VB2_BUF_STATE_ERROR)) {
 			state = VB2_BUF_STATE_ERROR;
 	}
 
-- 
1.9.0

Re: [REVIEW PATCH 08/11] vb2: simplify a confusing condition.

[Pawel Osciak]

On Tue, Mar 11, 2014 at 6:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> q->start_streaming_called is always true, so the WARN_ON check against
> it being false can be dropped.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 8984187..2ae316b 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1099,9 +1099,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>         if (!q->start_streaming_called) {
>                 if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
>                         state = VB2_BUF_STATE_QUEUED;
> -       } else if (!WARN_ON(!q->start_streaming_called)) {
> -               if (WARN_ON(state != VB2_BUF_STATE_DONE &&
> -                           state != VB2_BUF_STATE_ERROR))
> +       } else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
> +                          state != VB2_BUF_STATE_ERROR)) {
>                         state = VB2_BUF_STATE_ERROR;
>         }
>
> --
> 1.9.0
>



-- 
Best regards,
Pawel Osciak

[REVIEW PATCH 09/11] vb2: add vb2_fileio_is_active and check it more often

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Added a vb2_fileio_is_active inline function that returns true if fileio
is in progress. Check for this too in mmap() (you don't want apps mmap()ing
buffers used by fileio) and expbuf() (same reason).

In addition drivers should be able to check for this in queue_setup() to
return an error if an attempt is made to read() or write() with
V4L2_FIELD_ALTERNATE being configured. This is illegal (there is no way
to pass the TOP/BOTTOM information around using file I/O).

However, in order to be able to check for this the init_fileio function
needs to set q->fileio early on, before the buffers are allocated. So switch
to using internal functions (__reqbufs, vb2_internal_qbuf and
vb2_internal_streamon) to skip the fileio check. Well, that's why the internal
functions were created...

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 39 ++++++++++++++++++++------------
 include/media/videobuf2-core.h           | 17 ++++++++++++++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2ae316b..f68a60f 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -759,7 +759,7 @@ static int __verify_memory_type(struct vb2_queue *q,
 	 * create_bufs is called with count == 0, but count == 0 should still
 	 * do the memory and type validation.
 	 */
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -1628,7 +1628,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 	struct vb2_buffer *vb;
 	int ret;
 
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s(): file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -1798,7 +1798,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
  */
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s(): file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -2018,7 +2018,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
  */
 int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 {
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -2148,7 +2148,7 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
  */
 int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -2195,7 +2195,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
  */
 int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 {
-	if (q->fileio) {
+	if (vb2_fileio_is_active(q)) {
 		dprintk(1, "%s: file io in progress\n", __func__);
 		return -EBUSY;
 	}
@@ -2280,6 +2280,11 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
 		return -EINVAL;
 	}
 
+	if (vb2_fileio_is_active(q)) {
+		dprintk(1, "expbuf: file io in progress\n");
+		return -EBUSY;
+	}
+
 	vb_plane = &vb->planes[eb->plane];
 
 	dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
@@ -2356,6 +2361,10 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
 			return -EINVAL;
 		}
 	}
+	if (vb2_fileio_is_active(q)) {
+		dprintk(1, "mmap: file io in progress\n");
+		return -EBUSY;
+	}
 
 	/*
 	 * Find the plane corresponding to the offset passed by userspace.
@@ -2467,7 +2476,7 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	/*
 	 * Start file I/O emulator only if streaming API has not been used yet.
 	 */
-	if (q->num_buffers == 0 && q->fileio == NULL) {
+	if (q->num_buffers == 0 && !vb2_fileio_is_active(q)) {
 		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ) &&
 				(req_events & (POLLIN | POLLRDNORM))) {
 			if (__vb2_init_fileio(q, 1))
@@ -2672,7 +2681,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	fileio->req.count = count;
 	fileio->req.memory = V4L2_MEMORY_MMAP;
 	fileio->req.type = q->type;
-	ret = vb2_reqbufs(q, &fileio->req);
+	q->fileio = fileio;
+	ret = __reqbufs(q, &fileio->req);
 	if (ret)
 		goto err_kfree;
 
@@ -2710,7 +2720,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 			b->type = q->type;
 			b->memory = q->memory;
 			b->index = i;
-			ret = vb2_qbuf(q, b);
+			ret = vb2_internal_qbuf(q, b);
 			if (ret)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
@@ -2726,19 +2736,18 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	/*
 	 * Start streaming.
 	 */
-	ret = vb2_streamon(q, q->type);
+	ret = vb2_internal_streamon(q, q->type);
 	if (ret)
 		goto err_reqbufs;
 
-	q->fileio = fileio;
-
 	return ret;
 
 err_reqbufs:
 	fileio->req.count = 0;
-	vb2_reqbufs(q, &fileio->req);
+	__reqbufs(q, &fileio->req);
 
 err_kfree:
+	q->fileio = NULL;
 	kfree(fileio);
 	return ret;
 }
@@ -2791,7 +2800,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	/*
 	 * Initialize emulator on first call.
 	 */
-	if (!q->fileio) {
+	if (!vb2_fileio_is_active(q)) {
 		ret = __vb2_init_fileio(q, read);
 		dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
 		if (ret)
@@ -3159,7 +3168,7 @@ unsigned int vb2_fop_poll(struct file *file, poll_table *wait)
 
 	/* Try to be smart: only lock if polling might start fileio,
 	   otherwise locking will only introduce unwanted delays. */
-	if (q->num_buffers == 0 && q->fileio == NULL) {
+	if (q->num_buffers == 0 && !vb2_fileio_is_active(q)) {
 		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ) &&
 				(req_events & (POLLIN | POLLRDNORM)))
 			must_lock = true;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 3b57851..af34ae0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -472,6 +472,23 @@ static inline bool vb2_is_streaming(struct vb2_queue *q)
 }
 
 /**
+ * vb2_fileio_is_active() - return true if fileio is active.
+ * @q:		videobuf queue
+ *
+ * This returns true if read() or write() is used to stream the data
+ * as opposed to stream I/O. This is almost never an important distinction,
+ * except in rare cases. One such case is that using read() or write() to
+ * stream a format using V4L2_FIELD_ALTERNATE is not allowed since there
+ * is no way you can pass the field information of each buffer to/from
+ * userspace. A driver that supports this field format should check for
+ * this in the queue_setup op and reject it if this function returns true.
+ */
+static inline bool vb2_fileio_is_active(struct vb2_queue *q)
+{
+	return q->fileio;
+}
+
+/**
  * vb2_is_busy() - return busy status of the queue
  * @q:		videobuf queue
  *
-- 
1.9.0

[REVIEW PATCH 10/11] vb2: set v4l2_buffer.bytesused to 0 for mp buffers

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The bytesused field of struct v4l2_buffer is not used for multiplanar
formats, so just zero it to prevent it from having some random value.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f68a60f..54a4150 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -583,6 +583,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		 * for it. The caller has already verified memory and size.
 		 */
 		b->length = vb->num_planes;
+		b->bytesused = 0;
 		memcpy(b->m.planes, vb->v4l2_planes,
 			b->length * sizeof(struct v4l2_plane));
 	} else {
-- 
1.9.0

Re: [REVIEW PATCH 10/11] vb2: set v4l2_buffer.bytesused to 0 for mp buffers

[Sakari Ailus]

Hi Hans,

Thanks for the set.

On Mon, Mar 10, 2014 at 10:20:57PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The bytesused field of struct v4l2_buffer is not used for multiplanar
> formats, so just zero it to prevent it from having some random value.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index f68a60f..54a4150 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -583,6 +583,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * for it. The caller has already verified memory and size.
>  		 */
>  		b->length = vb->num_planes;
> +		b->bytesused = 0;

I wonder if I'm missing something, but doesn't the value of the field come
from the v4l2_buf field of the vb2_buffer which is allocated using kzalloc()
in __vb2_queue_alloc(), and never changed afterwards?

>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [REVIEW PATCH 10/11] vb2: set v4l2_buffer.bytesused to 0 for mp buffers

[Hans Verkuil]

On 04/09/2014 07:21 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the set.
> 
> On Mon, Mar 10, 2014 at 10:20:57PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The bytesused field of struct v4l2_buffer is not used for multiplanar
>> formats, so just zero it to prevent it from having some random value.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index f68a60f..54a4150 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -583,6 +583,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>>  		 * for it. The caller has already verified memory and size.
>>  		 */
>>  		b->length = vb->num_planes;
>> +		b->bytesused = 0;
> 
> I wonder if I'm missing something, but doesn't the value of the field come
> from the v4l2_buf field of the vb2_buffer which is allocated using kzalloc()
> in __vb2_queue_alloc(), and never changed afterwards?

You are right, this isn't necessary. I've dropped this patch.

Thanks!

	Hans

> 
>>  		memcpy(b->m.planes, vb->v4l2_planes,
>>  			b->length * sizeof(struct v4l2_plane));
>>  	} else {
> 

[REVIEW PATCH 11/11] vb2: allow read/write as long as the format is single planar

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

It was impossible to read() or write() a frame if the queue type was multiplanar.
Even if the current format is single planar. Change this to just check whether
the number of planes is 1 or more.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 54a4150..8faf1ef 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2622,6 +2622,7 @@ struct vb2_fileio_buf {
  */
 struct vb2_fileio_data {
 	struct v4l2_requestbuffers req;
+	struct v4l2_plane p;
 	struct v4l2_buffer b;
 	struct vb2_fileio_buf bufs[VIDEO_MAX_FRAME];
 	unsigned int cur_index;
@@ -2712,13 +2713,21 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 * Read mode requires pre queuing of all buffers.
 	 */
 	if (read) {
+		bool is_multiplanar = V4L2_TYPE_IS_MULTIPLANAR(q->type);
+
 		/*
 		 * Queue all buffers.
 		 */
 		for (i = 0; i < q->num_buffers; i++) {
 			struct v4l2_buffer *b = &fileio->b;
+
 			memset(b, 0, sizeof(*b));
 			b->type = q->type;
+			if (is_multiplanar) {
+				memset(&fileio->p, 0, sizeof(fileio->p));
+				b->m.planes = &fileio->p;
+				b->length = 1;
+			}
 			b->memory = q->memory;
 			b->index = i;
 			ret = vb2_internal_qbuf(q, b);
@@ -2786,6 +2795,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 {
 	struct vb2_fileio_data *fileio;
 	struct vb2_fileio_buf *buf;
+	bool is_multiplanar = V4L2_TYPE_IS_MULTIPLANAR(q->type);
 	bool set_timestamp = !read &&
 		(q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) ==
 		V4L2_BUF_FLAG_TIMESTAMP_COPY;
@@ -2820,6 +2830,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		memset(&fileio->b, 0, sizeof(fileio->b));
 		fileio->b.type = q->type;
 		fileio->b.memory = q->memory;
+		if (is_multiplanar) {
+			memset(&fileio->p, 0, sizeof(fileio->p));
+			fileio->b.m.planes = &fileio->p;
+			fileio->b.length = 1;
+		}
 		ret = vb2_internal_dqbuf(q, &fileio->b, nonblock);
 		dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
 		if (ret)
@@ -2890,6 +2905,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->b.memory = q->memory;
 		fileio->b.index = index;
 		fileio->b.bytesused = buf->pos;
+		if (is_multiplanar) {
+			memset(&fileio->p, 0, sizeof(fileio->p));
+			fileio->p.bytesused = buf->pos;
+			fileio->b.m.planes = &fileio->p;
+			fileio->b.length = 1;
+		}
 		if (set_timestamp)
 			v4l2_get_timestamp(&fileio->b.timestamp);
 		ret = vb2_internal_qbuf(q, &fileio->b);
-- 
1.9.0

Re: vb2: various small fixes/improvements

[Hans Verkuil]

Can someone review this? That would be much appreciated!

Regards,

	Hans

On 03/10/2014 10:20 PM, Hans Verkuil wrote:
> This patch series contains a list of various vb2 fixes and improvements.
> 
> These patches were originally part of this RFC patch series:
> 
> http://www.spinics.net/lists/linux-media/msg73391.html
> 
> They are now rebased and reordered a bit. It's little stuff for the
> most part, although the first patch touches on more drivers since it
> changes the return type of stop_streaming to void. The return value was
> always ignored by vb2 and you really cannot do anything sensible with it.
> In general resource allocations can return an error, but freeing up resources
> should not. That should always succeed.
> 
> Regards,
> 
> 	Hans
> 
> --
> 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: vb2: various small fixes/improvements

[Sakari Ailus]

On Mon, Mar 10, 2014 at 10:20:47PM +0100, Hans Verkuil wrote:
> This patch series contains a list of various vb2 fixes and improvements.
> 
> These patches were originally part of this RFC patch series:
> 
> http://www.spinics.net/lists/linux-media/msg73391.html
> 
> They are now rebased and reordered a bit. It's little stuff for the
> most part, although the first patch touches on more drivers since it
> changes the return type of stop_streaming to void. The return value was
> always ignored by vb2 and you really cannot do anything sensible with it.
> In general resource allocations can return an error, but freeing up resources
> should not. That should always succeed.

For patches 1--10, with Pawel's comments addressed:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk