Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted
Hi Ezequiel, On 5/29/18, Ezequiel Garcia wrote: > On Fri, 2018-05-25 at 12:11 +0530, sathyam panda wrote: >> Hello, >> >> On 5/21/18, Ezequiel Garcia wrote: >> > The in-fence implementation involves having a per-buffer fence >> > callback, >> > that triggers on the fence signal. The fence callback is called >> > asynchronously >> > and needs a valid reference to the associated ideobuf2 buffer. >> > >> > Allow this by making the vb2_buffer refcounted, so it can be passed >> > to other contexts. >> > >> >> -Is it really required, because when a queued buffer with an in_fence >> is deallocated, firstly queue is cancelled. >> -And __vb2_dqbuf is called which calls dma_fence_remove_callback. >> -So if fence callback has been called -__vb2_dqbuf will wait to >> acquire fence lock. >> -So during execution of fence callback, buffers and queue are still >> valid. >> -And if __vb2_dqbuf remove callback first ,then dma_fence_signal will >> wait for lock >> - so there won't be any fence callback to call for that buffer when >> dma_fence_signal resumes. >> > > Hi Sathyam, > > Thanks for your review! The refcount is definitely required, > as the fence callback only schedules a workqueue, which is > completely asynchronous with respect to the rest of the > ioctls. > > In particular, the workqueue is not synchronized with > vb2_core_queue_release. > > Also, another subtle detail, dma_fence_remove_callback > can fail to remove the callback. > > Thanks, > Eze > - Sorry, I didn't pay attention to the use of workqueue, thanks for bringing that into my notice. Now it make sense. - I have a doubt about the queue since many driver specific structures have queues which they release with kfree() after calling vb2_queue_release(). - And since vb2_core_queue_release decrements the buffer refcount only once if fence is already signalled, so buffer is still valid, but queue isn't as the memory is deallocated. - So is it safe to access queue in the __qbuf_work. Please correct me if I am wrong. Regards, Sathyam
Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted
Hello, On 5/21/18, Ezequiel Garcia wrote: > The in-fence implementation involves having a per-buffer fence callback, > that triggers on the fence signal. The fence callback is called > asynchronously > and needs a valid reference to the associated ideobuf2 buffer. > > Allow this by making the vb2_buffer refcounted, so it can be passed > to other contexts. > -Is it really required, because when a queued buffer with an in_fence is deallocated, firstly queue is cancelled. -And __vb2_dqbuf is called which calls dma_fence_remove_callback. -So if fence callback has been called -__vb2_dqbuf will wait to acquire fence lock. -So during execution of fence callback, buffers and queue are still valid. -And if __vb2_dqbuf remove callback first ,then dma_fence_signal will wait for lock - so there won't be any fence callback to call for that buffer when dma_fence_signal resumes. > Signed-off-by: Ezequiel Garcia > --- > drivers/media/common/videobuf2/videobuf2-core.c | 27 > ++--- > include/media/videobuf2-core.h | 7 +-- > 2 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c > b/drivers/media/common/videobuf2/videobuf2-core.c > index d3f7bb33a54d..f1feb45c1e37 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -190,6 +190,26 @@ module_param(debug, int, 0644); > static void __vb2_queue_cancel(struct vb2_queue *q); > static void __enqueue_in_driver(struct vb2_buffer *vb); > > +static void __vb2_buffer_free(struct kref *kref) > +{ > + struct vb2_buffer *vb = > + container_of(kref, struct vb2_buffer, refcount); > + kfree(vb); > +} > + > +static void __vb2_buffer_put(struct vb2_buffer *vb) > +{ > + if (vb) > + kref_put(&vb->refcount, __vb2_buffer_free); > +} > + > +static struct vb2_buffer *__vb2_buffer_get(struct vb2_buffer *vb) > +{ > + if (vb) > + kref_get(&vb->refcount); > + return vb; > +} > + > /* > * __vb2_buf_mem_alloc() - allocate video memory for the given buffer > */ > @@ -346,6 +366,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum > vb2_memory memory, > break; > } > > + kref_init(&vb->refcount); > vb->state = VB2_BUF_STATE_DEQUEUED; > vb->vb2_queue = q; > vb->num_planes = num_planes; > @@ -365,7 +386,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum > vb2_memory memory, > dprintk(1, "failed allocating memory for buffer > %d\n", > buffer); > q->bufs[vb->index] = NULL; > - kfree(vb); > + __vb2_buffer_put(vb); > break; > } > __setup_offsets(vb); > @@ -380,7 +401,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum > vb2_memory memory, > buffer, vb); > __vb2_buf_mem_free(vb); > q->bufs[vb->index] = NULL; > - kfree(vb); > + __vb2_buffer_put(vb); > break; > } > } > @@ -520,7 +541,7 @@ static int __vb2_queue_free(struct vb2_queue *q, > unsigned int buffers) > /* Free videobuf buffers */ > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; >++buffer) { > - kfree(q->bufs[buffer]); > + __vb2_buffer_put(q->bufs[buffer]); > q->bufs[buffer] = NULL; > } > > diff --git a/include/media/videobuf2-core.h > b/include/media/videobuf2-core.h > index f6818f732f34..baa4632c7e59 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -12,11 +12,12 @@ > #ifndef _MEDIA_VIDEOBUF2_CORE_H > #define _MEDIA_VIDEOBUF2_CORE_H > > +#include > +#include > +#include > #include > #include > #include > -#include > -#include > > #define VB2_MAX_FRAME(32) > #define VB2_MAX_PLANES (8) > @@ -249,6 +250,7 @@ struct vb2_buffer { > > /* private: internal use only >* > + * refcount:refcount for this buffer >* state: current buffer state; do not change >* queued_entry:entry on the queued buffers list, which holds >* all buffers queued from userspace > @@ -256,6 +258,7 @@ struct vb2_buffer { >* to be dequeued to userspace >* vb2_plane: per-plane information; do not change >*/ > + struct kref refcount; > enum vb2_buffer_state state; > > struct vb2_planeplanes[VB2_MAX_PLANES]; > Regards, Sathyam
Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF
Hello, On 5/21/18, Ezequiel Garcia wrote: > From: Gustavo Padovan > > Receive in-fence from userspace and add support for waiting on them > before queueing the buffer to the driver. Buffers can't be queued to the > driver before its fences signal. And a buffer can't be queued to the driver > out of the order they were queued from userspace. That means that even if > its fence signals it must wait for all other buffers, ahead of it in the > queue, > to signal first. > > If the fence for some buffer fails we do not queue it to the driver, > instead we mark it as error and wait until the previous buffer is done > to notify userspace of the error. We wait here to deliver the buffers back > to userspace in order. > > v13: - cleanup implementation. > - remove wrong Kconfig changes. > - print noisy warning on unexpected enqueue conditioin > - schedule a vb2_start_streaming work from the fence callback > > v12: fixed dvb_vb2.c usage of vb2_core_qbuf. > > v11: - minor doc/comments fixes (Hans Verkuil) > - reviewed the in-fence path at __fill_v4l2_buffer() > > v10: - rename fence to in_fence in many places > - handle fences signalling with error better (Hans Verkuil) > > v9: - improve comments and docs (Hans Verkuil) > - fix unlocking of vb->fence_cb_lock on vb2_core_qbuf (Hans Verkuil) > - move in-fences code that was in the out-fences patch here (Alex) > > v8: - improve comments about fences with errors > > v7: - get rid of the fence array stuff for ordering and just use > get_num_buffers_ready() (Hans) > - fix issue of queuing the buffer twice (Hans) > - avoid the dma_fence_wait() in core_qbuf() (Alex) > - merge preparation commit in > > v6: - With fences always keep the order userspace queues the buffers. > - Protect in_fence manipulation with a lock (Brian Starkey) > - check if fences have the same context before adding a fence array > - Fix last_fence ref unbalance in __set_in_fence() (Brian Starkey) > - Clean up fence if __set_in_fence() fails (Brian Starkey) > - treat -EINVAL from dma_fence_add_callback() (Brian Starkey) > > v5: - use fence_array to keep buffers ordered in vb2 core when > needed (Brian Starkey) > - keep backward compat on the reserved2 field (Brian Starkey) > - protect fence callback removal with lock (Brian Starkey) > > v4: - Add a comment about dma_fence_add_callback() not returning a > error (Hans) > - Call dma_fence_put(vb->in_fence) if fence signaled (Hans) > - select SYNC_FILE under config VIDEOBUF2_CORE (Hans) > - Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans) > - Remove list_for_each_entry() in __vb2_core_qbuf() (Hans) > - Remove if (vb->state != VB2_BUF_STATE_QUEUED) from > vb2_start_streaming() (Hans) > - set IN_FENCE flags on __fill_v4l2_buffer (Hans) > - Queue buffers to the driver as soon as they are ready (Hans) > - call fill_user_buffer() after queuing the buffer (Hans) > - add err: label to clean up fence > - add dma_fence_wait() before calling vb2_start_streaming() > > v3: - document fence parameter > - remove ternary if at vb2_qbuf() return (Mauro) > - do not change if conditions behaviour (Mauro) > > v2: - fix vb2_queue_or_prepare_buf() ret check > - remove check for VB2_MEMORY_DMABUF only (Javier) > - check num of ready buffers to start streaming > - when queueing, start from the first ready buffer > - handle queue cancel > > Signed-off-by: Gustavo Padovan > Signed-off-by: Ezequiel Garcia > --- > drivers/media/common/videobuf2/Kconfig | 1 + > drivers/media/common/videobuf2/videobuf2-core.c | 224 > > drivers/media/common/videobuf2/videobuf2-v4l2.c | 37 +++- > drivers/media/dvb-core/dvb_vb2.c| 2 +- > include/media/videobuf2-core.h | 19 +- > 5 files changed, 242 insertions(+), 41 deletions(-) > > diff --git a/drivers/media/common/videobuf2/Kconfig > b/drivers/media/common/videobuf2/Kconfig > index 17c32ea58395..27ad9e8a268b 100644 > --- a/drivers/media/common/videobuf2/Kconfig > +++ b/drivers/media/common/videobuf2/Kconfig > @@ -1,6 +1,7 @@ > # Used by drivers that need Videobuf2 modules > config VIDEOBUF2_CORE > select DMA_SHARED_BUFFER > + select SYNC_FILE > tristate > > config VIDEOBUF2_V4L2 > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c > b/drivers/media/common/videobuf2/videobuf2-core.c > index a9a0a9d1decb..86b5ebe25263 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -27,6 +27,7 @@ > #include > > #include > +#include > #include > > #include > @@ -189,6 +190,7 @@ module_param(debug, int, 0644); > > static void __vb2_queue_cancel(struct vb2_queue *q); > static void __enqueue_in_driver(struct vb2_buffer *vb); > +static void __qbuf_work(struct work_struct *work); > > static void __vb2_buffer_