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

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

[Ian Molton]

Resent to include the author and a couple of other interested parties :)

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 stoppin

[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

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

[Sergei Shtylyov]

On 07/08/2014 01:41 PM, Ian Molton wrote:

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

    Fixed kernel WARNINGs for me! \o/
    Ian, perhaps it makes sense for me to take these patches into my hands?

WBR, Sergei

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

[Ian Molton]

>     Fixed kernel WARNINGs for me! \o/
>     Ian, perhaps it makes sense for me to take these patches into my hands?

I'm planning to respin these tomorrow - is that OK? I have test hardware with two different frontends here.

-Ian

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

[Sergei Shtylyov]

Hello.

On 08/13/2014 09:30 PM, Ian Molton wrote:

>>      Fixed kernel WARNINGs for me! \o/
>>      Ian, perhaps it makes sense for me to take these patches into my hands?

> I'm planning to respin these tomorrow - is that OK?

    Yes.

> I have test hardware with two different frontends here.

    I'm sorry, what do you mean by frontends?

> -Ian

WBR, Sergei

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

[Guennadi Liakhovetski]

Hi Ian,

On Wed, 13 Aug 2014, Ian Molton wrote:

> 
> >     Fixed kernel WARNINGs for me! \o/
> >     Ian, perhaps it makes sense for me to take these patches into my hands?
> 
> I'm planning to respin these tomorrow - is that OK? I have test hardware with two different frontends here.

Any progress on this?

Thanks
Guennadi

> -Ian

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

[Hans Verkuil]

On 07/08/2014 11:41 AM, Ian Molton wrote:
> 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);

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.

Regards,

	Hans

>  	spin_unlock_irq(&priv->lock);
> 

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

[Guennadi Liakhovetski]

Hi Ian,

Apparently, I wasn't quite right in my reply to patch 1 from this series. 
It seems to be related to this one and to Hans' comment below. Taking it 
into account, it seems you shouldn't remove cancelled active buffers from 
the list without calling vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR) 
for them. So, I think you should modify your patch 1 to either leave those 
buffers on the list and report them "done" here, or call "done" for them 
when removing from the list. I think I'd rather go with the latter option 
to not keep failed buffers on the list and rely, that the vb2 core will 
stop streaming soon. Hans, do you agree?

Thanks
Guennadi

On Mon, 4 Aug 2014, Hans Verkuil wrote:

> On 07/08/2014 11:41 AM, Ian Molton wrote:
> > 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);
> 
> 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.
> 
> Regards,
> 
> 	Hans
> 
> >  	spin_unlock_irq(&priv->lock);
> > 
> 

[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

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

[Sergei Shtylyov]

Hello.

On 07/08/2014 01:41 PM, Ian Molton wrote:

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

    Hm, my spell checker trips on "finalising"...

> 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
[...]
> @@ -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);

    This seems like a random whitespace change. This empty should be present.

[...]
> @@ -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);
> +

    Also quite a random "drove-by" change.

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

WBR, Sergei

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

[Simon Horman]

On Tue, Jul 08, 2014 at 08:09:58PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/08/2014 01:41 PM, Ian Molton wrote:
> 
> >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.
> 
>    Hm, my spell checker trips on "finalising"...

I believe it is a valid spelling of the word in question.

> 
> >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
> [...]
> >@@ -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);
> 
>    This seems like a random whitespace change. This empty should be present.
> 
> [...]
> >@@ -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);
> >+
> 
>    Also quite a random "drove-by" change.
> 
> >  	spin_unlock_irq(&priv->lock);
> >  }
> 
> WBR, Sergei
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Ian Molton]

On Tue, 08 Jul 2014 20:09:58 +0400
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

> Hello.

Hi,

> > 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
> [...]
> > @@ -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);
> 
>     This seems like a random whitespace change. This empty should be present.

Agreed.

> [...]
> > @@ -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);
> > +
> 
>     Also quite a random "drove-by" change.

Agreed.

Any further comments? If not, I can re-spin this ready for upstreaming.


-- 
Ian Molton <ian.molton@codethink.co.uk>

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

[Sergei Shtylyov]

Hello.

On 07/10/2014 02:15 PM, Ian Molton wrote:

>>> 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
>> [...]
>>> @@ -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);

>>      This seems like a random whitespace change. This empty should be present.

> Agreed.

>> [...]
>>> @@ -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);
>>> +
>>
>>      Also quite a random "drove-by" change.

> Agreed.

> Any further comments? If not, I can re-spin this ready for upstreaming.

    There has been no further comments but you've never re-appeared. :-(
Now I'm about to test these patches...

WBR, Sergei

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

[Hans Verkuil]

On 07/08/2014 11:41 AM, Ian Molton wrote:
> 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);
> +

Why on earth would videobuf_release call stop_streaming()?

You start streaming in the start_streaming op, not in the buf_queue op. If you
need a certain minimum of buffers before start_streaming can be called, then just
set min_buffers_needed in struct vb2_queue.

And stop streaming happens in stop_streaming. The various vb2 queue ops should just
do what the op name says. That way everything works nicely together and it makes
your driver much easier to understand.

Sorry I am late in reviewing this, but I only now stumbled on these patches.

Regards,

	Hans

>  		/*
>  		 * 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);
>  }
>  
> 

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

[Ian Molton]

On Mon, 04 Aug 2014 23:27:24 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> > +/*
> > + * 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);
> > +
> 
> Why on earth would videobuf_release call stop_streaming()?

It doesn't. But it appears it can be called whilst the driver still possesses the buffer, in which case, the driver (as was) would request capture stop, and wait for the buffer to be returned from the driver.

This logic here has not been changed, merely moved out to an appropriately named function, so that it can be re-used in rcar_vin_stop_streaming().

> You start streaming in the start_streaming op, not in the buf_queue op. If you
> need a certain minimum of buffers before start_streaming can be called, then just
> set min_buffers_needed in struct vb2_queue.

I can submit an additional patch to correct this behaviour in the rcar_vin driver, if that would be helpful?

> And stop streaming happens in stop_streaming. The various vb2 queue ops should just
> do what the op name says. That way everything works nicely together and it makes
> your driver much easier to understand.

Agreed. It was particularly difficult to understand WTH this driver was doing.

> Sorry I am late in reviewing this, but I only now stumbled on these patches.

Thanks for the review!

-Ian

> >  		/*
> >  		 * 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);
> >  }
> >  
> > 
> 
> 
> 


-- 
Ian Molton <ian.molton@codethink.co.uk>

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

[Hans Verkuil]

On 08/11/2014 01:23 PM, Ian Molton wrote:
> On Mon, 04 Aug 2014 23:27:24 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>>> +/*
>>> + * 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);
>>> +
>>
>> Why on earth would videobuf_release call stop_streaming()?
> 
> It doesn't. But it appears it can be called whilst the driver still
> possesses the buffer, in which case, the driver (as was) would
> request capture stop, and wait for the buffer to be returned from the
> driver.
> 
> This logic here has not been changed, merely moved out to an
> appropriately named function, so that it can be re-used in
> rcar_vin_stop_streaming().
> 
>> You start streaming in the start_streaming op, not in the buf_queue op. If you
>> need a certain minimum of buffers before start_streaming can be called, then just
>> set min_buffers_needed in struct vb2_queue.
> 
> I can submit an additional patch to correct this behaviour in the rcar_vin driver, if that would be helpful?

Please do. The vb2 framework is great, but not if try to second-guess it.
I can guarantee that the current approach will fail in all sorts of subtle
ways. Frankly, I'm surprised it works at all!
 
>> And stop streaming happens in stop_streaming. The various vb2 queue ops should just
>> do what the op name says. That way everything works nicely together and it makes
>> your driver much easier to understand.
> 
> Agreed. It was particularly difficult to understand WTH this driver was doing.

If you can, take kernel 3.15 or 3.16, enable CONFIG_VIDEO_ADV_DEBUG and then
rewrite that code. A good template for the vb2 part might be
Documentation/video4linux/v4l2-pci-skeleton.c.

Looking at the rcar code I think that you only have to implement buf_queue and
start/stop_streaming and can ditch buf_init/cleanup altogether. The stop_streaming
op is the crucial one since that's where you have to stop the DMA, wait (if needed)
for the hardware to access any remaining buffers and return those buffers to the
vb2 framework.

If you try that and test with CONFIG_VIDEO_ADV_DEBUG on, then the vb2 instrumentation
I added will tell you if there are any remaining issues. Also test what happens if
start_streaming fails (assuming it can fail).

I also recommend testing with the v4l2-compliance tool from v4l-utils.git. See what
works/breaks.

If you have questions, don't hesitate to contact me.

Regards,

	Hans

> 
>> Sorry I am late in reviewing this, but I only now stumbled on these patches.
> 
> Thanks for the review!
> 
> -Ian
> 
>>>  		/*
>>>  		 * 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);
>>>  }
>>>  
>>>
>>
>>
>>
> 
> 

[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: Resend: [PATCH 0/4] rcar_vin: fix soc_camera WARN_ON() issues.

[Sergei Shtylyov]

Hello.

On 07/08/2014 01:41 PM, Ian Molton wrote:

> Resent to include the author and a couple of other interested parties :)

    Thank you (despite you didn't include me :-).

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

    Perhaps it's worth to just swap patches 2 & 3...

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

    It's not a good practice to post fixes and clean-ups in a single series.

> Comments welcome!

    I've just tested the whole series, so here's my:

Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

WBR, Sergei