Re: [RFC PATCH 3/5] media: rcar_vin: Fix race condition terminating stream
On Thu, 18 Dec 2014, Sergei Shtylyov wrote: Hello. On 12/18/2014 05:49 PM, Ben Hutchings wrote: From: Ian Molton ian.mol...@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.mol...@codethink.co.uk Signed-off-by: William Towle william.to...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 3/5] media: rcar_vin: Fix race condition terminating stream
From: Ian Molton ian.mol...@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.mol...@codethink.co.uk
Signed-off-by: William Towle william.to...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/5] media: rcar_vin: Fix race condition terminating stream
Hello. On 12/18/2014 05:49 PM, Ben Hutchings wrote: From: Ian Molton ian.mol...@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.mol...@codethink.co.uk Signed-off-by: William Towle william.to...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
