Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[sathyam panda]

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 

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[sathyam panda]

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 

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Ezequiel Garcia]

On Tue, 2018-05-22 at 18:48 +0200, Hans Verkuil wrote:
> On 22/05/18 18:22, Ezequiel Garcia wrote:
> > > > @@ -1615,7 +1762,12 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
> > > > return;
> > > >  
> > > > vb->state = VB2_BUF_STATE_DEQUEUED;
> > > > -
> > > > +   if (vb->in_fence) {
> > > > +   if (dma_fence_remove_callback(vb->in_fence, 
> > > > >fence_cb))
> > > > +   __vb2_buffer_put(vb);
> > > > +   dma_fence_put(vb->in_fence);
> > > > +   vb->in_fence = NULL;
> > > > +   }
> > > > /* unmap DMABUF buffer */
> > > > if (q->memory == VB2_MEMORY_DMABUF)
> > > > for (i = 0; i < vb->num_planes; ++i) {
> > > > @@ -1653,7 +1805,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned 
> > > > int *pindex, void *pb,
> > > > if (pindex)
> > > > *pindex = vb->index;
> > > >  
> > > > -   /* Fill buffer information for the userspace */
> > > > +   /* Fill buffer information for userspace */
> > > > if (pb)
> > > > call_void_bufop(q, fill_user_buffer, vb, pb);
> > > >  
> > > > @@ -1700,8 +1852,8 @@ static void __vb2_queue_cancel(struct vb2_queue 
> > > > *q)
> > > > if (WARN_ON(atomic_read(>owned_by_drv_count))) {
> > > > for (i = 0; i < q->num_buffers; ++i)
> > > > if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> > > > -   pr_warn("driver bug: stop_streaming 
> > > > operation is leaving buf %p in active
> > > > state\n",
> > > > -   q->bufs[i]);
> > > > +   pr_warn("driver bug: stop_streaming 
> > > > operation is leaving buf[%d] 0x%p in active
> > > > state\n",
> > > > +   q->bufs[i]->index, q->bufs[i]);
> > > > vb2_buffer_done(q->bufs[i], 
> > > > VB2_BUF_STATE_ERROR);
> > > > }
> > > 
> > > Shouldn't any pending fences be canceled here?
> > > 
> > 
> > No, we don't have to flush -- that's the reason of the refcount :)
> > The qbuf_work won't do anything if all the buffers are returned
> > by the driver (with error or done state), and if !streaming.
> > 
> > Also, note that's why qbuf_work checks for the queued state, and not
> > for the error state.
> > 
> > > I feel uncomfortable with the refcounting of buffers, I'd rather that 
> > > when we
> > > cancel the queue all fences for buffers are removed/canceled/whatever.
> > > 
> > > Is there any reason for refcounting if we cancel all pending fences here?
> > > 
> > > Note that besides canceling fences you also need to cancel/flush 
> > > __qbuf_work.
> > > 
> > > 
> > 
> > Like I said above, I'm trying to avoid cancel/flushing the workqueue.
> > Currently, I believe it works fine without any flushing, provided we 
> > refcount
> > the buffers.
> > 
> > The problem with cancelling the workqueue, is that you need to unlock the 
> > queue
> > lock, to avoid a deadlock. It seemed to me that having a refcount is more 
> > natural.
> > 
> > Thoughts?
> > 
> 
> I'll take another look tomorrow morning. Do you have a public git tree 
> containing
> this series that I can browse?
> 
> 

Sure, there you go 
http://git.infradead.org/users/ezequielg/linux/shortlog/refs/heads/fences_v10_v4.17-rc1

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Ezequiel Garcia]

On Tue, 2018-05-22 at 18:48 +0200, Hans Verkuil wrote:
> On 22/05/18 18:22, Ezequiel Garcia wrote:
> > > > @@ -1615,7 +1762,12 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
> > > > return;
> > > >  
> > > > vb->state = VB2_BUF_STATE_DEQUEUED;
> > > > -
> > > > +   if (vb->in_fence) {
> > > > +   if (dma_fence_remove_callback(vb->in_fence, 
> > > > >fence_cb))
> > > > +   __vb2_buffer_put(vb);
> > > > +   dma_fence_put(vb->in_fence);
> > > > +   vb->in_fence = NULL;
> > > > +   }
> > > > /* unmap DMABUF buffer */
> > > > if (q->memory == VB2_MEMORY_DMABUF)
> > > > for (i = 0; i < vb->num_planes; ++i) {
> > > > @@ -1653,7 +1805,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned 
> > > > int *pindex, void *pb,
> > > > if (pindex)
> > > > *pindex = vb->index;
> > > >  
> > > > -   /* Fill buffer information for the userspace */
> > > > +   /* Fill buffer information for userspace */
> > > > if (pb)
> > > > call_void_bufop(q, fill_user_buffer, vb, pb);
> > > >  
> > > > @@ -1700,8 +1852,8 @@ static void __vb2_queue_cancel(struct vb2_queue 
> > > > *q)
> > > > if (WARN_ON(atomic_read(>owned_by_drv_count))) {
> > > > for (i = 0; i < q->num_buffers; ++i)
> > > > if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> > > > -   pr_warn("driver bug: stop_streaming 
> > > > operation is leaving buf %p in active
> > > > state\n",
> > > > -   q->bufs[i]);
> > > > +   pr_warn("driver bug: stop_streaming 
> > > > operation is leaving buf[%d] 0x%p in active
> > > > state\n",
> > > > +   q->bufs[i]->index, q->bufs[i]);
> > > > vb2_buffer_done(q->bufs[i], 
> > > > VB2_BUF_STATE_ERROR);
> > > > }
> > > 
> > > Shouldn't any pending fences be canceled here?
> > > 
> > 
> > No, we don't have to flush -- that's the reason of the refcount :)
> > The qbuf_work won't do anything if all the buffers are returned
> > by the driver (with error or done state), and if !streaming.
> > 
> > Also, note that's why qbuf_work checks for the queued state, and not
> > for the error state.
> > 
> > > I feel uncomfortable with the refcounting of buffers, I'd rather that 
> > > when we
> > > cancel the queue all fences for buffers are removed/canceled/whatever.
> > > 
> > > Is there any reason for refcounting if we cancel all pending fences here?
> > > 
> > > Note that besides canceling fences you also need to cancel/flush 
> > > __qbuf_work.
> > > 
> > > 
> > 
> > Like I said above, I'm trying to avoid cancel/flushing the workqueue.
> > Currently, I believe it works fine without any flushing, provided we 
> > refcount
> > the buffers.
> > 
> > The problem with cancelling the workqueue, is that you need to unlock the 
> > queue
> > lock, to avoid a deadlock. It seemed to me that having a refcount is more 
> > natural.
> > 
> > Thoughts?
> > 
> 
> I'll take another look tomorrow morning. Do you have a public git tree 
> containing
> this series that I can browse?
> 
> 

Sure, there you go 
http://git.infradead.org/users/ezequielg/linux/shortlog/refs/heads/fences_v10_v4.17-rc1

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 22/05/18 18:22, Ezequiel Garcia wrote:
>>> @@ -1615,7 +1762,12 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>>> return;
>>>  
>>> vb->state = VB2_BUF_STATE_DEQUEUED;
>>> -
>>> +   if (vb->in_fence) {
>>> +   if (dma_fence_remove_callback(vb->in_fence, >fence_cb))
>>> +   __vb2_buffer_put(vb);
>>> +   dma_fence_put(vb->in_fence);
>>> +   vb->in_fence = NULL;
>>> +   }
>>> /* unmap DMABUF buffer */
>>> if (q->memory == VB2_MEMORY_DMABUF)
>>> for (i = 0; i < vb->num_planes; ++i) {
>>> @@ -1653,7 +1805,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int 
>>> *pindex, void *pb,
>>> if (pindex)
>>> *pindex = vb->index;
>>>  
>>> -   /* Fill buffer information for the userspace */
>>> +   /* Fill buffer information for userspace */
>>> if (pb)
>>> call_void_bufop(q, fill_user_buffer, vb, pb);
>>>  
>>> @@ -1700,8 +1852,8 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>>> if (WARN_ON(atomic_read(>owned_by_drv_count))) {
>>> for (i = 0; i < q->num_buffers; ++i)
>>> if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
>>> -   pr_warn("driver bug: stop_streaming operation 
>>> is leaving buf %p in active state\n",
>>> -   q->bufs[i]);
>>> +   pr_warn("driver bug: stop_streaming operation 
>>> is leaving buf[%d] 0x%p in active
>>> state\n",
>>> +   q->bufs[i]->index, q->bufs[i]);
>>> vb2_buffer_done(q->bufs[i], 
>>> VB2_BUF_STATE_ERROR);
>>> }
>>
>> Shouldn't any pending fences be canceled here?
>>
> 
> No, we don't have to flush -- that's the reason of the refcount :)
> The qbuf_work won't do anything if all the buffers are returned
> by the driver (with error or done state), and if !streaming.
> 
> Also, note that's why qbuf_work checks for the queued state, and not
> for the error state.
> 
>> I feel uncomfortable with the refcounting of buffers, I'd rather that when we
>> cancel the queue all fences for buffers are removed/canceled/whatever.
>>
>> Is there any reason for refcounting if we cancel all pending fences here?
>>
>> Note that besides canceling fences you also need to cancel/flush __qbuf_work.
>>
>>
> 
> Like I said above, I'm trying to avoid cancel/flushing the workqueue.
> Currently, I believe it works fine without any flushing, provided we refcount
> the buffers.
> 
> The problem with cancelling the workqueue, is that you need to unlock the 
> queue
> lock, to avoid a deadlock. It seemed to me that having a refcount is more 
> natural.
> 
> Thoughts?
> 

I'll take another look tomorrow morning. Do you have a public git tree 
containing
this series that I can browse?

Regards,

Hans

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 22/05/18 18:22, Ezequiel Garcia wrote:
>>> @@ -1615,7 +1762,12 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>>> return;
>>>  
>>> vb->state = VB2_BUF_STATE_DEQUEUED;
>>> -
>>> +   if (vb->in_fence) {
>>> +   if (dma_fence_remove_callback(vb->in_fence, >fence_cb))
>>> +   __vb2_buffer_put(vb);
>>> +   dma_fence_put(vb->in_fence);
>>> +   vb->in_fence = NULL;
>>> +   }
>>> /* unmap DMABUF buffer */
>>> if (q->memory == VB2_MEMORY_DMABUF)
>>> for (i = 0; i < vb->num_planes; ++i) {
>>> @@ -1653,7 +1805,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int 
>>> *pindex, void *pb,
>>> if (pindex)
>>> *pindex = vb->index;
>>>  
>>> -   /* Fill buffer information for the userspace */
>>> +   /* Fill buffer information for userspace */
>>> if (pb)
>>> call_void_bufop(q, fill_user_buffer, vb, pb);
>>>  
>>> @@ -1700,8 +1852,8 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>>> if (WARN_ON(atomic_read(>owned_by_drv_count))) {
>>> for (i = 0; i < q->num_buffers; ++i)
>>> if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
>>> -   pr_warn("driver bug: stop_streaming operation 
>>> is leaving buf %p in active state\n",
>>> -   q->bufs[i]);
>>> +   pr_warn("driver bug: stop_streaming operation 
>>> is leaving buf[%d] 0x%p in active
>>> state\n",
>>> +   q->bufs[i]->index, q->bufs[i]);
>>> vb2_buffer_done(q->bufs[i], 
>>> VB2_BUF_STATE_ERROR);
>>> }
>>
>> Shouldn't any pending fences be canceled here?
>>
> 
> No, we don't have to flush -- that's the reason of the refcount :)
> The qbuf_work won't do anything if all the buffers are returned
> by the driver (with error or done state), and if !streaming.
> 
> Also, note that's why qbuf_work checks for the queued state, and not
> for the error state.
> 
>> I feel uncomfortable with the refcounting of buffers, I'd rather that when we
>> cancel the queue all fences for buffers are removed/canceled/whatever.
>>
>> Is there any reason for refcounting if we cancel all pending fences here?
>>
>> Note that besides canceling fences you also need to cancel/flush __qbuf_work.
>>
>>
> 
> Like I said above, I'm trying to avoid cancel/flushing the workqueue.
> Currently, I believe it works fine without any flushing, provided we refcount
> the buffers.
> 
> The problem with cancelling the workqueue, is that you need to unlock the 
> queue
> lock, to avoid a deadlock. It seemed to me that having a refcount is more 
> natural.
> 
> Thoughts?
> 

I'll take another look tomorrow morning. Do you have a public git tree 
containing
this series that I can browse?

Regards,

Hans

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Ezequiel Garcia]

On Tue, 2018-05-22 at 14:37 +0200, Hans Verkuil wrote:
> On 21/05/18 18:59, 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
> > +++ 

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Ezequiel Garcia]

On Tue, 2018-05-22 at 14:37 +0200, Hans Verkuil wrote:
> On 21/05/18 18:59, 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 
> > 

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 21/05/18 18:59, 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 

Re: [PATCH v10 12/16] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 21/05/18 18:59, 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);
>