[PATCHv3 0/7] media: use vb2_video_unregister_device()

[PATCHv3 0/7] media: use vb2_video_unregister_device()

[Hans Verkuil]

This series supersedes this patch:

https://patchwork.linuxtv.org/project/linux-media/patch/7e7e9841-7f3a-468b-01c8-5921e5c14df8@xs4all.nl/

This series checks drivers that use vb2_queue_release() incorrectly
and fixes them, and calls vb2_video_unregister_device() as well if the
driver set vdev->queue.

There are a lot more drivers that set vdev->queue and can probably
benefit from using vb2_video_unregister_device(), but that's something
for a future series.

The main focus of this series is to check for incorrect use of
vb2_queue_release().

Changes since v2: add patches 2-7.

Regards,

	Hans

Hans Verkuil (7):
  videobuf2-v4l2.c: add vb2_video_unregister_device helper function
  qcom/camss: use vb2_video_unregister_device()
  media/pci: use vb2_video_unregister_device()
  media/platform: drop vb2_queue_release()
  media/usb: use vb2_video_unregister_device()
  vimc: use vb2_video_unregister_device()
  staging/media: drop vb2_queue_release()

 .../media/common/videobuf2/videobuf2-v4l2.c   | 51 ++++++++++++++++---
 drivers/media/pci/dt3155/dt3155.c             |  3 +-
 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  9 ++--
 drivers/media/pci/saa7134/saa7134-core.c      |  6 +--
 drivers/media/pci/saa7134/saa7134-empress.c   |  3 +-
 drivers/media/pci/saa7134/saa7134-go7007.c    |  2 +-
 drivers/media/pci/saa7134/saa7134-video.c     |  2 -
 drivers/media/pci/sta2x11/sta2x11_vip.c       |  6 +--
 drivers/media/pci/tw5864/tw5864-video.c       |  4 +-
 drivers/media/platform/aspeed-video.c         |  5 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec.c      |  4 +-
 drivers/media/platform/qcom/camss/camss-vfe.c |  8 ---
 drivers/media/platform/qcom/camss/camss-vfe.h |  2 -
 .../media/platform/qcom/camss/camss-video.c   | 12 +----
 .../media/platform/qcom/camss/camss-video.h   |  2 -
 drivers/media/platform/qcom/camss/camss.c     |  5 --
 drivers/media/platform/qcom/venus/vdec.c      |  8 +--
 drivers/media/platform/qcom/venus/venc.c      |  8 +--
 .../platform/sunxi/sun4i-csi/sun4i_csi.c      |  1 +
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      |  6 +--
 .../platform/sunxi/sun6i-csi/sun6i_video.c    |  7 +--
 .../media/test-drivers/vimc/vimc-capture.c    |  7 +--
 drivers/media/test-drivers/vivid/vivid-core.c | 32 ++++++------
 drivers/media/usb/au0828/au0828-video.c       | 12 ++---
 drivers/media/usb/dvb-usb/cxusb-analog.c      | 13 ++---
 drivers/media/usb/usbtv/usbtv-video.c         |  4 +-
 drivers/staging/media/meson/vdec/vdec.c       |  8 +--
 drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
 drivers/staging/media/rkisp1/rkisp1-params.c  |  7 +--
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  6 +--
 drivers/staging/media/tegra-video/vi.c        |  8 +--
 include/media/videobuf2-v4l2.h                | 17 +++++++
 32 files changed, 118 insertions(+), 152 deletions(-)

-- 
2.27.0

[PATCHv3 1/7] videobuf2-v4l2.c: add vb2_video_unregister_device helper function

[Hans Verkuil]

If a driver calls (_)vb2_fop_release(), then such a driver should also
call vb2_video_unregister_device() instead of video_unregister_device().
This helper will all vb2_queue_release() if a filehandle is marked as
owner of the queue. This ensures that at unregister time any streaming
is cancelled and all buffers are returned to userspace.

This is very useful for complex drivers since this stops all streaming
in all subdevs in the pipeline controlled by this video device. Otherwise
this would happen until the owner filehandle is closed, which can be quite
some time later.

Bonus points for ordering the includes :-)

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Helen Koike <helen.koike@collabora.com>
Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Alexandre Courbot <acourbot@chromium.org>
Cc: Tiffany Lin <tiffany.lin@mediatek.com>
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 51 ++++++++++++++++---
 include/media/videobuf2-v4l2.h                | 17 +++++++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 57aa183bd198..576057132d3e 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -14,21 +14,22 @@
  * the Free Software Foundation.
  */
 
+#include <linux/device.h>
 #include <linux/err.h>
+#include <linux/freezer.h>
 #include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/kthread.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 #include <linux/poll.h>
-#include <linux/slab.h>
 #include <linux/sched.h>
-#include <linux/freezer.h>
-#include <linux/kthread.h>
+#include <linux/slab.h>
 
+#include <media/v4l2-common.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
-#include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
-#include <media/v4l2-common.h>
+#include <media/v4l2-fh.h>
 
 #include <media/videobuf2-v4l2.h>
 
@@ -1234,6 +1235,44 @@ unsigned long vb2_fop_get_unmapped_area(struct file *file, unsigned long addr,
 EXPORT_SYMBOL_GPL(vb2_fop_get_unmapped_area);
 #endif
 
+void vb2_video_unregister_device(struct video_device *vdev)
+{
+	/* Check if vdev was ever registered at all */
+	if (!vdev || !video_is_registered(vdev))
+		return;
+
+	/*
+	 * Calling this function only makes sense if vdev->queue is set.
+	 * If it is NULL, then just call video_unregister_device() instead.
+	 */
+	WARN_ON(!vdev->queue);
+
+	/*
+	 * Take a reference to the device since video_unregister_device()
+	 * calls device_unregister(), but we don't want that to release
+	 * the device since we want to clean up the queue first.
+	 */
+	get_device(&vdev->dev);
+	video_unregister_device(vdev);
+	if (vdev->queue && vdev->queue->owner) {
+		struct mutex *lock = vdev->queue->lock ?
+			vdev->queue->lock : vdev->lock;
+
+		if (lock)
+			mutex_lock(lock);
+		vb2_queue_release(vdev->queue);
+		vdev->queue->owner = NULL;
+		if (lock)
+			mutex_unlock(lock);
+	}
+	/*
+	 * Now we put the device, and in most cases this will release
+	 * everything.
+	 */
+	put_device(&vdev->dev);
+}
+EXPORT_SYMBOL_GPL(vb2_video_unregister_device);
+
 /* vb2_ops helpers. Only use if vq->lock is non-NULL. */
 
 void vb2_ops_wait_prepare(struct vb2_queue *vq)
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index b7b5a9cb5a28..c203047eb834 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -23,6 +23,8 @@
 #error VB2_MAX_PLANES != VIDEO_MAX_PLANES
 #endif
 
+struct video_device;
+
 /**
  * struct vb2_v4l2_buffer - video buffer information for v4l2.
  *
@@ -319,6 +321,21 @@ unsigned long vb2_fop_get_unmapped_area(struct file *file, unsigned long addr,
 		unsigned long len, unsigned long pgoff, unsigned long flags);
 #endif
 
+/**
+ * vb2_video_unregister_device - unregister the video device and release queue
+ *
+ * @vdev: pointer to &struct video_device
+ *
+ * If the driver uses vb2_fop_release()/_vb2_fop_release(), then it should use
+ * vb2_video_unregister_device() instead of video_unregister_device().
+ *
+ * This function will call video_unregister_device() and then release the
+ * vb2_queue if streaming is in progress. This will stop streaming and
+ * this will simplify the unbind sequence since after this call all subdevs
+ * will have stopped streaming as well.
+ */
+void vb2_video_unregister_device(struct video_device *vdev);
+
 /**
  * vb2_ops_wait_prepare - helper function to lock a struct &vb2_queue
  *
-- 
2.27.0

[PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Hans Verkuil]

Use vb2_video_unregister_device() to automatically stop streaming
at unregister time.

This avoids the use of vb2_queue_release() which should not be
called by drivers that set vdev->queue.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
 drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
 drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
 drivers/media/platform/qcom/camss/camss-video.h |  2 --
 drivers/media/platform/qcom/camss/camss.c       |  5 -----
 5 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index fc31c2c169cd..b7d2293a5004 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
 	.flush_buffers = vfe_flush_buffers,
 };
 
-void msm_vfe_stop_streaming(struct vfe_device *vfe)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
-		msm_video_stop_streaming(&vfe->line[i].video_out);
-}
-
 /*
  * msm_vfe_register_entities - Register subdev node for VFE module
  * @vfe: VFE device
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 0d10071ae881..a90b0d2cc6de 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
 void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
 void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
 
-void msm_vfe_stop_streaming(struct vfe_device *vfe);
-
 extern const struct vfe_hw_ops vfe_ops_4_1;
 extern const struct vfe_hw_ops vfe_ops_4_7;
 
diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index cdbd6dba1122..0e2fcee97eeb 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 	if (ret < 0) {
 		dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
 			ret);
-		goto error_media_init;
+		goto error_vb2_init;
 	}
 
 	mutex_init(&video->lock);
@@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 error_video_register:
 	media_entity_cleanup(&vdev->entity);
 	mutex_destroy(&video->lock);
-error_media_init:
-	vb2_queue_release(&video->vb2_q);
 error_vb2_init:
 	mutex_destroy(&video->q_lock);
 
 	return ret;
 }
 
-void msm_video_stop_streaming(struct camss_video *video)
-{
-	if (vb2_is_streaming(&video->vb2_q))
-		vb2_queue_release(&video->vb2_q);
-}
-
 void msm_video_unregister(struct camss_video *video)
 {
 	atomic_inc(&video->camss->ref_count);
-	video_unregister_device(&video->vdev);
+	vb2_video_unregister_device(&video->vdev);
 	atomic_dec(&video->camss->ref_count);
 }
diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
index aa35e8cc6fd5..bdbae8424140 100644
--- a/drivers/media/platform/qcom/camss/camss-video.h
+++ b/drivers/media/platform/qcom/camss/camss-video.h
@@ -52,8 +52,6 @@ struct camss_video {
 	unsigned int nformats;
 };
 
-void msm_video_stop_streaming(struct camss_video *video);
-
 int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 		       const char *name, int is_pix);
 
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 3fdc9f964a3c..d0f4360eb9a0 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
  */
 static int camss_remove(struct platform_device *pdev)
 {
-	unsigned int i;
-
 	struct camss *camss = platform_get_drvdata(pdev);
 
-	for (i = 0; i < camss->vfe_num; i++)
-		msm_vfe_stop_streaming(&camss->vfe[i]);
-
 	v4l2_async_notifier_unregister(&camss->notifier);
 	v4l2_async_notifier_cleanup(&camss->notifier);
 	camss_unregister_entities(camss);
-- 
2.27.0

[PATCHv3 3/7] media/pci: use vb2_video_unregister_device()

[Hans Verkuil]

Use vb2_video_unregister_device() to automatically stop streaming
at unregister time.

This avoids the use of vb2_queue_release() which should not be
called by drivers that set vdev->queue.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/dt3155/dt3155.c           | 3 +--
 drivers/media/pci/intel/ipu3/ipu3-cio2.c    | 9 +++------
 drivers/media/pci/saa7134/saa7134-core.c    | 6 +++---
 drivers/media/pci/saa7134/saa7134-empress.c | 3 +--
 drivers/media/pci/saa7134/saa7134-go7007.c  | 2 +-
 drivers/media/pci/saa7134/saa7134-video.c   | 2 --
 drivers/media/pci/sta2x11/sta2x11_vip.c     | 6 ++----
 drivers/media/pci/tw5864/tw5864-video.c     | 4 +---
 8 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/media/pci/dt3155/dt3155.c b/drivers/media/pci/dt3155/dt3155.c
index 82581aa5a2a3..45c61a12e451 100644
--- a/drivers/media/pci/dt3155/dt3155.c
+++ b/drivers/media/pci/dt3155/dt3155.c
@@ -575,9 +575,8 @@ static void dt3155_remove(struct pci_dev *pdev)
 	struct dt3155_priv *pd = container_of(v4l2_dev, struct dt3155_priv,
 					      v4l2_dev);
 
-	video_unregister_device(&pd->vdev);
+	vb2_video_unregister_device(&pd->vdev);
 	free_irq(pd->pdev->irq, pd);
-	vb2_queue_release(&pd->vidq);
 	v4l2_device_unregister(&pd->v4l2_dev);
 	pci_iounmap(pdev, pd->regs);
 	pci_release_region(pdev, 0);
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 92f5eadf2c99..4e9e34857e4c 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1633,7 +1633,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 	if (r) {
 		dev_err(&cio2->pci_dev->dev,
 			"failed to initialize videobuf2 queue (%d)\n", r);
-		goto fail_vbq;
+		goto fail_subdev;
 	}
 
 	/* Initialize vdev */
@@ -1664,10 +1664,8 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 	return 0;
 
 fail_link:
-	video_unregister_device(&q->vdev);
+	vb2_video_unregister_device(&q->vdev);
 fail_vdev:
-	vb2_queue_release(vbq);
-fail_vbq:
 	v4l2_device_unregister_subdev(subdev);
 fail_subdev:
 	media_entity_cleanup(&vdev->entity);
@@ -1683,9 +1681,8 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 
 static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q)
 {
-	video_unregister_device(&q->vdev);
+	vb2_video_unregister_device(&q->vdev);
 	media_entity_cleanup(&q->vdev.entity);
-	vb2_queue_release(&q->vbq);
 	v4l2_device_unregister_subdev(&q->subdev);
 	media_entity_cleanup(&q->subdev.entity);
 	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
index e4623ed2f831..e3ccb464c044 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -965,21 +965,21 @@ static void saa7134_unregister_video(struct saa7134_dev *dev)
 
 	if (dev->video_dev) {
 		if (video_is_registered(dev->video_dev))
-			video_unregister_device(dev->video_dev);
+			vb2_video_unregister_device(dev->video_dev);
 		else
 			video_device_release(dev->video_dev);
 		dev->video_dev = NULL;
 	}
 	if (dev->vbi_dev) {
 		if (video_is_registered(dev->vbi_dev))
-			video_unregister_device(dev->vbi_dev);
+			vb2_video_unregister_device(dev->vbi_dev);
 		else
 			video_device_release(dev->vbi_dev);
 		dev->vbi_dev = NULL;
 	}
 	if (dev->radio_dev) {
 		if (video_is_registered(dev->radio_dev))
-			video_unregister_device(dev->radio_dev);
+			vb2_video_unregister_device(dev->radio_dev);
 		else
 			video_device_release(dev->radio_dev);
 		dev->radio_dev = NULL;
diff --git a/drivers/media/pci/saa7134/saa7134-empress.c b/drivers/media/pci/saa7134/saa7134-empress.c
index 8ad7879bd840..39e3c7f8c5b4 100644
--- a/drivers/media/pci/saa7134/saa7134-empress.c
+++ b/drivers/media/pci/saa7134/saa7134-empress.c
@@ -314,8 +314,7 @@ static int empress_fini(struct saa7134_dev *dev)
 	if (NULL == dev->empress_dev)
 		return 0;
 	flush_work(&dev->empress_workqueue);
-	video_unregister_device(dev->empress_dev);
-	vb2_queue_release(&dev->empress_vbq);
+	vb2_video_unregister_device(dev->empress_dev);
 	v4l2_ctrl_handler_free(&dev->empress_ctrl_handler);
 	dev->empress_dev = NULL;
 	return 0;
diff --git a/drivers/media/pci/saa7134/saa7134-go7007.c b/drivers/media/pci/saa7134/saa7134-go7007.c
index e1b034663958..f319edb39c0e 100644
--- a/drivers/media/pci/saa7134/saa7134-go7007.c
+++ b/drivers/media/pci/saa7134/saa7134-go7007.c
@@ -493,7 +493,7 @@ static int saa7134_go7007_fini(struct saa7134_dev *dev)
 	free_page((unsigned long)saa->bottom);
 	v4l2_device_unregister_subdev(&saa->sd);
 	kfree(saa);
-	video_unregister_device(&go->vdev);
+	vb2_video_unregister_device(&go->vdev);
 
 	v4l2_device_put(&go->v4l2_dev);
 	dev->empress_dev = NULL;
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index a8ac94fadc14..9a6a6b68f8e3 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2154,9 +2154,7 @@ int saa7134_video_init1(struct saa7134_dev *dev)
 void saa7134_video_fini(struct saa7134_dev *dev)
 {
 	/* free stuff */
-	vb2_queue_release(&dev->video_vbq);
 	saa7134_pgtable_free(dev->pci, &dev->video_q.pt);
-	vb2_queue_release(&dev->vbi_vbq);
 	saa7134_pgtable_free(dev->pci, &dev->vbi_q.pt);
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
 	if (card_has_radio(dev))
diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
index 798574cfad35..a8ab1598dc83 100644
--- a/drivers/media/pci/sta2x11/sta2x11_vip.c
+++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
@@ -1101,12 +1101,11 @@ static int sta2x11_vip_init_one(struct pci_dev *pdev,
 vunreg:
 	video_set_drvdata(&vip->video_dev, NULL);
 vrelease:
-	video_unregister_device(&vip->video_dev);
+	vb2_video_unregister_device(&vip->video_dev);
 	free_irq(pdev->irq, vip);
 release_buf:
 	pci_disable_msi(pdev);
 unmap:
-	vb2_queue_release(&vip->vb_vidq);
 	pci_iounmap(pdev, vip->iomem);
 release:
 	pci_release_regions(pdev);
@@ -1146,10 +1145,9 @@ static void sta2x11_vip_remove_one(struct pci_dev *pdev)
 	sta2x11_vip_clear_register(vip);
 
 	video_set_drvdata(&vip->video_dev, NULL);
-	video_unregister_device(&vip->video_dev);
+	vb2_video_unregister_device(&vip->video_dev);
 	free_irq(pdev->irq, vip);
 	pci_disable_msi(pdev);
-	vb2_queue_release(&vip->vb_vidq);
 	pci_iounmap(pdev, vip->iomem);
 	pci_release_regions(pdev);
 
diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
index ec1e06da7e4f..943f692024af 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -1178,7 +1178,6 @@ static int tw5864_video_input_init(struct tw5864_input *input, int video_nr)
 
 free_v4l2_hdl:
 	v4l2_ctrl_handler_free(hdl);
-	vb2_queue_release(&input->vidq);
 free_mutex:
 	mutex_destroy(&input->lock);
 
@@ -1187,9 +1186,8 @@ static int tw5864_video_input_init(struct tw5864_input *input, int video_nr)
 
 static void tw5864_video_input_fini(struct tw5864_input *dev)
 {
-	video_unregister_device(&dev->vdev);
+	vb2_video_unregister_device(&dev->vdev);
 	v4l2_ctrl_handler_free(&dev->hdl);
-	vb2_queue_release(&dev->vidq);
 }
 
 void tw5864_video_fini(struct tw5864_dev *dev)
-- 
2.27.0

[PATCHv3 4/7] media/platform: drop vb2_queue_release()

[Hans Verkuil]

This is only needed for drivers that do not use vb2_fop_release().
Note that vb2_queue_release() is *not* the counterpart of vb2_queue_init()
as some drivers here seem to think.

Also use vb2_video_unregister_device() to automatically stop streaming
at unregister time for those drivers that set vdev->queue.

Note that sun4i-csi didn't unregister the video device at all. That's
now fixed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Alexandre Courbot <acourbot@chromium.org>
Cc: Tiffany Lin <tiffany.lin@mediatek.com>
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/aspeed-video.c                | 5 +----
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c   | 4 +---
 drivers/media/platform/qcom/venus/vdec.c             | 8 +-------
 drivers/media/platform/qcom/venus/venc.c             | 8 +-------
 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c   | 1 +
 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c   | 6 +-----
 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c | 7 ++-----
 7 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 7d98db1d9b52..c46a79eace98 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -1597,7 +1597,6 @@ static int aspeed_video_setup_video(struct aspeed_video *video)
 	video_set_drvdata(vdev, video);
 	rc = video_register_device(vdev, VFL_TYPE_VIDEO, 0);
 	if (rc) {
-		vb2_queue_release(vbq);
 		v4l2_ctrl_handler_free(&video->ctrl_handler);
 		v4l2_device_unregister(v4l2_dev);
 
@@ -1737,9 +1736,7 @@ static int aspeed_video_remove(struct platform_device *pdev)
 	clk_unprepare(video->vclk);
 	clk_unprepare(video->eclk);
 
-	video_unregister_device(&video->vdev);
-
-	vb2_queue_release(&video->queue);
+	vb2_video_unregister_device(&video->vdev);
 
 	v4l2_ctrl_handler_free(&video->ctrl_handler);
 
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index 0f3e710aed4e..25b2ac03243d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -1525,10 +1525,8 @@ int mtk_vcodec_dec_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->dev             = &ctx->dev->plat_dev->dev;
 
 	ret = vb2_queue_init(dst_vq);
-	if (ret) {
-		vb2_queue_release(src_vq);
+	if (ret)
 		mtk_v4l2_err("Failed to initialize videobuf2 queue(capture)");
-	}
 
 	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 7c4c483d5438..fbefc1634820 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1453,13 +1453,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->allow_zero_bytesused = 1;
 	dst_vq->min_buffers_needed = 0;
 	dst_vq->dev = inst->core->dev;
-	ret = vb2_queue_init(dst_vq);
-	if (ret) {
-		vb2_queue_release(src_vq);
-		return ret;
-	}
-
-	return 0;
+	return vb2_queue_init(dst_vq);
 }
 
 static int vdec_open(struct file *file)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 513bbc07f7bc..a4a92c2a3a28 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1129,13 +1129,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->allow_zero_bytesused = 1;
 	dst_vq->min_buffers_needed = 1;
 	dst_vq->dev = inst->core->dev;
-	ret = vb2_queue_init(dst_vq);
-	if (ret) {
-		vb2_queue_release(src_vq);
-		return ret;
-	}
-
-	return 0;
+	return vb2_queue_init(dst_vq);
 }
 
 static void venc_inst_init(struct venus_inst *inst)
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
index eff34ded6305..fdabd6888bd3 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -285,6 +285,7 @@ static int sun4i_csi_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&csi->notifier);
 	v4l2_async_notifier_cleanup(&csi->notifier);
+	vb2_video_unregister_device(&csi->vdev);
 	media_device_unregister(&csi->mdev);
 	sun4i_csi_dma_unregister(csi);
 	media_device_cleanup(&csi->mdev);
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
index 78fa1c535ac6..dbc9fe254ffd 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -431,7 +431,7 @@ int sun4i_csi_dma_register(struct sun4i_csi *csi, int irq)
 	ret = v4l2_device_register(csi->dev, &csi->v4l);
 	if (ret) {
 		dev_err(csi->dev, "Couldn't register the v4l2 device\n");
-		goto err_free_queue;
+		goto err_free_mutex;
 	}
 
 	ret = devm_request_irq(csi->dev, irq, sun4i_csi_irq, 0,
@@ -446,9 +446,6 @@ int sun4i_csi_dma_register(struct sun4i_csi *csi, int irq)
 err_unregister_device:
 	v4l2_device_unregister(&csi->v4l);
 
-err_free_queue:
-	vb2_queue_release(q);
-
 err_free_mutex:
 	mutex_destroy(&csi->lock);
 	return ret;
@@ -457,6 +454,5 @@ int sun4i_csi_dma_register(struct sun4i_csi *csi, int irq)
 void sun4i_csi_dma_unregister(struct sun4i_csi *csi)
 {
 	v4l2_device_unregister(&csi->v4l);
-	vb2_queue_release(&csi->queue);
 	mutex_destroy(&csi->lock);
 }
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
index d9648b2810b9..b55de9ab64d8 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
@@ -660,13 +660,11 @@ int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
 	if (ret < 0) {
 		v4l2_err(&csi->v4l2_dev,
 			 "video_register_device failed: %d\n", ret);
-		goto release_vb2;
+		goto clean_entity;
 	}
 
 	return 0;
 
-release_vb2:
-	vb2_queue_release(&video->vb2_vidq);
 clean_entity:
 	media_entity_cleanup(&video->vdev.entity);
 	mutex_destroy(&video->lock);
@@ -675,8 +673,7 @@ int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
 
 void sun6i_video_cleanup(struct sun6i_video *video)
 {
-	video_unregister_device(&video->vdev);
+	vb2_video_unregister_device(&video->vdev);
 	media_entity_cleanup(&video->vdev.entity);
-	vb2_queue_release(&video->vb2_vidq);
 	mutex_destroy(&video->lock);
 }
-- 
2.27.0

[PATCHv3 5/7] media/usb: use vb2_video_unregister_device()

[Hans Verkuil]

Use vb2_video_unregister_device() to automatically stop streaming
at unregister time.

This avoids the use of vb2_queue_release() which should not be
called by drivers that set vdev->queue.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/au0828/au0828-video.c  | 12 ++++--------
 drivers/media/usb/dvb-usb/cxusb-analog.c | 13 +++----------
 drivers/media/usb/usbtv/usbtv-video.c    |  4 +---
 3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
index 51b8d14fb4dc..aa5bc6a2ae20 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -938,8 +938,8 @@ int au0828_analog_unregister(struct au0828_dev *dev)
 		return 0;
 
 	mutex_lock(&au0828_sysfs_lock);
-	video_unregister_device(&dev->vdev);
-	video_unregister_device(&dev->vbi_dev);
+	vb2_video_unregister_device(&dev->vdev);
+	vb2_video_unregister_device(&dev->vbi_dev);
 	mutex_unlock(&au0828_sysfs_lock);
 
 	v4l2_device_disconnect(&dev->v4l2_dev);
@@ -2011,8 +2011,7 @@ int au0828_analog_register(struct au0828_dev *dev,
 	if (retval != 0) {
 		dprintk(1, "unable to register video device (error = %d).\n",
 			retval);
-		ret = -ENODEV;
-		goto err_reg_vdev;
+		return -ENODEV;
 	}
 
 	/* Register the vbi device */
@@ -2040,10 +2039,7 @@ int au0828_analog_register(struct au0828_dev *dev,
 	return 0;
 
 err_reg_vbi_dev:
-	video_unregister_device(&dev->vdev);
-err_reg_vdev:
-	vb2_queue_release(&dev->vb_vidq);
-	vb2_queue_release(&dev->vb_vbiq);
+	vb2_video_unregister_device(&dev->vdev);
 	return ret;
 }
 
diff --git a/drivers/media/usb/dvb-usb/cxusb-analog.c b/drivers/media/usb/dvb-usb/cxusb-analog.c
index 001cae648797..e93183ddd797 100644
--- a/drivers/media/usb/dvb-usb/cxusb-analog.c
+++ b/drivers/media/usb/dvb-usb/cxusb-analog.c
@@ -1615,8 +1615,6 @@ static void cxusb_medion_videodev_release(struct video_device *vdev)
 
 	cxusb_vprintk(dvbdev, OPS, "video device release\n");
 
-	vb2_queue_release(vdev->queue);
-
 	video_device_release(vdev);
 }
 
@@ -1647,8 +1645,7 @@ static int cxusb_medion_register_analog_video(struct dvb_usb_device *dvbdev)
 	cxdev->videodev = video_device_alloc();
 	if (!cxdev->videodev) {
 		dev_err(&dvbdev->udev->dev, "video device alloc failed\n");
-		ret = -ENOMEM;
-		goto ret_qrelease;
+		return -ENOMEM;
 	}
 
 	cxdev->videodev->device_caps = videocaps;
@@ -1674,10 +1671,6 @@ static int cxusb_medion_register_analog_video(struct dvb_usb_device *dvbdev)
 
 ret_vrelease:
 	video_device_release(cxdev->videodev);
-
-ret_qrelease:
-	vb2_queue_release(&cxdev->videoqueue);
-
 	return ret;
 }
 
@@ -1820,7 +1813,7 @@ int cxusb_medion_register_analog(struct dvb_usb_device *dvbdev)
 	return 0;
 
 ret_vunreg:
-	video_unregister_device(cxdev->videodev);
+	vb2_video_unregister_device(cxdev->videodev);
 
 ret_unregister:
 	v4l2_device_put(&cxdev->v4l2dev);
@@ -1836,7 +1829,7 @@ void cxusb_medion_unregister_analog(struct dvb_usb_device *dvbdev)
 	cxusb_vprintk(dvbdev, OPS, "unregistering analog\n");
 
 	video_unregister_device(cxdev->radiodev);
-	video_unregister_device(cxdev->videodev);
+	vb2_video_unregister_device(cxdev->videodev);
 
 	v4l2_device_put(&cxdev->v4l2dev);
 	wait_for_completion(&cxdev->v4l2_release);
diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index c89efcd46163..3b4a2e769230 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -872,7 +872,6 @@ static void usbtv_release(struct v4l2_device *v4l2_dev)
 
 	v4l2_device_unregister(&usbtv->v4l2_dev);
 	v4l2_ctrl_handler_free(&usbtv->ctrl);
-	vb2_queue_release(&usbtv->vb2q);
 	kfree(usbtv);
 }
 
@@ -954,7 +953,6 @@ int usbtv_video_init(struct usbtv *usbtv)
 v4l2_fail:
 ctrl_fail:
 	v4l2_ctrl_handler_free(&usbtv->ctrl);
-	vb2_queue_release(&usbtv->vb2q);
 
 	return ret;
 }
@@ -965,7 +963,7 @@ void usbtv_video_free(struct usbtv *usbtv)
 	mutex_lock(&usbtv->v4l2_lock);
 
 	usbtv_stop(usbtv);
-	video_unregister_device(&usbtv->vdev);
+	vb2_video_unregister_device(&usbtv->vdev);
 	v4l2_device_disconnect(&usbtv->v4l2_dev);
 
 	mutex_unlock(&usbtv->v4l2_lock);
-- 
2.27.0

[PATCHv3 6/7] vimc: use vb2_video_unregister_device()

[Hans Verkuil]

Use vb2_video_unregister_device() to automatically stop streaming
at unregister time.

This avoids the use of vb2_queue_release() which should not be
called by drivers that set vdev->queue.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Helen Koike <helen.koike@collabora.com>
---
 .../media/test-drivers/vimc/vimc-capture.c    |  7 ++--
 drivers/media/test-drivers/vivid/vivid-core.c | 32 +++++++++----------
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index c63496b17b9a..5e9fd902cd37 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -351,8 +351,7 @@ static void vimc_cap_unregister(struct vimc_ent_device *ved)
 	struct vimc_cap_device *vcap =
 		container_of(ved, struct vimc_cap_device, ved);
 
-	vb2_queue_release(&vcap->queue);
-	video_unregister_device(&vcap->vdev);
+	vb2_video_unregister_device(&vcap->vdev);
 }
 
 static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
@@ -477,13 +476,11 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 	if (ret) {
 		dev_err(vimc->mdev.dev, "%s: video register failed (err=%d)\n",
 			vcap->vdev.name, ret);
-		goto err_release_queue;
+		goto err_clean_m_ent;
 	}
 
 	return &vcap->ved;
 
-err_release_queue:
-	vb2_queue_release(q);
 err_clean_m_ent:
 	media_entity_cleanup(&vcap->vdev.entity);
 err_free_vcap:
diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index f7ee37e9508d..931218fa955e 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -1827,16 +1827,16 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 	return 0;
 
 unreg_dev:
-	video_unregister_device(&dev->touch_cap_dev);
-	video_unregister_device(&dev->meta_out_dev);
-	video_unregister_device(&dev->meta_cap_dev);
+	vb2_video_unregister_device(&dev->touch_cap_dev);
+	vb2_video_unregister_device(&dev->meta_out_dev);
+	vb2_video_unregister_device(&dev->meta_cap_dev);
 	video_unregister_device(&dev->radio_tx_dev);
 	video_unregister_device(&dev->radio_rx_dev);
-	video_unregister_device(&dev->sdr_cap_dev);
-	video_unregister_device(&dev->vbi_out_dev);
-	video_unregister_device(&dev->vbi_cap_dev);
-	video_unregister_device(&dev->vid_out_dev);
-	video_unregister_device(&dev->vid_cap_dev);
+	vb2_video_unregister_device(&dev->sdr_cap_dev);
+	vb2_video_unregister_device(&dev->vbi_out_dev);
+	vb2_video_unregister_device(&dev->vbi_cap_dev);
+	vb2_video_unregister_device(&dev->vid_out_dev);
+	vb2_video_unregister_device(&dev->vid_cap_dev);
 	cec_unregister_adapter(dev->cec_rx_adap);
 	for (i = 0; i < MAX_OUTPUTS; i++)
 		cec_unregister_adapter(dev->cec_tx_adap[i]);
@@ -1907,27 +1907,27 @@ static int vivid_remove(struct platform_device *pdev)
 		if (dev->has_vid_cap) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				video_device_node_name(&dev->vid_cap_dev));
-			video_unregister_device(&dev->vid_cap_dev);
+			vb2_video_unregister_device(&dev->vid_cap_dev);
 		}
 		if (dev->has_vid_out) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				video_device_node_name(&dev->vid_out_dev));
-			video_unregister_device(&dev->vid_out_dev);
+			vb2_video_unregister_device(&dev->vid_out_dev);
 		}
 		if (dev->has_vbi_cap) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				video_device_node_name(&dev->vbi_cap_dev));
-			video_unregister_device(&dev->vbi_cap_dev);
+			vb2_video_unregister_device(&dev->vbi_cap_dev);
 		}
 		if (dev->has_vbi_out) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				video_device_node_name(&dev->vbi_out_dev));
-			video_unregister_device(&dev->vbi_out_dev);
+			vb2_video_unregister_device(&dev->vbi_out_dev);
 		}
 		if (dev->has_sdr_cap) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				video_device_node_name(&dev->sdr_cap_dev));
-			video_unregister_device(&dev->sdr_cap_dev);
+			vb2_video_unregister_device(&dev->sdr_cap_dev);
 		}
 		if (dev->has_radio_rx) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
@@ -1948,17 +1948,17 @@ static int vivid_remove(struct platform_device *pdev)
 		if (dev->has_meta_cap) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				  video_device_node_name(&dev->meta_cap_dev));
-			video_unregister_device(&dev->meta_cap_dev);
+			vb2_video_unregister_device(&dev->meta_cap_dev);
 		}
 		if (dev->has_meta_out) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				  video_device_node_name(&dev->meta_out_dev));
-			video_unregister_device(&dev->meta_out_dev);
+			vb2_video_unregister_device(&dev->meta_out_dev);
 		}
 		if (dev->has_touch_cap) {
 			v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
 				  video_device_node_name(&dev->touch_cap_dev));
-			video_unregister_device(&dev->touch_cap_dev);
+			vb2_video_unregister_device(&dev->touch_cap_dev);
 		}
 		cec_unregister_adapter(dev->cec_rx_adap);
 		for (j = 0; j < MAX_OUTPUTS; j++)
-- 
2.27.0

[PATCHv3 7/7] staging/media: drop vb2_queue_release()

[Hans Verkuil]

This is only needed for drivers that do not use vb2_fop_release().
Note that vb2_queue_release() is *not* the counterpart of vb2_queue_init()
as some drivers here seem to think.

Also use vb2_video_unregister_device() to automatically stop streaming
at unregister time for those drivers that set vdev->queue.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/staging/media/meson/vdec/vdec.c       | 8 +-------
 drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +-
 drivers/staging/media/rkisp1/rkisp1-params.c  | 7 ++-----
 drivers/staging/media/rkisp1/rkisp1-stats.c   | 6 ++----
 drivers/staging/media/tegra-video/vi.c        | 8 ++------
 5 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 3040136ceb77..5ccb3846c879 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -841,13 +841,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->min_buffers_needed = 1;
 	dst_vq->dev = sess->core->dev;
 	dst_vq->lock = &sess->lock;
-	ret = vb2_queue_init(dst_vq);
-	if (ret) {
-		vb2_queue_release(src_vq);
-		return ret;
-	}
-
-	return 0;
+	return vb2_queue_init(dst_vq);
 }
 
 static int vdec_init_ctrls(struct amvdec_session *sess)
diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 793ec884c894..2a25c3a99ea4 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1277,7 +1277,7 @@ static const struct v4l2_file_operations rkisp1_fops = {
 static void rkisp1_unregister_capture(struct rkisp1_capture *cap)
 {
 	media_entity_cleanup(&cap->vnode.vdev.entity);
-	video_unregister_device(&cap->vnode.vdev);
+	vb2_video_unregister_device(&cap->vnode.vdev);
 }
 
 void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1)
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 797e79de659c..bea074950fc1 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1604,7 +1604,7 @@ int rkisp1_params_register(struct rkisp1_params *params,
 	node->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
 	if (ret)
-		goto err_release_queue;
+		return ret;
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret) {
 		dev_err(rkisp1->dev,
@@ -1614,8 +1614,6 @@ int rkisp1_params_register(struct rkisp1_params *params,
 	return 0;
 err_cleanup_media_entity:
 	media_entity_cleanup(&vdev->entity);
-err_release_queue:
-	vb2_queue_release(vdev->queue);
 	return ret;
 }
 
@@ -1624,7 +1622,6 @@ void rkisp1_params_unregister(struct rkisp1_params *params)
 	struct rkisp1_vdev_node *node = &params->vnode;
 	struct video_device *vdev = &node->vdev;
 
-	video_unregister_device(vdev);
+	vb2_video_unregister_device(vdev);
 	media_entity_cleanup(&vdev->entity);
-	vb2_queue_release(vdev->queue);
 }
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 0616793ae395..7345721a8e99 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -498,11 +498,10 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 	return 0;
 
 err_unreg_vdev:
-	video_unregister_device(vdev);
+	vb2_video_unregister_device(vdev);
 err_cleanup_media_entity:
 	media_entity_cleanup(&vdev->entity);
 err_release_queue:
-	vb2_queue_release(vdev->queue);
 	mutex_destroy(&node->vlock);
 	mutex_destroy(&stats->wq_lock);
 	return ret;
@@ -514,9 +513,8 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
 	struct video_device *vdev = &node->vdev;
 
 	destroy_workqueue(stats->readout_wq);
-	video_unregister_device(vdev);
+	vb2_video_unregister_device(vdev);
 	media_entity_cleanup(&vdev->entity);
-	vb2_queue_release(vdev->queue);
 	mutex_destroy(&node->vlock);
 	mutex_destroy(&stats->wq_lock);
 }
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 1b5e660155f5..a3b9b212f8c8 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -795,12 +795,8 @@ void tegra_v4l2_nodes_cleanup_tpg(struct tegra_video_device *vid)
 	struct tegra_csi_channel *csi_chan;
 	struct tegra_vi_channel *chan;
 
-	list_for_each_entry(chan, &vi->vi_chans, list) {
-		video_unregister_device(&chan->video);
-		mutex_lock(&chan->video_lock);
-		vb2_queue_release(&chan->queue);
-		mutex_unlock(&chan->video_lock);
-	}
+	list_for_each_entry(chan, &vi->vi_chans, list)
+		vb2_video_unregister_device(&chan->video);
 
 	list_for_each_entry(csi_chan, &csi->csi_chans, list)
 		v4l2_device_unregister_subdev(&csi_chan->subdev);
-- 
2.27.0

Re: [PATCHv3 1/7] videobuf2-v4l2.c: add vb2_video_unregister_device helper function

[Robert Foss]

Hey Hans,

Thanks for submitting this.

> If a driver calls (_)vb2_fop_release(), then such a driver should also
> call vb2_video_unregister_device() instead of video_unregister_device().
> This helper will all vb2_queue_release() if a filehandle is marked as
> owner of the queue. This ensures that at unregister time any streaming
> is cancelled and all buffers are returned to userspace.

Nit: "This helper will all vb2_queue_release()" wording is a bit confusing.

Re: [PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Robert Foss]

On Mon, 13 Jul 2020 at 13:30, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Use vb2_video_unregister_device() to automatically stop streaming
> at unregister time.
>
> This avoids the use of vb2_queue_release() which should not be
> called by drivers that set vdev->queue.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
>  drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
>  drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>  drivers/media/platform/qcom/camss/camss-video.h |  2 --
>  drivers/media/platform/qcom/camss/camss.c       |  5 -----
>  5 files changed, 2 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index fc31c2c169cd..b7d2293a5004 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>         .flush_buffers = vfe_flush_buffers,
>  };
>
> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
> -{
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
> -               msm_video_stop_streaming(&vfe->line[i].video_out);
> -}
> -
>  /*
>   * msm_vfe_register_entities - Register subdev node for VFE module
>   * @vfe: VFE device
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index 0d10071ae881..a90b0d2cc6de 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>  void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>  void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>
> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
> -
>  extern const struct vfe_hw_ops vfe_ops_4_1;
>  extern const struct vfe_hw_ops vfe_ops_4_7;
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index cdbd6dba1122..0e2fcee97eeb 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>         if (ret < 0) {
>                 dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>                         ret);
> -               goto error_media_init;
> +               goto error_vb2_init;
>         }
>
>         mutex_init(&video->lock);
> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  error_video_register:
>         media_entity_cleanup(&vdev->entity);
>         mutex_destroy(&video->lock);
> -error_media_init:
> -       vb2_queue_release(&video->vb2_q);
>  error_vb2_init:
>         mutex_destroy(&video->q_lock);
>
>         return ret;
>  }
>
> -void msm_video_stop_streaming(struct camss_video *video)
> -{
> -       if (vb2_is_streaming(&video->vb2_q))
> -               vb2_queue_release(&video->vb2_q);
> -}
> -
>  void msm_video_unregister(struct camss_video *video)
>  {
>         atomic_inc(&video->camss->ref_count);
> -       video_unregister_device(&video->vdev);
> +       vb2_video_unregister_device(&video->vdev);
>         atomic_dec(&video->camss->ref_count);
>  }
> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
> index aa35e8cc6fd5..bdbae8424140 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.h
> +++ b/drivers/media/platform/qcom/camss/camss-video.h
> @@ -52,8 +52,6 @@ struct camss_video {
>         unsigned int nformats;
>  };
>
> -void msm_video_stop_streaming(struct camss_video *video);
> -
>  int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>                        const char *name, int is_pix);
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 3fdc9f964a3c..d0f4360eb9a0 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>   */
>  static int camss_remove(struct platform_device *pdev)
>  {
> -       unsigned int i;
> -
>         struct camss *camss = platform_get_drvdata(pdev);
>
> -       for (i = 0; i < camss->vfe_num; i++)
> -               msm_vfe_stop_streaming(&camss->vfe[i]);
> -

I'm trying to understand this change, with msm_vfe_stop_streaming
removed, are we relying on vb2_video_unregister_device being called
through some other path?

>         v4l2_async_notifier_unregister(&camss->notifier);
>         v4l2_async_notifier_cleanup(&camss->notifier);
>         camss_unregister_entities(camss);
> --
> 2.27.0
>

Re: [PATCHv3 1/7] videobuf2-v4l2.c: add vb2_video_unregister_device helper function

[Hans Verkuil]

On 13/07/2020 17:55, Robert Foss wrote:
> Hey Hans,
> 
> Thanks for submitting this.
> 
>> If a driver calls (_)vb2_fop_release(), then such a driver should also
>> call vb2_video_unregister_device() instead of video_unregister_device().
>> This helper will all vb2_queue_release() if a filehandle is marked as
>> owner of the queue. This ensures that at unregister time any streaming
>> is cancelled and all buffers are returned to userspace.
> 
> Nit: "This helper will all vb2_queue_release()" wording is a bit confusing.
> 

Oops! all -> call

Regards,

	Hans

Re: [PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Hans Verkuil]

On 13/07/2020 18:05, Robert Foss wrote:
> On Mon, 13 Jul 2020 at 13:30, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> Use vb2_video_unregister_device() to automatically stop streaming
>> at unregister time.
>>
>> This avoids the use of vb2_queue_release() which should not be
>> called by drivers that set vdev->queue.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
>>  drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
>>  drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>>  drivers/media/platform/qcom/camss/camss-video.h |  2 --
>>  drivers/media/platform/qcom/camss/camss.c       |  5 -----
>>  5 files changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index fc31c2c169cd..b7d2293a5004 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>>         .flush_buffers = vfe_flush_buffers,
>>  };
>>
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
>> -{
>> -       int i;
>> -
>> -       for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
>> -               msm_video_stop_streaming(&vfe->line[i].video_out);
>> -}
>> -
>>  /*
>>   * msm_vfe_register_entities - Register subdev node for VFE module
>>   * @vfe: VFE device
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
>> index 0d10071ae881..a90b0d2cc6de 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>>  void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>>  void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>>
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
>> -
>>  extern const struct vfe_hw_ops vfe_ops_4_1;
>>  extern const struct vfe_hw_ops vfe_ops_4_7;
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
>> index cdbd6dba1122..0e2fcee97eeb 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.c
>> +++ b/drivers/media/platform/qcom/camss/camss-video.c
>> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>         if (ret < 0) {
>>                 dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>>                         ret);
>> -               goto error_media_init;
>> +               goto error_vb2_init;
>>         }
>>
>>         mutex_init(&video->lock);
>> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>  error_video_register:
>>         media_entity_cleanup(&vdev->entity);
>>         mutex_destroy(&video->lock);
>> -error_media_init:
>> -       vb2_queue_release(&video->vb2_q);
>>  error_vb2_init:
>>         mutex_destroy(&video->q_lock);
>>
>>         return ret;
>>  }
>>
>> -void msm_video_stop_streaming(struct camss_video *video)
>> -{
>> -       if (vb2_is_streaming(&video->vb2_q))
>> -               vb2_queue_release(&video->vb2_q);
>> -}
>> -
>>  void msm_video_unregister(struct camss_video *video)
>>  {
>>         atomic_inc(&video->camss->ref_count);
>> -       video_unregister_device(&video->vdev);
>> +       vb2_video_unregister_device(&video->vdev);
>>         atomic_dec(&video->camss->ref_count);
>>  }
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
>> index aa35e8cc6fd5..bdbae8424140 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.h
>> +++ b/drivers/media/platform/qcom/camss/camss-video.h
>> @@ -52,8 +52,6 @@ struct camss_video {
>>         unsigned int nformats;
>>  };
>>
>> -void msm_video_stop_streaming(struct camss_video *video);
>> -
>>  int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>                        const char *name, int is_pix);
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 3fdc9f964a3c..d0f4360eb9a0 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>>   */
>>  static int camss_remove(struct platform_device *pdev)
>>  {
>> -       unsigned int i;
>> -
>>         struct camss *camss = platform_get_drvdata(pdev);
>>
>> -       for (i = 0; i < camss->vfe_num; i++)
>> -               msm_vfe_stop_streaming(&camss->vfe[i]);
>> -
> 
> I'm trying to understand this change, with msm_vfe_stop_streaming
> removed, are we relying on vb2_video_unregister_device being called
> through some other path?
> 
>>         v4l2_async_notifier_unregister(&camss->notifier);
>>         v4l2_async_notifier_cleanup(&camss->notifier);
>>         camss_unregister_entities(camss);

Yes: camss_unregister_entities -> camss_unregister_entities ->
	msm_vfe_unregister_entities -> msm_video_unregister -> vb2_video_unregister_device

I have to say that this is a pretty convoluted call sequence, it took me a
bit of time to unravel it.

That said, I think I want a 'Tested-by' before I'll apply this specific patch.

Unfortunately, I don't have hardware to test this. Are you or Andrey able to test it?

Regards,

	Hans

Re: [PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Andrey Konovalov]

Hi Hans,

On 16.07.2020 11:12, Hans Verkuil wrote:
> On 13/07/2020 18:05, Robert Foss wrote:
>> On Mon, 13 Jul 2020 at 13:30, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>
>>> Use vb2_video_unregister_device() to automatically stop streaming
>>> at unregister time.
>>>
>>> This avoids the use of vb2_queue_release() which should not be
>>> called by drivers that set vdev->queue.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Cc: Robert Foss <robert.foss@linaro.org>
>>> Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
>>>   drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
>>>   drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>>>   drivers/media/platform/qcom/camss/camss-video.h |  2 --
>>>   drivers/media/platform/qcom/camss/camss.c       |  5 -----
>>>   5 files changed, 2 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>>> index fc31c2c169cd..b7d2293a5004 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>>> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>>>          .flush_buffers = vfe_flush_buffers,
>>>   };
>>>
>>> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
>>> -{
>>> -       int i;
>>> -
>>> -       for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
>>> -               msm_video_stop_streaming(&vfe->line[i].video_out);
>>> -}
>>> -
>>>   /*
>>>    * msm_vfe_register_entities - Register subdev node for VFE module
>>>    * @vfe: VFE device
>>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
>>> index 0d10071ae881..a90b0d2cc6de 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>>> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>>>   void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>>>   void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>>>
>>> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
>>> -
>>>   extern const struct vfe_hw_ops vfe_ops_4_1;
>>>   extern const struct vfe_hw_ops vfe_ops_4_7;
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
>>> index cdbd6dba1122..0e2fcee97eeb 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-video.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-video.c
>>> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>>          if (ret < 0) {
>>>                  dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>>>                          ret);
>>> -               goto error_media_init;
>>> +               goto error_vb2_init;
>>>          }
>>>
>>>          mutex_init(&video->lock);
>>> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>>   error_video_register:
>>>          media_entity_cleanup(&vdev->entity);
>>>          mutex_destroy(&video->lock);
>>> -error_media_init:
>>> -       vb2_queue_release(&video->vb2_q);
>>>   error_vb2_init:
>>>          mutex_destroy(&video->q_lock);
>>>
>>>          return ret;
>>>   }
>>>
>>> -void msm_video_stop_streaming(struct camss_video *video)
>>> -{
>>> -       if (vb2_is_streaming(&video->vb2_q))
>>> -               vb2_queue_release(&video->vb2_q);
>>> -}
>>> -
>>>   void msm_video_unregister(struct camss_video *video)
>>>   {
>>>          atomic_inc(&video->camss->ref_count);
>>> -       video_unregister_device(&video->vdev);
>>> +       vb2_video_unregister_device(&video->vdev);
>>>          atomic_dec(&video->camss->ref_count);
>>>   }
>>> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
>>> index aa35e8cc6fd5..bdbae8424140 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-video.h
>>> +++ b/drivers/media/platform/qcom/camss/camss-video.h
>>> @@ -52,8 +52,6 @@ struct camss_video {
>>>          unsigned int nformats;
>>>   };
>>>
>>> -void msm_video_stop_streaming(struct camss_video *video);
>>> -
>>>   int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>>                         const char *name, int is_pix);
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>>> index 3fdc9f964a3c..d0f4360eb9a0 100644
>>> --- a/drivers/media/platform/qcom/camss/camss.c
>>> +++ b/drivers/media/platform/qcom/camss/camss.c
>>> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>>>    */
>>>   static int camss_remove(struct platform_device *pdev)
>>>   {
>>> -       unsigned int i;
>>> -
>>>          struct camss *camss = platform_get_drvdata(pdev);
>>>
>>> -       for (i = 0; i < camss->vfe_num; i++)
>>> -               msm_vfe_stop_streaming(&camss->vfe[i]);
>>> -
>>
>> I'm trying to understand this change, with msm_vfe_stop_streaming
>> removed, are we relying on vb2_video_unregister_device being called
>> through some other path?
>>
>>>          v4l2_async_notifier_unregister(&camss->notifier);
>>>          v4l2_async_notifier_cleanup(&camss->notifier);
>>>          camss_unregister_entities(camss);
> 
> Yes: camss_unregister_entities -> camss_unregister_entities ->
> 	msm_vfe_unregister_entities -> msm_video_unregister -> vb2_video_unregister_device
> 
> I have to say that this is a pretty convoluted call sequence, it took me a
> bit of time to unravel it.
> 
> That said, I think I want a 'Tested-by' before I'll apply this specific patch.
> 
> Unfortunately, I don't have hardware to test this. Are you or Andrey able to test it?

I can do that.

Thanks,
Andrey

> Regards,
> 
> 	Hans
> 

Re: [PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Andrey Konovalov]

Hi Hans,

I've applied the first two patches of this series, and have given them a try
on db410c board with imx327c camera module by Framos.
Looks good.

Tested-by: Andrey Konovalov <andrey.konovalov@linaro.org>

Thanks,
Andrey

On 13.07.2020 14:30, Hans Verkuil wrote:
> Use vb2_video_unregister_device() to automatically stop streaming
> at unregister time.
> 
> This avoids the use of vb2_queue_release() which should not be
> called by drivers that set vdev->queue.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
>   drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
>   drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>   drivers/media/platform/qcom/camss/camss-video.h |  2 --
>   drivers/media/platform/qcom/camss/camss.c       |  5 -----
>   5 files changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index fc31c2c169cd..b7d2293a5004 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>   	.flush_buffers = vfe_flush_buffers,
>   };
>   
> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
> -		msm_video_stop_streaming(&vfe->line[i].video_out);
> -}
> -
>   /*
>    * msm_vfe_register_entities - Register subdev node for VFE module
>    * @vfe: VFE device
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index 0d10071ae881..a90b0d2cc6de 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>   void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>   void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>   
> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
> -
>   extern const struct vfe_hw_ops vfe_ops_4_1;
>   extern const struct vfe_hw_ops vfe_ops_4_7;
>   
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index cdbd6dba1122..0e2fcee97eeb 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>   	if (ret < 0) {
>   		dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>   			ret);
> -		goto error_media_init;
> +		goto error_vb2_init;
>   	}
>   
>   	mutex_init(&video->lock);
> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>   error_video_register:
>   	media_entity_cleanup(&vdev->entity);
>   	mutex_destroy(&video->lock);
> -error_media_init:
> -	vb2_queue_release(&video->vb2_q);
>   error_vb2_init:
>   	mutex_destroy(&video->q_lock);
>   
>   	return ret;
>   }
>   
> -void msm_video_stop_streaming(struct camss_video *video)
> -{
> -	if (vb2_is_streaming(&video->vb2_q))
> -		vb2_queue_release(&video->vb2_q);
> -}
> -
>   void msm_video_unregister(struct camss_video *video)
>   {
>   	atomic_inc(&video->camss->ref_count);
> -	video_unregister_device(&video->vdev);
> +	vb2_video_unregister_device(&video->vdev);
>   	atomic_dec(&video->camss->ref_count);
>   }
> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
> index aa35e8cc6fd5..bdbae8424140 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.h
> +++ b/drivers/media/platform/qcom/camss/camss-video.h
> @@ -52,8 +52,6 @@ struct camss_video {
>   	unsigned int nformats;
>   };
>   
> -void msm_video_stop_streaming(struct camss_video *video);
> -
>   int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>   		       const char *name, int is_pix);
>   
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 3fdc9f964a3c..d0f4360eb9a0 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>    */
>   static int camss_remove(struct platform_device *pdev)
>   {
> -	unsigned int i;
> -
>   	struct camss *camss = platform_get_drvdata(pdev);
>   
> -	for (i = 0; i < camss->vfe_num; i++)
> -		msm_vfe_stop_streaming(&camss->vfe[i]);
> -
>   	v4l2_async_notifier_unregister(&camss->notifier);
>   	v4l2_async_notifier_cleanup(&camss->notifier);
>   	camss_unregister_entities(camss);
> 

Re: [PATCHv3 2/7] qcom/camss: use vb2_video_unregister_device()

[Hans Verkuil]

On 17/07/2020 18:21, Andrey Konovalov wrote:
> Hi Hans,
> 
> I've applied the first two patches of this series, and have given them a try
> on db410c board with imx327c camera module by Framos.
> Looks good.
> 
> Tested-by: Andrey Konovalov <andrey.konovalov@linaro.org>

Much appreciated!

	Hans

> 
> Thanks,
> Andrey
> 
> On 13.07.2020 14:30, Hans Verkuil wrote:
>> Use vb2_video_unregister_device() to automatically stop streaming
>> at unregister time.
>>
>> This avoids the use of vb2_queue_release() which should not be
>> called by drivers that set vdev->queue.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/platform/qcom/camss/camss-vfe.c   |  8 --------
>>   drivers/media/platform/qcom/camss/camss-vfe.h   |  2 --
>>   drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>>   drivers/media/platform/qcom/camss/camss-video.h |  2 --
>>   drivers/media/platform/qcom/camss/camss.c       |  5 -----
>>   5 files changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index fc31c2c169cd..b7d2293a5004 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>>   	.flush_buffers = vfe_flush_buffers,
>>   };
>>   
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
>> -{
>> -	int i;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
>> -		msm_video_stop_streaming(&vfe->line[i].video_out);
>> -}
>> -
>>   /*
>>    * msm_vfe_register_entities - Register subdev node for VFE module
>>    * @vfe: VFE device
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
>> index 0d10071ae881..a90b0d2cc6de 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>>   void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>>   void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>>   
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
>> -
>>   extern const struct vfe_hw_ops vfe_ops_4_1;
>>   extern const struct vfe_hw_ops vfe_ops_4_7;
>>   
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
>> index cdbd6dba1122..0e2fcee97eeb 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.c
>> +++ b/drivers/media/platform/qcom/camss/camss-video.c
>> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>   	if (ret < 0) {
>>   		dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>>   			ret);
>> -		goto error_media_init;
>> +		goto error_vb2_init;
>>   	}
>>   
>>   	mutex_init(&video->lock);
>> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>   error_video_register:
>>   	media_entity_cleanup(&vdev->entity);
>>   	mutex_destroy(&video->lock);
>> -error_media_init:
>> -	vb2_queue_release(&video->vb2_q);
>>   error_vb2_init:
>>   	mutex_destroy(&video->q_lock);
>>   
>>   	return ret;
>>   }
>>   
>> -void msm_video_stop_streaming(struct camss_video *video)
>> -{
>> -	if (vb2_is_streaming(&video->vb2_q))
>> -		vb2_queue_release(&video->vb2_q);
>> -}
>> -
>>   void msm_video_unregister(struct camss_video *video)
>>   {
>>   	atomic_inc(&video->camss->ref_count);
>> -	video_unregister_device(&video->vdev);
>> +	vb2_video_unregister_device(&video->vdev);
>>   	atomic_dec(&video->camss->ref_count);
>>   }
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
>> index aa35e8cc6fd5..bdbae8424140 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.h
>> +++ b/drivers/media/platform/qcom/camss/camss-video.h
>> @@ -52,8 +52,6 @@ struct camss_video {
>>   	unsigned int nformats;
>>   };
>>   
>> -void msm_video_stop_streaming(struct camss_video *video);
>> -
>>   int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>>   		       const char *name, int is_pix);
>>   
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 3fdc9f964a3c..d0f4360eb9a0 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>>    */
>>   static int camss_remove(struct platform_device *pdev)
>>   {
>> -	unsigned int i;
>> -
>>   	struct camss *camss = platform_get_drvdata(pdev);
>>   
>> -	for (i = 0; i < camss->vfe_num; i++)
>> -		msm_vfe_stop_streaming(&camss->vfe[i]);
>> -
>>   	v4l2_async_notifier_unregister(&camss->notifier);
>>   	v4l2_async_notifier_cleanup(&camss->notifier);
>>   	camss_unregister_entities(camss);
>>