| Message ID | 20110714150934.74777696@bike.lwn.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Thursday, July 14, 2011 11:10 PM Jonathan Corbet wrote: > When user space stops streaming, there may be buffers which have been given > to buf_prepare() and which may or may not have been passed to buf_queue(). > The videobuf2 core simply takes those buffers back; if buf_prepare() does > work that needs cleaning up (like setting up a DMA mapping), that cleanup > will not happen. > > This patch establishes a simple contract with drivers: buffers given to > buf_prepare() will eventually see a buf_finish() call. > > Signed-off-by: Jonathan Corbet <corbet@lwn.net> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/media/video/videobuf2-core.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c > b/drivers/media/video/videobuf2-core.c > index 6ba1461..2ba08ab 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1177,6 +1177,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon); > */ > static void __vb2_queue_cancel(struct vb2_queue *q) > { > + struct vb2_buffer *vb; > unsigned int i; > > /* > @@ -1188,13 +1189,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > q->streaming = 0; > > /* > - * Remove all buffers from videobuf's list... > + * Remove all buffers from videobuf's list... Give the driver > + * a chance to clean them up first, though. > */ > + list_for_each_entry(vb, &q->queued_list, queued_entry) > + call_qop(q, buf_finish, vb); > INIT_LIST_HEAD(&q->queued_list); > /* > * ...and done list; userspace will not receive any buffers it > * has not already dequeued before initiating cancel. > */ > + list_for_each_entry(vb, &q->done_list, done_entry) > + call_qop(q, buf_finish, vb); > INIT_LIST_HEAD(&q->done_list); > wake_up_all(&q->done_wq); > > -- > 1.7.6 Best regards
Hi Jon, There is a misunderstanding here. This patch will call buf_finish for some buffers twice. A buffer does not get removed from queued_list even if it is on the done_list. A buffer gets into the queued_list on qbuf, gets into the done_list after an operation is finished and is removed from both lists on dqbuf. This means that you shouldn't call buf_finish for buffers on both lists, only for all buffers in the queued_list, as it is a superset of both.
Em 24-07-2011 19:10, Pawel Osciak escreveu: > Hi Jon, > There is a misunderstanding here. This patch will call buf_finish for > some buffers twice. A buffer does not get removed from queued_list > even if it is on the done_list. A buffer gets into the queued_list on > qbuf, gets into the done_list after an operation is finished and is > removed from both lists on dqbuf. This means that you shouldn't call > buf_finish for buffers on both lists, only for all buffers in the > queued_list, as it is a superset of both. Is there any update on this? Thanks, Mauro > -- Best regards, Pawel Osciak On Thu, Jul 14, 2011 at 14:09, Jonathan Corbet <corbet@lwn.net> wrote: >> > When user space stops streaming, there may be buffers which have been given >> > to buf_prepare() and which may or may not have been passed to buf_queue(). >> > The videobuf2 core simply takes those buffers back; if buf_prepare() does >> > work that needs cleaning up (like setting up a DMA mapping), that cleanup >> > will not happen. >> > >> > This patch establishes a simple contract with drivers: buffers given to >> > buf_prepare() will eventually see a buf_finish() call. >> > >> > Signed-off-by: Jonathan Corbet <corbet@lwn.net> >> > --- >> > drivers/media/video/videobuf2-core.c | 8 +++++++- >> > 1 files changed, 7 insertions(+), 1 deletions(-) >> > >> > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >> > index 6ba1461..2ba08ab 100644 >> > --- a/drivers/media/video/videobuf2-core.c >> > +++ b/drivers/media/video/videobuf2-core.c >> > @@ -1177,6 +1177,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon); >> > */ >> > static void __vb2_queue_cancel(struct vb2_queue *q) >> > { >> > + struct vb2_buffer *vb; >> > unsigned int i; >> > >> > /* >> > @@ -1188,13 +1189,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> > q->streaming = 0; >> > >> > /* >> > - * Remove all buffers from videobuf's list... >> > + * Remove all buffers from videobuf's list... Give the driver >> > + * a chance to clean them up first, though. >> > */ >> > + list_for_each_entry(vb, &q->queued_list, queued_entry) >> > + call_qop(q, buf_finish, vb); >> > INIT_LIST_HEAD(&q->queued_list); >> > /* >> > * ...and done list; userspace will not receive any buffers it >> > * has not already dequeued before initiating cancel. >> > */ >> > + list_for_each_entry(vb, &q->done_list, done_entry) >> > + call_qop(q, buf_finish, vb); >> > INIT_LIST_HEAD(&q->done_list); >> > wake_up_all(&q->done_wq); >> > >> > -- >> > 1.7.6 >> > >> > -- >> > 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 >> > > -- > 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 > -- 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
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index 6ba1461..2ba08ab 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1177,6 +1177,7 @@ EXPORT_SYMBOL_GPL(vb2_streamon); */ static void __vb2_queue_cancel(struct vb2_queue *q) { + struct vb2_buffer *vb; unsigned int i; /* @@ -1188,13 +1189,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q) q->streaming = 0; /* - * Remove all buffers from videobuf's list... + * Remove all buffers from videobuf's list... Give the driver + * a chance to clean them up first, though. */ + list_for_each_entry(vb, &q->queued_list, queued_entry) + call_qop(q, buf_finish, vb); INIT_LIST_HEAD(&q->queued_list); /* * ...and done list; userspace will not receive any buffers it * has not already dequeued before initiating cancel. */ + list_for_each_entry(vb, &q->done_list, done_entry) + call_qop(q, buf_finish, vb); INIT_LIST_HEAD(&q->done_list); wake_up_all(&q->done_wq);
When user space stops streaming, there may be buffers which have been given to buf_prepare() and which may or may not have been passed to buf_queue(). The videobuf2 core simply takes those buffers back; if buf_prepare() does work that needs cleaning up (like setting up a DMA mapping), that cleanup will not happen. This patch establishes a simple contract with drivers: buffers given to buf_prepare() will eventually see a buf_finish() call. Signed-off-by: Jonathan Corbet <corbet@lwn.net> --- drivers/media/video/videobuf2-core.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)