[1/5] vb2: redesign the stop_streaming() callback and make it obligatory
Commit Message
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(-)
Comments
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
@@ -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);
@@ -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);
};