[PATCH 0/4] rcar_vin: fix soc_camera WARN_ON() issues.

[PATCH 0/4] rcar_vin: fix soc_camera WARN_ON() issues.

[Ian Molton]

This patch series provides fixes that allow the rcar_vin driver to function
without triggering dozens of warnings from the videobuf2 and soc_camera layers.

Patches 2/3 should probably be merged into a single, atomic change, although
patch 2 does not make the existing situation /worse/ in and of itself.

Patch 4 does not change the code logic, but is cleaner and less prone to
breakage caused by furtutre modification. Also, more consistent with the use of
vb pointers elsewhere in the driver.

Comments welcome!

-Ian

[PATCH 1/4] media: rcar_vin: Dont aggressively retire buffers

[Ian Molton]

rcar_vin_videobuf_release() is called once per buffer from the buf_cleanup hook.

There is no need to look up the queue and free all buffers at this point.

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 | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index e594230..7154500 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -493,17 +493,11 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 		 * 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));
+		priv->queue_buf[i] = NULL;
 	}
 
+	list_del_init(to_buf_list(vb));
+
 	spin_unlock_irq(&priv->lock);
 }
 
-- 
1.9.1

[PATCH 2/4] media: rcar_vin: Ensure all in-flight buffers are returned to error state before stopping.

[Ian Molton]

Videobuf2 complains about buffers that are still marked ACTIVE (in use by the driver) following a call to stop_streaming().

This patch returns all active buffers to state ERROR prior to stopping.

Note: this introduces a (non fatal) race condition as the stream is not guaranteed to be stopped at this point.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 7154500..06ce705 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -513,8 +513,14 @@ 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);
+
+	for (i = 0; i < vq->num_buffers; ++i)
+		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
+			vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR);
+
 	list_for_each_safe(buf_head, tmp, &priv->capture)
 		list_del_init(buf_head);
 	spin_unlock_irq(&priv->lock);
-- 
1.9.1

[PATCH 3/4] media: rcar_vin: Fix race condition terminating stream

[Ian Molton]

This patch fixes a race condition whereby a frame being captured may generate an
 interrupt between requesting capture to halt and freeing buffers.

This condition is exposed by the earlier patch that explicitly calls
vb2_buffer_done() during stop streaming.

The solution is to wait for capture to finish prior to finalising these buffers.

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 | 43 ++++++++++++++++++----------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 06ce705..aeda4e2 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -455,6 +455,29 @@ 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);
@@ -462,7 +485,6 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
 	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? */
@@ -474,20 +496,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
@@ -517,12 +527,15 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 
 	spin_lock_irq(&priv->lock);
 
+	rcar_vin_wait_stop_streaming(priv);
+
 	for (i = 0; i < vq->num_buffers; ++i)
 		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
 			vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR);
 
 	list_for_each_safe(buf_head, tmp, &priv->capture)
 		list_del_init(buf_head);
+
 	spin_unlock_irq(&priv->lock);
 }
 
-- 
1.9.1

[PATCH 4/4] media: rcar_vin: Clean up rcar_vin_irq

[Ian Molton]

This patch makes the rcar_vin IRQ handler a little more readable.

Removes an else clause, and simplifies the buffer handling.

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

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index aeda4e2..a8d2785 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -554,7 +554,6 @@ static irqreturn_t rcar_vin_irq(int irq, void *data)
 	struct rcar_vin_priv *priv = data;
 	u32 int_status;
 	bool can_run = false, hw_stopped;
-	int slot;
 	unsigned int handled = 0;
 
 	spin_lock(&priv->lock);
@@ -573,17 +572,22 @@ static irqreturn_t rcar_vin_irq(int irq, void *data)
 	hw_stopped = !(ioread32(priv->base + VNMS_REG) & VNMS_CA);
 
 	if (!priv->request_to_stop) {
+		struct vb2_buffer **q_entry = priv->queue_buf;
+		struct vb2_buffer *vb;
+
 		if (is_continuous_transfer(priv))
-			slot = (ioread32(priv->base + VNMS_REG) &
-				VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
-		else
-			slot = 0;
+			q_entry += (ioread32(priv->base + VNMS_REG) &
+					VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
+
+		vb = *q_entry;
+
+		vb->v4l2_buf.field = priv->field;
+		vb->v4l2_buf.sequence = priv->sequence++;
+		do_gettimeofday(&vb->v4l2_buf.timestamp);
+
+		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 
-		priv->queue_buf[slot]->v4l2_buf.field = priv->field;
-		priv->queue_buf[slot]->v4l2_buf.sequence = priv->sequence++;
-		do_gettimeofday(&priv->queue_buf[slot]->v4l2_buf.timestamp);
-		vb2_buffer_done(priv->queue_buf[slot], VB2_BUF_STATE_DONE);
-		priv->queue_buf[slot] = NULL;
+		*q_entry = NULL;
 
 		if (priv->state != STOPPING)
 			can_run = rcar_vin_fill_hw_slot(priv);
-- 
1.9.1

Re: [Linux-kernel] [PATCH 0/4] rcar_vin: fix soc_camera WARN_ON() issues.

[Ben Dooks]

On 07/07/14 17:37, Ian Molton wrote:
> This patch series provides fixes that allow the rcar_vin driver to function
> without triggering dozens of warnings from the videobuf2 and soc_camera layers.
> 
> Patches 2/3 should probably be merged into a single, atomic change, although
> patch 2 does not make the existing situation /worse/ in and of itself.
> 
> Patch 4 does not change the code logic, but is cleaner and less prone to
> breakage caused by furtutre modification. Also, more consistent with the use of
> vb pointers elsewhere in the driver.
> 
> Comments welcome!

You should have probably CC:d the original authors
as well as the linux-sh list and possibly Magnus and
Horms.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius