| Message ID | 20180521165946.11778-2-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On 5/21/18, Ezequiel Garcia <ezequiel@collabora.com> 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 <ezequiel@collabora.com> > --- > 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 <linux/bitops.h> > +#include <linux/dma-buf.h> > +#include <linux/kref.h> > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/poll.h> > -#include <linux/dma-buf.h> > -#include <linux/bitops.h> > > #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_plane planes[VB2_MAX_PLANES]; > Regards, Sathyam
On Fri, 2018-05-25 at 12:11 +0530, sathyam panda wrote: > Hello, > > On 5/21/18, Ezequiel Garcia <ezequiel@collabora.com> 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
Hi Ezequiel, On 5/29/18, Ezequiel Garcia <ezequiel@collabora.com> wrote: > On Fri, 2018-05-25 at 12:11 +0530, sathyam panda wrote: >> Hello, >> >> On 5/21/18, Ezequiel Garcia <ezequiel@collabora.com> 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
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 <linux/bitops.h> +#include <linux/dma-buf.h> +#include <linux/kref.h> #include <linux/mm_types.h> #include <linux/mutex.h> #include <linux/poll.h> -#include <linux/dma-buf.h> -#include <linux/bitops.h> #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_plane planes[VB2_MAX_PLANES];
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. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/common/videobuf2/videobuf2-core.c | 27 ++++++++++++++++++++++--- include/media/videobuf2-core.h | 7 +++++-- 2 files changed, 29 insertions(+), 5 deletions(-)