| Message ID | 1301874670-14833-2-git-send-email-pawel@osciak.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hello, On Monday, April 04, 2011 1:51 AM Pawel Osciak wrote: > Drivers are now required to implement the stop_streaming() callback > to ensure that all ongoing hardware operations are finished and their > ownership of buffers is ceded. > Drivers do not have to call vb2_buffer_done() for each buffer they own > anymore. > Also remove the return value from the callback. > > Signed-off-by: Pawel Osciak <pawel@osciak.com> > --- > drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- > include/media/videobuf2-core.h | 16 +++++++--------- > 2 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 6e69584..59d5e8b 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > struct vb2_queue *q = vb->vb2_queue; > unsigned long flags; > > + if (atomic_read(&q->queued_count) == 0) > + return; > + > if (vb->state != VB2_BUF_STATE_ACTIVE) > return; > > @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > unsigned int i; > > /* > - * Tell driver to stop all transactions and release all queued > + * Tell the driver to stop all transactions and release all queued > * buffers. > */ > if (q->streaming) > call_qop(q, stop_streaming, q); > + > + /* > + * All buffers should now not be in use by the driver anymore, but we > + * have to manually set queued_count to 0, as the driver was not > + * required to call vb2_buffer_done() from stop_streaming() for all > + * buffers it had queued. > + */ > q->streaming = 0; > + atomic_set(&q->queued_count, 0); If you removed the need to call vb2_buffer_done() then you must also call wake_up_all(&q->done_wq) to wake any other threads/processes that might be sleeping waiting for buffers. > > /* > * Remove all buffers from videobuf's list... > @@ -1197,7 +1208,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > wake_up_all(&q->done_wq); > > /* > - * Reinitialize all buffers for next use. > + * Reinitialize all buffers for future use. > */ > for (i = 0; i < q->num_buffers; ++i) > q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > @@ -1440,6 +1451,7 @@ int vb2_queue_init(struct vb2_queue *q) > > BUG_ON(!q->ops->queue_setup); > BUG_ON(!q->ops->buf_queue); > + BUG_ON(!q->ops->stop_streaming); > > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index f3bdbb2..8115fe9 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -184,7 +184,7 @@ struct vb2_buffer { > > /** > * struct vb2_ops - driver-specific callbacks to be implemented by the driver > - * Required: queue_setup, buf_queue. The rest is optional. > + * Required: queue_setup, buf_queue, stop_streaming. The rest is optional. > * > * @queue_setup: used to negotiate queue parameters between the userspace > * and the driver; called before memory allocation; > @@ -231,13 +231,11 @@ struct vb2_buffer { > * the driver before streaming begins (such as enabling > * the device); > * @stop_streaming: called when the 'streaming' state must be disabled; > - * drivers should stop any DMA transactions here (or wait > - * until they are finished) and give back all the buffers > - * received via buf_queue() by calling vb2_buffer_done() > - * for each of them; > - * drivers can use the vb2_wait_for_all_buffers() function > - * here to wait for asynchronous completion events that > - * call vb2_buffer_done(), such as ISRs; > + * drivers should stop any DMA transactions here (or wait > + * until they are finished) before returning; > + * drivers can use the vb2_wait_for_all_buffers() function > + * here to wait for asynchronous completion events, such > + * as ISRs; > * @buf_queue: passes a buffer to the driver; the driver may start > * a hardware operation on that buffer; this callback > * MUST return immediately, i.e. it may NOT wait for > @@ -259,7 +257,7 @@ struct vb2_ops { > void (*buf_cleanup)(struct vb2_buffer *vb); > > int (*start_streaming)(struct vb2_queue *q); > - int (*stop_streaming)(struct vb2_queue *q); > + void (*stop_streaming)(struct vb2_queue *q); > > void (*buf_queue)(struct vb2_buffer *vb); > }; Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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
On Sun, 3 Apr 2011, Pawel Osciak wrote: > Drivers are now required to implement the stop_streaming() callback > to ensure that all ongoing hardware operations are finished and their > ownership of buffers is ceded. > Drivers do not have to call vb2_buffer_done() for each buffer they own > anymore. > Also remove the return value from the callback. > > Signed-off-by: Pawel Osciak <pawel@osciak.com> > --- > drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- > include/media/videobuf2-core.h | 16 +++++++--------- > 2 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 6e69584..59d5e8b 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > struct vb2_queue *q = vb->vb2_queue; > unsigned long flags; > > + if (atomic_read(&q->queued_count) == 0) > + return; > + > if (vb->state != VB2_BUF_STATE_ACTIVE) > return; > > @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > unsigned int i; > > /* > - * Tell driver to stop all transactions and release all queued > + * Tell the driver to stop all transactions and release all queued > * buffers. > */ > if (q->streaming) > call_qop(q, stop_streaming, q); > + > + /* > + * All buffers should now not be in use by the driver anymore, but we > + * have to manually set queued_count to 0, as the driver was not > + * required to call vb2_buffer_done() from stop_streaming() for all > + * buffers it had queued. > + */ > q->streaming = 0; > + atomic_set(&q->queued_count, 0); > > /* > * Remove all buffers from videobuf's list... > @@ -1197,7 +1208,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > wake_up_all(&q->done_wq); > > /* > - * Reinitialize all buffers for next use. > + * Reinitialize all buffers for future use. > */ > for (i = 0; i < q->num_buffers; ++i) > q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > @@ -1440,6 +1451,7 @@ int vb2_queue_init(struct vb2_queue *q) > > BUG_ON(!q->ops->queue_setup); > BUG_ON(!q->ops->buf_queue); > + BUG_ON(!q->ops->stop_streaming); > > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index f3bdbb2..8115fe9 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -184,7 +184,7 @@ struct vb2_buffer { > > /** > * struct vb2_ops - driver-specific callbacks to be implemented by the driver > - * Required: queue_setup, buf_queue. The rest is optional. > + * Required: queue_setup, buf_queue, stop_streaming. The rest is optional. > * > * @queue_setup: used to negotiate queue parameters between the userspace > * and the driver; called before memory allocation; > @@ -231,13 +231,11 @@ struct vb2_buffer { > * the driver before streaming begins (such as enabling > * the device); > * @stop_streaming: called when the 'streaming' state must be disabled; > - * drivers should stop any DMA transactions here (or wait > - * until they are finished) and give back all the buffers > - * received via buf_queue() by calling vb2_buffer_done() > - * for each of them; > - * drivers can use the vb2_wait_for_all_buffers() function > - * here to wait for asynchronous completion events that > - * call vb2_buffer_done(), such as ISRs; > + * drivers should stop any DMA transactions here (or wait > + * until they are finished) before returning; > + * drivers can use the vb2_wait_for_all_buffers() function > + * here to wait for asynchronous completion events, such > + * as ISRs; > * @buf_queue: passes a buffer to the driver; the driver may start > * a hardware operation on that buffer; this callback > * MUST return immediately, i.e. it may NOT wait for > @@ -259,7 +257,7 @@ struct vb2_ops { > void (*buf_cleanup)(struct vb2_buffer *vb); > > int (*start_streaming)(struct vb2_queue *q); > - int (*stop_streaming)(struct vb2_queue *q); > + void (*stop_streaming)(struct vb2_queue *q); Won't compilation break after this patch with "assignment from incompatible pointer type?" I know, it's only until it is fixed by the follow-up patches, but normally we're trying to avoid such bisection breakages. Thanks Guennadi > > void (*buf_queue)(struct vb2_buffer *vb); > }; > -- > 1.7.4.2 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On Monday, April 04, 2011 1:51 AM Pawel Osciak wrote: > >> Drivers are now required to implement the stop_streaming() callback >> to ensure that all ongoing hardware operations are finished and their >> ownership of buffers is ceded. >> Drivers do not have to call vb2_buffer_done() for each buffer they own >> anymore. >> Also remove the return value from the callback. >> >> Signed-off-by: Pawel Osciak <pawel@osciak.com> >> --- >> drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- >> include/media/videobuf2-core.h | 16 +++++++--------- >> 2 files changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >> index 6e69584..59d5e8b 100644 >> --- a/drivers/media/video/videobuf2-core.c >> +++ b/drivers/media/video/videobuf2-core.c >> @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> struct vb2_queue *q = vb->vb2_queue; >> unsigned long flags; >> >> + if (atomic_read(&q->queued_count) == 0) >> + return; >> + >> if (vb->state != VB2_BUF_STATE_ACTIVE) >> return; >> >> @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> unsigned int i; >> >> /* >> - * Tell driver to stop all transactions and release all queued >> + * Tell the driver to stop all transactions and release all queued >> * buffers. >> */ >> if (q->streaming) >> call_qop(q, stop_streaming, q); >> + >> + /* >> + * All buffers should now not be in use by the driver anymore, but we >> + * have to manually set queued_count to 0, as the driver was not >> + * required to call vb2_buffer_done() from stop_streaming() for all >> + * buffers it had queued. >> + */ >> q->streaming = 0; >> + atomic_set(&q->queued_count, 0); > > If you removed the need to call vb2_buffer_done() then you must also call > wake_up_all(&q->done_wq) to wake any other threads/processes that might be > sleeping waiting for buffers. True, setting queued_count to 0 is not enough. Hm, I'm wondering why tests on vivi and qv4l2 didn't catch this, it uses poll as well...
Hi again Marek On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On Monday, April 04, 2011 1:51 AM Pawel Osciak wrote: > >> Drivers are now required to implement the stop_streaming() callback >> to ensure that all ongoing hardware operations are finished and their >> ownership of buffers is ceded. >> Drivers do not have to call vb2_buffer_done() for each buffer they own >> anymore. >> Also remove the return value from the callback. >> >> Signed-off-by: Pawel Osciak <pawel@osciak.com> >> --- >> drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- >> include/media/videobuf2-core.h | 16 +++++++--------- >> 2 files changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >> index 6e69584..59d5e8b 100644 >> --- a/drivers/media/video/videobuf2-core.c >> +++ b/drivers/media/video/videobuf2-core.c >> @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> struct vb2_queue *q = vb->vb2_queue; >> unsigned long flags; >> >> + if (atomic_read(&q->queued_count) == 0) >> + return; >> + >> if (vb->state != VB2_BUF_STATE_ACTIVE) >> return; >> >> @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> unsigned int i; >> >> /* >> - * Tell driver to stop all transactions and release all queued >> + * Tell the driver to stop all transactions and release all queued >> * buffers. >> */ >> if (q->streaming) >> call_qop(q, stop_streaming, q); >> + >> + /* >> + * All buffers should now not be in use by the driver anymore, but we >> + * have to manually set queued_count to 0, as the driver was not >> + * required to call vb2_buffer_done() from stop_streaming() for all >> + * buffers it had queued. >> + */ >> q->streaming = 0; >> + atomic_set(&q->queued_count, 0); > > If you removed the need to call vb2_buffer_done() then you must also call > wake_up_all(&q->done_wq) to wake any other threads/processes that might be > sleeping waiting for buffers. You made me doubt myself for a second there, but the patch is correct. There is a call to wake_up_all a few lines below this.
Hello, On Wednesday, April 06, 2011 5:03 AM Pawel Osciak wrote: > On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hello, > > > > On Monday, April 04, 2011 1:51 AM Pawel Osciak wrote: > > > >> Drivers are now required to implement the stop_streaming() callback > >> to ensure that all ongoing hardware operations are finished and their > >> ownership of buffers is ceded. > >> Drivers do not have to call vb2_buffer_done() for each buffer they own > >> anymore. > >> Also remove the return value from the callback. > >> > >> Signed-off-by: Pawel Osciak <pawel@osciak.com> > >> --- > >> drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- > >> include/media/videobuf2-core.h | 16 +++++++--------- > >> 2 files changed, 21 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > >> index 6e69584..59d5e8b 100644 > >> --- a/drivers/media/video/videobuf2-core.c > >> +++ b/drivers/media/video/videobuf2-core.c > >> @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > >> struct vb2_queue *q = vb->vb2_queue; > >> unsigned long flags; > >> > >> + if (atomic_read(&q->queued_count) == 0) > >> + return; > >> + > >> if (vb->state != VB2_BUF_STATE_ACTIVE) > >> return; > >> > >> @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > >> unsigned int i; > >> > >> /* > >> - * Tell driver to stop all transactions and release all queued > >> + * Tell the driver to stop all transactions and release all queued > >> * buffers. > >> */ > >> if (q->streaming) > >> call_qop(q, stop_streaming, q); > >> + > >> + /* > >> + * All buffers should now not be in use by the driver anymore, but we > >> + * have to manually set queued_count to 0, as the driver was not > >> + * required to call vb2_buffer_done() from stop_streaming() for all > >> + * buffers it had queued. > >> + */ > >> q->streaming = 0; > >> + atomic_set(&q->queued_count, 0); > > > > If you removed the need to call vb2_buffer_done() then you must also call > > wake_up_all(&q->done_wq) to wake any other threads/processes that might be > > sleeping waiting for buffers. > > You made me doubt myself for a second there, but the patch is correct. > There is a call to wake_up_all a few lines below this. Yes, I must have been blind or really tired that I've missed it. I'm sorry for the confusion. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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 6e69584..59d5e8b 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -640,6 +640,9 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) struct vb2_queue *q = vb->vb2_queue; unsigned long flags; + if (atomic_read(&q->queued_count) == 0) + return; + if (vb->state != VB2_BUF_STATE_ACTIVE) return; @@ -1178,12 +1181,20 @@ static void __vb2_queue_cancel(struct vb2_queue *q) unsigned int i; /* - * Tell driver to stop all transactions and release all queued + * Tell the driver to stop all transactions and release all queued * buffers. */ if (q->streaming) call_qop(q, stop_streaming, q); + + /* + * All buffers should now not be in use by the driver anymore, but we + * have to manually set queued_count to 0, as the driver was not + * required to call vb2_buffer_done() from stop_streaming() for all + * buffers it had queued. + */ q->streaming = 0; + atomic_set(&q->queued_count, 0); /* * Remove all buffers from videobuf's list... @@ -1197,7 +1208,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) wake_up_all(&q->done_wq); /* - * Reinitialize all buffers for next use. + * Reinitialize all buffers for future use. */ for (i = 0; i < q->num_buffers; ++i) q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; @@ -1440,6 +1451,7 @@ int vb2_queue_init(struct vb2_queue *q) BUG_ON(!q->ops->queue_setup); BUG_ON(!q->ops->buf_queue); + BUG_ON(!q->ops->stop_streaming); INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index f3bdbb2..8115fe9 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -184,7 +184,7 @@ struct vb2_buffer { /** * struct vb2_ops - driver-specific callbacks to be implemented by the driver - * Required: queue_setup, buf_queue. The rest is optional. + * Required: queue_setup, buf_queue, stop_streaming. The rest is optional. * * @queue_setup: used to negotiate queue parameters between the userspace * and the driver; called before memory allocation; @@ -231,13 +231,11 @@ struct vb2_buffer { * the driver before streaming begins (such as enabling * the device); * @stop_streaming: called when the 'streaming' state must be disabled; - * drivers should stop any DMA transactions here (or wait - * until they are finished) and give back all the buffers - * received via buf_queue() by calling vb2_buffer_done() - * for each of them; - * drivers can use the vb2_wait_for_all_buffers() function - * here to wait for asynchronous completion events that - * call vb2_buffer_done(), such as ISRs; + * drivers should stop any DMA transactions here (or wait + * until they are finished) before returning; + * drivers can use the vb2_wait_for_all_buffers() function + * here to wait for asynchronous completion events, such + * as ISRs; * @buf_queue: passes a buffer to the driver; the driver may start * a hardware operation on that buffer; this callback * MUST return immediately, i.e. it may NOT wait for @@ -259,7 +257,7 @@ struct vb2_ops { void (*buf_cleanup)(struct vb2_buffer *vb); int (*start_streaming)(struct vb2_queue *q); - int (*stop_streaming)(struct vb2_queue *q); + void (*stop_streaming)(struct vb2_queue *q); void (*buf_queue)(struct vb2_buffer *vb); };
Drivers are now required to implement the stop_streaming() callback to ensure that all ongoing hardware operations are finished and their ownership of buffers is ceded. Drivers do not have to call vb2_buffer_done() for each buffer they own anymore. Also remove the return value from the callback. Signed-off-by: Pawel Osciak <pawel@osciak.com> --- drivers/media/video/videobuf2-core.c | 16 ++++++++++++++-- include/media/videobuf2-core.h | 16 +++++++--------- 2 files changed, 21 insertions(+), 11 deletions(-)