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

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

[Ben Hutchings]

This is a re-submission of patches previously sent and archived at
<http://thread.gmane.org/gmane.linux.ports.sh.devel/37184/>.  Will has
rebased onto 3.18 and added a further patch to address Hans' review
comments.

The driver continues to works for single frame capture, and no longer
provokes a WARNing.  However, video capture has regressed (a gstreamer
capture pipeline yields a zero-length file).

Ben.

Ian Molton (4):
  media: rcar_vin: Dont aggressively retire buffers
  media: rcar_vin: Ensure all in-flight buffers are returned to error
    state before stopping.
  media: rcar_vin: Fix race condition terminating stream
  media: rcar_vin: Clean up rcar_vin_irq

William Towle (1):
  media: rcar_vin: move buffer management to .stop_streaming handler

 drivers/media/platform/soc_camera/rcar_vin.c |  109 ++++++++++++++------------
 1 file changed, 59 insertions(+), 50 deletions(-)

-- 
1.7.10.4

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

[Ben Hutchings]

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

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 8d8438b..773de53 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -496,17 +496,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.7.10.4

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

[Ben Hutchings]

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

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 773de53..7069176 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -516,8 +516,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.7.10.4

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

[Ben Hutchings]

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

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 7069176..b234e57 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -458,6 +458,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);
@@ -465,7 +488,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? */
@@ -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
@@ -520,12 +530,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.7.10.4

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

[Ben Hutchings]

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

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 b234e57..20dbedf 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -557,7 +557,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);
@@ -576,17 +575,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.7.10.4

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

[Ben Hutchings]

From: William Towle <william.towle@codethink.co.uk>

Move the buffer state test in the .buf_cleanup handler into
.stop_streaming so that a) the vb2_queue API is not subverted, and
b) tracking of active-state buffers via priv->queue_buf[] is handled
as early as is possible

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

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 20dbedf..bf60074 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -486,28 +486,8 @@ 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
-		 */
-		priv->queue_buf[i] = NULL;
-	}
+	spin_lock_irq(&priv->lock);
 
 	list_del_init(to_buf_list(vb));
 
@@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
 	rcar_vin_wait_stop_streaming(priv);
 
 	for (i = 0; i < vq->num_buffers; ++i)
-		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
+		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
+			int j;
+
+			/*  Is this a buffer we have told the
+			 *  hardware about? Update the associated
+			 *  list, if so
+			 */
+			for (j = 0; j < MAX_BUFFER_NUM; j++) {
+				if (priv->queue_buf[j] == vq->bufs[i]) {
+					priv->queue_buf[j] = NULL;
+				}
+			}
 			vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR);
+		}
 
 	list_for_each_safe(buf_head, tmp, &priv->capture)
 		list_del_init(buf_head);
-- 
1.7.10.4

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Sergei Shtylyov]

Hello.

On 12/18/2014 05:50 PM, Ben Hutchings wrote:

> From: William Towle <william.towle@codethink.co.uk>

> Move the buffer state test in the .buf_cleanup handler into
> .stop_streaming so that a) the vb2_queue API is not subverted, and
> b) tracking of active-state buffers via priv->queue_buf[] is handled
> as early as is possible

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

> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 20dbedf..bf60074 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
[...]
> @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
>   	rcar_vin_wait_stop_streaming(priv);
>
>   	for (i = 0; i < vq->num_buffers; ++i)
> -		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
> +		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> +			int j;
> +
> +			/*  Is this a buffer we have told the
> +			 *  hardware about? Update the associated
> +			 *  list, if so
> +			 */
> +			for (j = 0; j < MAX_BUFFER_NUM; j++) {
> +				if (priv->queue_buf[j] == vq->bufs[i]) {
> +					priv->queue_buf[j] = NULL;
> +				}

    Don't need {} here.

> +			}
>   			vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR);
> +		}

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 12/18/2014 05:47 PM, Ben Hutchings wrote:

> This is a re-submission of patches previously sent and archived at
> <http://thread.gmane.org/gmane.linux.ports.sh.devel/37184/>.  Will has
> rebased onto 3.18 and added a further patch to address Hans' review
> comments.

> The driver continues to works for single frame capture, and no longer
> provokes a WARNing.  However, video capture has regressed (a gstreamer
> capture pipeline yields a zero-length file).

> Ben.

> Ian Molton (4):
>    media: rcar_vin: Dont aggressively retire buffers
>    media: rcar_vin: Ensure all in-flight buffers are returned to error
>      state before stopping.
>    media: rcar_vin: Fix race condition terminating stream
>    media: rcar_vin: Clean up rcar_vin_irq

> William Towle (1):
>    media: rcar_vin: move buffer management to .stop_streaming handler

    Having actual fixes and clean-ups in a single series is not a good idea...

WBR, Sergei

Re: [RFC PATCH 3/5] media: rcar_vin: Fix race condition terminating stream

[Sergei Shtylyov]

Hello.

On 12/18/2014 05:49 PM, Ben Hutchings wrote:

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

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

    No need for the leading space.

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

    Hm, perhaps for the sake of bisection, these 2 patches need to be merged?

> 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 7069176..b234e57 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
[...]
> @@ -465,7 +488,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;
> -

    Unrelated white space change. Moreover, there should be an empty line 
after declarations.

>   	spin_lock_irq(&priv->lock);
>
>   	/* Is the buffer in use by the VIN hardware? */
[...]
> @@ -520,12 +530,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);
> +

    Also seems like unrelated whitespace cleanup.

>   	spin_unlock_irq(&priv->lock);
>   }

WBR, Sergei

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

[Sakari Ailus]

Hi Ben,

On Thu, Dec 18, 2014 at 02:49:33PM +0000, Ben Hutchings wrote:
> From: Ian Molton <ian.molton@codethink.co.uk>
> 
> 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 773de53..7069176 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -516,8 +516,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);

I'd use the driver's own queued buffer list to access the queued buffers,
you alread loop over that just below your own change.

-- 
Regards,

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

Re: [RFC PATCH 1/5] media: rcar_vin: Dont aggressively retire buffers

[Guennadi Liakhovetski]

Hi Ben,

Sorry for a long delay. The patch looks good to me, although I'll extend 
it a bit by also removing the comment above the deleted loop - it's no 
longer relevant.

Thanks
Guennadi

On Thu, 18 Dec 2014, Ben Hutchings wrote:

> From: Ian Molton <ian.molton@codethink.co.uk>
> 
> 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 8d8438b..773de53 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -496,17 +496,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.7.10.4
> 
> 
> 
> 

Re: [RFC PATCH 3/5] media: rcar_vin: Fix race condition terminating stream

[Guennadi Liakhovetski]

On Thu, 18 Dec 2014, Sergei Shtylyov wrote:

> Hello.
> 
> On 12/18/2014 05:49 PM, Ben Hutchings wrote:
> 
> > From: Ian Molton <ian.molton@codethink.co.uk>
> 
> > This patch fixes a race condition whereby a frame being captured may
> > generate an
> >   interrupt between requesting capture to halt and freeing buffers.
> 
>    No need for the leading space.
> 
> > This condition is exposed by the earlier patch that explicitly calls
> > vb2_buffer_done() during stop streaming.
> 
>    Hm, perhaps for the sake of bisection, these 2 patches need to be merged?

+1. Please, don't introduce a bug in one patch to fix it in a later one.

Thanks
Guennadi

> 
> > 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 7069176..b234e57 100644
> > --- a/drivers/media/platform/soc_camera/rcar_vin.c
> > +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> [...]
> > @@ -465,7 +488,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;
> > -
> 
>    Unrelated white space change. Moreover, there should be an empty line after
> declarations.
> 
> >   	spin_lock_irq(&priv->lock);
> > 
> >   	/* Is the buffer in use by the VIN hardware? */
> [...]
> > @@ -520,12 +530,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);
> > +
> 
>    Also seems like unrelated whitespace cleanup.
> 
> >   	spin_unlock_irq(&priv->lock);
> >   }
> 
> WBR, Sergei
> 

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Guennadi Liakhovetski]

On Thu, 18 Dec 2014, Ben Hutchings wrote:

> From: William Towle <william.towle@codethink.co.uk>
> 
> Move the buffer state test in the .buf_cleanup handler into
> .stop_streaming so that a) the vb2_queue API is not subverted, and
> b) tracking of active-state buffers via priv->queue_buf[] is handled
> as early as is possible

Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
way to get from the present state to the destination. They all have to be 
merged IMHO. 

Further, looking at vb2, I don't think active buffers should ever stay 
active until .buf_cleanup() is called. I think rcar_vin's 
.stop_streaming() should stop the hardware, wait until it's stopped, then 
walk all queued buffers and return errors for them, while removing them 
from the list at the same time. Then I don't think a .buf_cleanup() method 
is needed there at all.

Thanks
Guennadi

> 
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |   36 ++++++++++----------------
>  1 file changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 20dbedf..bf60074 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -486,28 +486,8 @@ 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
> -		 */
> -		priv->queue_buf[i] = NULL;
> -	}
> +	spin_lock_irq(&priv->lock);
>  
>  	list_del_init(to_buf_list(vb));
>  
> @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
>  	rcar_vin_wait_stop_streaming(priv);
>  
>  	for (i = 0; i < vq->num_buffers; ++i)
> -		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
> +		if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> +			int j;
> +
> +			/*  Is this a buffer we have told the
> +			 *  hardware about? Update the associated
> +			 *  list, if so
> +			 */
> +			for (j = 0; j < MAX_BUFFER_NUM; j++) {
> +				if (priv->queue_buf[j] == vq->bufs[i]) {
> +					priv->queue_buf[j] = NULL;
> +				}
> +			}
>  			vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR);
> +		}
>  
>  	list_for_each_safe(buf_head, tmp, &priv->capture)
>  		list_del_init(buf_head);
> -- 
> 1.7.10.4
> 
> 
> 

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Ben Hutchings]

On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote:
> On Thu, 18 Dec 2014, Ben Hutchings wrote:
> 
> > From: William Towle <william.towle@codethink.co.uk>
> > 
> > Move the buffer state test in the .buf_cleanup handler into
> > .stop_streaming so that a) the vb2_queue API is not subverted, and
> > b) tracking of active-state buffers via priv->queue_buf[] is handled
> > as early as is possible
> 
> Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
> way to get from the present state to the destination. They all have to be 
> merged IMHO. 
[...]

Well, I thought that too.  Will's submission from last week has that
change:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009

Ben.

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Guennadi Liakhovetski]

On Mon, 19 Jan 2015, Ben Hutchings wrote:

> On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote:
> > On Thu, 18 Dec 2014, Ben Hutchings wrote:
> > 
> > > From: William Towle <william.towle@codethink.co.uk>
> > > 
> > > Move the buffer state test in the .buf_cleanup handler into
> > > .stop_streaming so that a) the vb2_queue API is not subverted, and
> > > b) tracking of active-state buffers via priv->queue_buf[] is handled
> > > as early as is possible
> > 
> > Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange 
> > way to get from the present state to the destination. They all have to be 
> > merged IMHO. 
> [...]
> 
> Well, I thought that too.  Will's submission from last week has that
> change:
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009

Hm... interesting, why I didn't get those mails in my INBOX, although I do 
see myself in CC... Only got them from the list, and no, I don't have 
"avoid copies" enabled.

Anyway, yes, that looks better! But I would still consider keeping buffers 
on the list in .buf_clean(), in which case you can remove it. And walk the 
list instead of the VB2 internal buffer array, as others have pointed out.

Thanks
Guennadi

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[William Towle]

On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:

>>> On Thu, 18 Dec 2014, Ben Hutchings wrote:
>> Well, I thought that too.  Will's submission from last week has that
>> change:
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009

> Anyway, yes, that looks better! But I would still consider keeping buffers
> on the list in .buf_clean(), in which case you can remove it. And walk the
> list instead of the VB2 internal buffer array, as others have pointed out.

Hi Guennadi,
   Thanks for the clarification. Ian (when he was with us) did say "it
was particularly difficult to understand WTH this driver was doing".

   Regarding your first point, if it's safe to skip the actions left
in rcar_vin_videobuf_release() then I will do a further rework to
remove it completely.

   Regarding your second, in the patchset Ben linked to above we think
we have the appropriate loops: a for loop for queue_buf[], and
list_for_each_safe() for anything left in priv->capture; this is
consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
pointers unlinked from priv->capture. This in turn suggests that we
are right not to call list_del_init() in both of
rcar_vin_stop_streaming()'s loops ... as long as I've correctly
interpreted the code and everyone's feedback thus far.


Cheers,
   Wills.

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Guennadi Liakhovetski]

Hi,

On Mon, 19 Jan 2015, William Towle wrote:

> 
> On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:
> 
> > > > On Thu, 18 Dec 2014, Ben Hutchings wrote:
> > > Well, I thought that too.  Will's submission from last week has that
> > > change:
> > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009
> 
> > Anyway, yes, that looks better! But I would still consider keeping buffers
> > on the list in .buf_clean(), in which case you can remove it. And walk the
> > list instead of the VB2 internal buffer array, as others have pointed out.
> 
> Hi Guennadi,
>   Thanks for the clarification. Ian (when he was with us) did say "it
> was particularly difficult to understand WTH this driver was doing".
> 
>   Regarding your first point, if it's safe to skip the actions left
> in rcar_vin_videobuf_release() then I will do a further rework to
> remove it completely.
> 
>   Regarding your second, in the patchset Ben linked to above we think
> we have the appropriate loops: a for loop for queue_buf[], and
> list_for_each_safe() for anything left in priv->capture; this is
> consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
> pointers unlinked from priv->capture. This in turn suggests that we
> are right not to call list_del_init() in both of
> rcar_vin_stop_streaming()'s loops ... as long as I've correctly
> interpreted the code and everyone's feedback thus far.

I'm referring to this comment by Hans Verkuil of 14 August last year:

> I'm assuming all buffers that are queued to the driver via buf_queue() are
> linked into priv->capture. So you would typically call vb2_buffer_done
> when you are walking that list:
> 
> 	list_for_each_safe(buf_head, tmp, &priv->capture) {
> 		// usually you go from buf_head to the real buffer struct
> 		// containing a vb2_buffer struct
> 		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> 		list_del_init(buf_head);
> 	}
> 
> Please use this rather than looking into internal vb2_queue 
> datastructures.

I think, that's the right way to implement that clean up loop.

Thanks
Guennadi

Re: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[Hans Verkuil]

On 01/19/2015 03:11 PM, William Towle wrote:
> 
> On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:
> 
>>>> On Thu, 18 Dec 2014, Ben Hutchings wrote:
>>> Well, I thought that too.  Will's submission from last week has that
>>> change:
>>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009
> 
>> Anyway, yes, that looks better! But I would still consider keeping buffers
>> on the list in .buf_clean(), in which case you can remove it. And walk the
>> list instead of the VB2 internal buffer array, as others have pointed out.
> 
> Hi Guennadi,
>    Thanks for the clarification. Ian (when he was with us) did say "it
> was particularly difficult to understand WTH this driver was doing".
> 
>    Regarding your first point, if it's safe to skip the actions left
> in rcar_vin_videobuf_release() then I will do a further rework to
> remove it completely.

Yes, that's safe. Just remove it altogether.

The buf_init and buf_release ops are matching ops that are normally only
used if you have to do per-buffer initialization and/or release. These
are only called when the buffer memory changes. In most drivers including
this one it's not needed at all.

The same is true for rcar_vin_videobuf_init: it's pointless since the
list initialization is done implicitly when you add the buffer to a
list with list_add_tail(). Just drop the function.

Regards,

	Hans

> 
>    Regarding your second, in the patchset Ben linked to above we think
> we have the appropriate loops: a for loop for queue_buf[], and
> list_for_each_safe() for anything left in priv->capture; this is
> consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
> pointers unlinked from priv->capture. This in turn suggests that we
> are right not to call list_del_init() in both of
> rcar_vin_stop_streaming()'s loops ... as long as I've correctly
> interpreted the code and everyone's feedback thus far.
> 
> 
> Cheers,
>    Wills.
> --
> 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: [RFC PATCH 5/5] media: rcar_vin: move buffer management to .stop_streaming handler

[William Towle]

On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote:
> On Mon, 19 Jan 2015, William Towle wrote:
>>   in the patchset Ben linked to above we think
>> we have the appropriate loops: a for loop for queue_buf[], and
>> list_for_each_safe() for anything left in priv->capture; this is
>> consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with
>> pointers unlinked from priv->capture. This in turn suggests that we
>> are right not to call list_del_init() in both of
>> rcar_vin_stop_streaming()'s loops ... as long as I've correctly
>> interpreted the code and everyone's feedback thus far.
>
> I'm referring to this comment by Hans Verkuil of 14 August last year:
>
>> I'm assuming all buffers that are queued to the driver via buf_queue() are
>> linked into priv->capture. So you would typically call vb2_buffer_done
>> when you are walking that list:
>>
>> 	list_for_each_safe(buf_head, tmp, &priv->capture) {
>> 		// usually you go from buf_head to the real buffer struct
>> 		// containing a vb2_buffer struct
>> 		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>> 		list_del_init(buf_head);
>> 	}
>>
>> Please use this rather than looking into internal vb2_queue
>> datastructures.
>
> I think, that's the right way to implement that clean up loop.

Hi,
   I was describing the code in my latest patch; we start with
rcar_vin_stop_streaming() having a list_for_each_safe() loop like
that above, and leave that loop in place but add statements to it.

   I add another loop to rcar_vin_stop_streaming() [which you will
have seen -in the patch Ben published in my absence over Christmas-
was particularly inelegant initially], but it can't be rewritten in
the same way.  The new version is undeniably neater, though.

   We believe the latest patches take Hans' comment above into
account properly, and we are looking into his latest suggestion at
the moment.

Thanks again,
   Wills.

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