* rcar_vin video buffer management fixes, v3
@ 2015-01-26 17:08 William Towle
2015-01-26 17:08 ` [PATCH 1/2] media: rcar_vin: helper function for streaming stop William Towle
2015-01-26 17:08 ` [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler William Towle
0 siblings, 2 replies; 5+ messages in thread
From: William Towle @ 2015-01-26 17:08 UTC (permalink / raw)
To: linux-kernel, linux-media, Guennadi Liakhovetski,
Sergei Shtylyov, Hans Verkuil
This is version 3 of the hotfix patchset for video support on Lager,
in which we aim to fix the kernel's warnings about unexpected buffer
states. It replaces the following series:
http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009
This version of the series contains two patches which add a helper
function and then refactors the code where it is called, respectively.
Based on recent feedback, further modification is included in the
second patch where initialise/cleanup routines are removed due to
all significant content having moved elsewhere.
The series comprises:
[PATCH 1/2] media: rcar_vin: helper function for streaming stop
[PATCH 2/2] media: rcar_vin: move buffer management to
Cheers,
Wills
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] media: rcar_vin: helper function for streaming stop
2015-01-26 17:08 rcar_vin video buffer management fixes, v3 William Towle
@ 2015-01-26 17:08 ` William Towle
2015-01-26 17:08 ` [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler William Towle
1 sibling, 0 replies; 5+ messages in thread
From: William Towle @ 2015-01-26 17:08 UTC (permalink / raw)
To: linux-kernel, linux-media, Guennadi Liakhovetski,
Sergei Shtylyov, Hans Verkuil
From: Ian Molton <ian.molton@codethink.co.uk>
The code that tests that capture from a stream has stopped is
presently insufficient and the potential for a race condition
exists where frame capture may generate an interrupt between
requesting the capture process halt and freeing buffers.
This patch refactors code out of rcar_vin_videobuf_release() and
into rcar_vin_wait_stop_streaming(), and ensures there are calls
in places where we need to know that capturing has finished.
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 | 41 +++++++++++++++++---------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 8d8438b..89c409b 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -458,6 +458,28 @@ 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);
@@ -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
@@ -524,8 +534,11 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq)
struct list_head *buf_head, *tmp;
spin_lock_irq(&priv->lock);
+
+ rcar_vin_wait_stop_streaming(priv);
list_for_each_safe(buf_head, tmp, &priv->capture)
list_del_init(buf_head);
+
spin_unlock_irq(&priv->lock);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler
2015-01-26 17:08 rcar_vin video buffer management fixes, v3 William Towle
2015-01-26 17:08 ` [PATCH 1/2] media: rcar_vin: helper function for streaming stop William Towle
@ 2015-01-26 17:08 ` William Towle
1 sibling, 0 replies; 5+ messages in thread
From: William Towle @ 2015-01-26 17:08 UTC (permalink / raw)
To: linux-kernel, linux-media, Guennadi Liakhovetski,
Sergei Shtylyov, Hans Verkuil
This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.
By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.
After the refactoring, the buf_init and buf_cleanup functions were
found to contain only initialisation/release steps as are carried out
elsewhere if omitted; these functions and references were removed.
Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
drivers/media/platform/soc_camera/rcar_vin.c | 62 ++++++--------------------
1 file changed, 13 insertions(+), 49 deletions(-)
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..aa25a3b 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -480,72 +480,36 @@ static void rcar_vin_wait_stop_streaming(struct rcar_vin_priv *priv)
}
}
-static void rcar_vin_videobuf_release(struct vb2_buffer *vb)
+static void rcar_vin_stop_streaming(struct vb2_queue *vq)
{
- struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
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;
+ struct list_head *buf_head, *tmp;
+ int i;
spin_lock_irq(&priv->lock);
+ rcar_vin_wait_stop_streaming(priv);
- /* 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
- */
- for (i = 0; i < MAX_BUFFER_NUM; i++) {
- if (priv->queue_buf[i]) {
- vb2_buffer_done(priv->queue_buf[i],
+ if (priv->queue_buf[i]) {
+ vb2_buffer_done(priv->queue_buf[i],
VB2_BUF_STATE_ERROR);
- priv->queue_buf[i] = NULL;
- }
+ priv->queue_buf[i] = NULL;
}
- } else {
- list_del_init(to_buf_list(vb));
}
- spin_unlock_irq(&priv->lock);
-}
-
-static int rcar_vin_videobuf_init(struct vb2_buffer *vb)
-{
- INIT_LIST_HEAD(to_buf_list(vb));
- return 0;
-}
-
-static void rcar_vin_stop_streaming(struct vb2_queue *vq)
-{
- struct soc_camera_device *icd = soc_camera_from_vb2q(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;
-
- spin_lock_irq(&priv->lock);
-
- rcar_vin_wait_stop_streaming(priv);
- list_for_each_safe(buf_head, tmp, &priv->capture)
+ list_for_each_safe(buf_head, tmp, &priv->capture) {
+ vb2_buffer_done(&list_entry(buf_head,
+ struct rcar_vin_buffer, list)->vb,
+ VB2_BUF_STATE_ERROR);
list_del_init(buf_head);
-
+ }
spin_unlock_irq(&priv->lock);
}
static struct vb2_ops rcar_vin_vb2_ops = {
.queue_setup = rcar_vin_videobuf_setup,
- .buf_init = rcar_vin_videobuf_init,
- .buf_cleanup = rcar_vin_videobuf_release,
.buf_queue = rcar_vin_videobuf_queue,
.stop_streaming = rcar_vin_stop_streaming,
.wait_prepare = soc_camera_unlock,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler
2015-01-16 16:22 [RFC PATCH 0/5] media: rcar_vin: Fixes for buffer management William Towle
@ 2015-01-16 16:22 ` William Towle
0 siblings, 0 replies; 5+ messages in thread
From: William Towle @ 2015-01-16 16:22 UTC (permalink / raw)
To: linux-kernel, linux-media
Cc: Guennadi Liakhovetski, linux-kernel, William Towle,
Sergei Shtylyov, Hans Verkuil
This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.
By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.
Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
drivers/media/platform/soc_camera/rcar_vin.c | 47 +++++++++-----------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..022fa9d 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -485,37 +485,10 @@ 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
- */
- 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));
- }
+ list_del_init(to_buf_list(vb));
spin_unlock_irq(&priv->lock);
}
@@ -532,13 +505,25 @@ 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);
-
rcar_vin_wait_stop_streaming(priv);
- list_for_each_safe(buf_head, tmp, &priv->capture)
- list_del_init(buf_head);
+ 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;
+ }
+ }
+
+ list_for_each_safe(buf_head, tmp, &priv->capture) {
+ vb2_buffer_done(&list_entry(buf_head,
+ struct rcar_vin_buffer, list)->vb,
+ VB2_BUF_STATE_ERROR);
+ list_del_init(buf_head);
+ }
spin_unlock_irq(&priv->lock);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler
2015-01-16 15:14 [PATCH 1/2] media: rcar_vin: Fix race condition terminating stream William Towle
@ 2015-01-16 15:14 ` William Towle
0 siblings, 0 replies; 5+ messages in thread
From: William Towle @ 2015-01-16 15:14 UTC (permalink / raw)
To: linux-kernel, linux-media
This commit moves the "buffer in use" logic from the .buf_cleanup
handler into .stop_streaming, based on advice that this is its
proper logical home.
By ensuring the list of pointers in priv->queue_buf[] is managed
as soon as possible, we avoid warnings concerning buffers in ACTIVE
state when the system cleans up after streaming stops. This fixes a
problem with modification of buffers after their content has been
cleared for passing to userspace.
Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
drivers/media/platform/soc_camera/rcar_vin.c | 47 +++++++++-----------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 89c409b..022fa9d 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -485,37 +485,10 @@ 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
- */
- 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));
- }
+ list_del_init(to_buf_list(vb));
spin_unlock_irq(&priv->lock);
}
@@ -532,13 +505,25 @@ 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);
-
rcar_vin_wait_stop_streaming(priv);
- list_for_each_safe(buf_head, tmp, &priv->capture)
- list_del_init(buf_head);
+ 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;
+ }
+ }
+
+ list_for_each_safe(buf_head, tmp, &priv->capture) {
+ vb2_buffer_done(&list_entry(buf_head,
+ struct rcar_vin_buffer, list)->vb,
+ VB2_BUF_STATE_ERROR);
+ list_del_init(buf_head);
+ }
spin_unlock_irq(&priv->lock);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-26 17:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-26 17:08 rcar_vin video buffer management fixes, v3 William Towle
2015-01-26 17:08 ` [PATCH 1/2] media: rcar_vin: helper function for streaming stop William Towle
2015-01-26 17:08 ` [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler William Towle
-- strict thread matches above, loose matches on Subject: below --
2015-01-16 16:22 [RFC PATCH 0/5] media: rcar_vin: Fixes for buffer management William Towle
2015-01-16 16:22 ` [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler William Towle
2015-01-16 15:14 [PATCH 1/2] media: rcar_vin: Fix race condition terminating stream William Towle
2015-01-16 15:14 ` [PATCH 2/2] media: rcar_vin: move buffer management to .stop_streaming handler William Towle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).