[RFC PATCH 0/5] media: rcar_vin: Fixes for buffer management

[RFC PATCH 0/5] media: rcar_vin: Fixes for buffer management

[William Towle]

  The following is a subset of our work in progress branch for video
support on the Renesas "Lager" board, comprising hotfixes for video
buffer management.

  We are successfully capturing single frames and video with the
complete branch, and intend to follow up with stable patches from
the branch in due course.

  Included here:
	[PATCH 1/2] media: rcar_vin: helper function for streaming stop
	[PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler

Cheers,
  Wills.

[PATCH 1/2] media: rcar_vin: helper function for streaming stop

[William Towle]

From: Ian Molton <ian.molton@codethink.co.uk>

The code that tests that capture from a stream has stopped is
presently insufficient and the potential for a race condition
exists where frame capture may generate an interrupt between
requesting the capture process halt and freeing buffers.

This patch refactors code out of rcar_vin_videobuf_release() and
into rcar_vin_wait_stop_streaming(), and ensures there are calls
in places where we need to know that capturing has finished.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   41 +++++++++++++++++---------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 8d8438b..89c409b 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -458,6 +458,28 @@ error:
 	vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
 }
 
+/*
+ * Wait for capture to stop and all in-flight buffers to be finished with by
+ * the video hardware. This must be called under &priv->lock
+ *
+ */
+static void rcar_vin_wait_stop_streaming(struct rcar_vin_priv *priv)
+{
+	while (priv->state != STOPPED) {
+		/* issue stop if running */
+		if (priv->state == RUNNING)
+			rcar_vin_request_capture_stop(priv);
+
+		/* wait until capturing has been stopped */
+		if (priv->state == STOPPING) {
+			priv->request_to_stop = true;
+			spin_unlock_irq(&priv->lock);
+			wait_for_completion(&priv->capture_stop);
+			spin_lock_irq(&priv->lock);
+		}
+	}
+}
+
 static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 {
 	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
@@ -477,20 +499,8 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 	}
 
 	if (buf_in_use) {
-		while (priv->state != STOPPED) {
-
-			/* issue stop if running */
-			if (priv->state == RUNNING)
-				rcar_vin_request_capture_stop(priv);
-
-			/* wait until capturing has been stopped */
-			if (priv->state == STOPPING) {
-				priv->request_to_stop = true;
-				spin_unlock_irq(&priv->lock);
-				wait_for_completion(&priv->capture_stop);
-				spin_lock_irq(&priv->lock);
-			}
-		}
+		rcar_vin_wait_stop_streaming(priv);
+
 		/*
 		 * Capturing has now stopped. The buffer we have been asked
 		 * to release could be any of the current buffers in use, so
@@ -524,8 +534,11 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 	struct list_head *buf_head, *tmp;
 
 	spin_lock_irq(&priv->lock);
+
+	rcar_vin_wait_stop_streaming(priv);
 	list_for_each_safe(buf_head, tmp, &priv->capture)
 		list_del_init(buf_head);
+
 	spin_unlock_irq(&priv->lock);
 }
 
-- 
1.7.10.4

[PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler

[William Towle]

This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.

By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   47 +++++++++-----------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..022fa9d 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -485,37 +485,10 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct rcar_vin_priv *priv = ici->priv;
-	unsigned int i;
-	int buf_in_use = 0;
 
 	spin_lock_irq(&priv->lock);
 
-	/* Is the buffer in use by the VIN hardware? */
-	for (i = 0; i < MAX_BUFFER_NUM; i++) {
-		if (priv->queue_buf[i] == vb) {
-			buf_in_use = 1;
-			break;
-		}
-	}
-
-	if (buf_in_use) {
-		rcar_vin_wait_stop_streaming(priv);
-
-		/*
-		 * Capturing has now stopped. The buffer we have been asked
-		 * to release could be any of the current buffers in use, so
-		 * release all buffers that are in use by HW
-		 */
-		for (i = 0; i < MAX_BUFFER_NUM; i++) {
-			if (priv->queue_buf[i]) {
-				vb2_buffer_done(priv->queue_buf[i],
-					VB2_BUF_STATE_ERROR);
-				priv->queue_buf[i] = NULL;
-			}
-		}
-	} else {
-		list_del_init(to_buf_list(vb));
-	}
+	list_del_init(to_buf_list(vb));
 
 	spin_unlock_irq(&priv->lock);
 }
@@ -532,13 +505,25 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct rcar_vin_priv *priv = ici->priv;
 	struct list_head *buf_head, *tmp;
+	int i;
 
 	spin_lock_irq(&priv->lock);
-
 	rcar_vin_wait_stop_streaming(priv);
-	list_for_each_safe(buf_head, tmp, &priv->capture)
-		list_del_init(buf_head);
 
+	for (i = 0; i < MAX_BUFFER_NUM; i++) {
+		if (priv->queue_buf[i]) {
+			vb2_buffer_done(priv->queue_buf[i],
+					VB2_BUF_STATE_ERROR);
+			priv->queue_buf[i] = NULL;
+		}
+	}
+
+	list_for_each_safe(buf_head, tmp, &priv->capture) {
+		vb2_buffer_done(&list_entry(buf_head,
+					struct rcar_vin_buffer, list)->vb,
+				VB2_BUF_STATE_ERROR);
+		list_del_init(buf_head);
+	}
 	spin_unlock_irq(&priv->lock);
 }
 
-- 
1.7.10.4

[PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler

[William Towle]

This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.

By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.

After the refactoring, the buf_init and buf_cleanup functions were
found to contain only initialisation/release steps as are carried out
elsewhere if omitted; these functions and references were removed.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   62 ++++++--------------------
 1 file changed, 13 insertions(+), 49 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..aa25a3b 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -480,72 +480,36 @@ static void rcar_vin_wait_stop_streaming(struct rcar_vin_priv *priv)
 	}
 }
 
-static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
+static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 {
-	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct rcar_vin_priv *priv = ici->priv;
-	unsigned int i;
-	int buf_in_use = 0;
+	struct list_head *buf_head, *tmp;
+	int i;
 
 	spin_lock_irq(&priv->lock);
+	rcar_vin_wait_stop_streaming(priv);
 
-	/* Is the buffer in use by the VIN hardware? */
 	for (i = 0; i < MAX_BUFFER_NUM; i++) {
-		if (priv->queue_buf[i] == vb) {
-			buf_in_use = 1;
-			break;
-		}
-	}
-
-	if (buf_in_use) {
-		rcar_vin_wait_stop_streaming(priv);
-
-		/*
-		 * Capturing has now stopped. The buffer we have been asked
-		 * to release could be any of the current buffers in use, so
-		 * release all buffers that are in use by HW
-		 */
-		for (i = 0; i < MAX_BUFFER_NUM; i++) {
-			if (priv->queue_buf[i]) {
-				vb2_buffer_done(priv->queue_buf[i],
+		if (priv->queue_buf[i]) {
+			vb2_buffer_done(priv->queue_buf[i],
 					VB2_BUF_STATE_ERROR);
-				priv->queue_buf[i] = NULL;
-			}
+			priv->queue_buf[i] = NULL;
 		}
-	} else {
-		list_del_init(to_buf_list(vb));
 	}
 
-	spin_unlock_irq(&priv->lock);
-}
-
-static int rcar_vin_videobuf_init(struct vb2_buffer *vb)
-{
-	INIT_LIST_HEAD(to_buf_list(vb));
-	return 0;
-}
-
-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);
-	struct rcar_vin_priv *priv = ici->priv;
-	struct list_head *buf_head, *tmp;
-
-	spin_lock_irq(&priv->lock);
-
-	rcar_vin_wait_stop_streaming(priv);
-	list_for_each_safe(buf_head, tmp, &priv->capture)
+	list_for_each_safe(buf_head, tmp, &priv->capture) {
+		vb2_buffer_done(&list_entry(buf_head,
+					struct rcar_vin_buffer, list)->vb,
+				VB2_BUF_STATE_ERROR);
 		list_del_init(buf_head);
-
+	}
 	spin_unlock_irq(&priv->lock);
 }
 
 static struct vb2_ops rcar_vin_vb2_ops = {
 	.queue_setup	= rcar_vin_videobuf_setup,
-	.buf_init	= rcar_vin_videobuf_init,
-	.buf_cleanup	= rcar_vin_videobuf_release,
 	.buf_queue	= rcar_vin_videobuf_queue,
 	.stop_streaming	= rcar_vin_stop_streaming,
 	.wait_prepare	= soc_camera_unlock,
-- 
1.7.10.4

[PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler

[William Towle]

This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.

By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   47 +++++++++-----------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..022fa9d 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -485,37 +485,10 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct rcar_vin_priv *priv = ici->priv;
-	unsigned int i;
-	int buf_in_use = 0;
 
 	spin_lock_irq(&priv->lock);
 
-	/* Is the buffer in use by the VIN hardware? */
-	for (i = 0; i < MAX_BUFFER_NUM; i++) {
-		if (priv->queue_buf[i] == vb) {
-			buf_in_use = 1;
-			break;
-		}
-	}
-
-	if (buf_in_use) {
-		rcar_vin_wait_stop_streaming(priv);
-
-		/*
-		 * Capturing has now stopped. The buffer we have been asked
-		 * to release could be any of the current buffers in use, so
-		 * release all buffers that are in use by HW
-		 */
-		for (i = 0; i < MAX_BUFFER_NUM; i++) {
-			if (priv->queue_buf[i]) {
-				vb2_buffer_done(priv->queue_buf[i],
-					VB2_BUF_STATE_ERROR);
-				priv->queue_buf[i] = NULL;
-			}
-		}
-	} else {
-		list_del_init(to_buf_list(vb));
-	}
+	list_del_init(to_buf_list(vb));
 
 	spin_unlock_irq(&priv->lock);
 }
@@ -532,13 +505,25 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct rcar_vin_priv *priv = ici->priv;
 	struct list_head *buf_head, *tmp;
+	int i;
 
 	spin_lock_irq(&priv->lock);
-
 	rcar_vin_wait_stop_streaming(priv);
-	list_for_each_safe(buf_head, tmp, &priv->capture)
-		list_del_init(buf_head);
 
+	for (i = 0; i < MAX_BUFFER_NUM; i++) {
+		if (priv->queue_buf[i]) {
+			vb2_buffer_done(priv->queue_buf[i],
+					VB2_BUF_STATE_ERROR);
+			priv->queue_buf[i] = NULL;
+		}
+	}
+
+	list_for_each_safe(buf_head, tmp, &priv->capture) {
+		vb2_buffer_done(&list_entry(buf_head,
+					struct rcar_vin_buffer, list)->vb,
+				VB2_BUF_STATE_ERROR);
+		list_del_init(buf_head);
+	}
 	spin_unlock_irq(&priv->lock);
 }
 
-- 
1.7.10.4