RE: [PATCH 1/5] [media] vb2: redesign the stop_streaming() callback and make it obligatory
Hello, On Wednesday, April 06, 2011 5:03 AM Pawel Osciak wrote: On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski m.szyprow...@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 pa...@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 RD Center -- 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: [PATCH 1/5] [media] vb2: redesign the stop_streaming() callback and make it obligatory
On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski m.szyprow...@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 pa...@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... -- Best regards, Pawel Osciak -- 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: [PATCH 1/5] [media] vb2: redesign the stop_streaming() callback and make it obligatory
Hi again Marek On Sun, Apr 3, 2011 at 22:49, Marek Szyprowski m.szyprow...@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 pa...@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. -- Best regards, Pawel Osciak -- 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: [PATCH 1/5] [media] vb2: redesign the stop_streaming() callback and make it obligatory
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 pa...@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
RE: [PATCH 1/5] [media] vb2: redesign the stop_streaming() callback and make it obligatory
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 pa...@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 RD Center -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to