| Message ID | 1404812474-7627-4-git-send-email-ian.molton@codethink.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- 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
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 > -- 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
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.
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 -- 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
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); > } > > -- 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
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); > > } > > > > > > >
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); >>> } >>> >>> >> >> >> > > -- 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
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); }