subject:"\[PATCH v10 01\/16\] videobuf2\: Make struct vb2

Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

2018-05-29 Thread sathyam panda

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

2018-05-29 Thread sathyam panda

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

2018-05-25 Thread sathyam panda

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(>refcount, __vb2_buffer_free);
> +}
> +
> +static struct vb2_buffer *__vb2_buffer_get(struct vb2_buffer *vb)
> +{
> + if (vb)
> + kref_get(>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(>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 01/16] videobuf2: Make struct vb2_buffer refcounted

2018-05-25 Thread sathyam panda

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(>refcount, __vb2_buffer_free);
> +}
> +
> +static struct vb2_buffer *__vb2_buffer_get(struct vb2_buffer *vb)
> +{
> + if (vb)
> + kref_get(>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(>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

[PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

2018-05-21 Thread Ezequiel Garcia

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 
---
 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(>refcount, __vb2_buffer_free);
+}
+
+static struct vb2_buffer *__vb2_buffer_get(struct vb2_buffer *vb)
+{
+   if (vb)
+   kref_get(>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(>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];
-- 
2.16.3

[PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

2018-05-21 Thread Ezequiel Garcia

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 
---
 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(>refcount, __vb2_buffer_free);
+}
+
+static struct vb2_buffer *__vb2_buffer_get(struct vb2_buffer *vb)
+{
+   if (vb)
+   kref_get(>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(>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];
-- 
2.16.3

Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

Re: [PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

[PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

[PATCH v10 01/16] videobuf2: Make struct vb2_buffer refcounted

6 matches

Site Navigation

Mail list logo

Footer information