Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Brian Starkey]

On Fri, Nov 17, 2017 at 11:19:05AM -0200, Mauro Carvalho Chehab wrote:

Em Fri, 17 Nov 2017 11:08:01 -0200
Gustavo Padovan  escreveu:


2017-11-17 Mauro Carvalho Chehab :

> Em Fri, 17 Nov 2017 15:49:23 +0900
> Alexandre Courbot  escreveu:
>
> > > @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct
> > > vb2_queue *q, struct v4l2_buffer *b,
> > >  return -EINVAL;
> > >  }
> > >
> > > +if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> >
> > Why do we need to consider both values invalid? Can 0 ever be a valid fence
> > fd?
>
> Programs that don't use fences will initialize reserved2/fence_fd field
> at the uAPI call to zero.
>
> So, I guess using fd=0 here could be a problem. Anyway, I would, instead,
> do:
>
>if ((b->fence_fd < 1) &&
>...
>
> as other negative values are likely invalid as well.

We are checking when the fence_fd is set but the flag wasn't. Checking
for < 1 is exactly the opposite. so we keep as is or do it fence_fd > 0.


Ah, yes. Anyway, I would stick with:
if ((b->fence_fd > 0) &&
...



0 is a valid fence_fd right? If I close stdin, and create a sync_file,
couldn't I get a fence with fd zero?

-Brian



Gustavo



--
Thanks,
Mauro

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Alexandre Courbot]

On Fri, Nov 17, 2017 at 10:01 PM, Gustavo Padovan  wrote:
> 2017-11-17 Alexandre Courbot :
>
>> Hi Gustavo,
>>
>> I am coming a bit late in this series' review, so apologies if some of my
>> comments have already have been discussed in an earlier revision.
>>
>> On Thursday, November 16, 2017 2:10:53 AM JST, Gustavo Padovan 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 queue to the driver
>> > out of the order they were queued from userspace. That means that even if
>> > it fence signal it must wait all other buffers, ahead of it in the queue,
>> > to signal first.
>> >
>> > To make that possible we use fence_array to keep that ordering. Basically
>> > we create a fence_array that contains both the current fence and the fence
>> > from the previous buffer (which might be a fence array as well). The base
>> > fence class for the fence_array becomes the new buffer fence, waiting on
>> > that one guarantees that it won't be queued out of order.
>> >
>> > 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 
>> > ---
>> >  drivers/media/v4l2-core/Kconfig  |   1 +
>> >  drivers/media/v4l2-core/videobuf2-core.c | 202
>> > ---
>> >  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
>> >  include/media/videobuf2-core.h   |  17 ++-
>> >  4 files changed, 231 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/media/v4l2-core/Kconfig
>> > b/drivers/media/v4l2-core/Kconfig
>> > index a35c33686abf..3f988c407c80 100644
>> > --- a/drivers/media/v4l2-core/Kconfig
>> > +++ b/drivers/media/v4l2-core/Kconfig
>> > @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
>> >  # Used by drivers that need Videobuf2 modules
>> >  config VIDEOBUF2_CORE
>> > select DMA_SHARED_BUFFER
>> > +   select SYNC_FILE
>> > tristate
>> >  config VIDEOBUF2_MEMOPS
>> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> > b/drivers/media/v4l2-core/videobuf2-core.c
>> > index 60f8b582396a..26de4c80717d 100644
>> > --- a/drivers/media/v4l2-core/videobuf2-core.c
>> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> > @@ -23,6 +23,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >  #include 
>> >  #include 
>> > @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q,
>> > enum vb2_memory memory,
>> > vb->index = q->num_buffers + buffer;
>> > vb->type = q->type;
>> > vb->memory = memory;
>> > +   spin_lock_init(>fence_cb_lock);
>> > for (plane = 0; plane < num_planes; ++plane) {
>> > vb->planes[plane].length = plane_sizes[plane];
>> > vb->planes[plane].min_length = plane_sizes[plane];
>> > @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer 
>> > *vb)
>> >  {
>> > struct 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 17/11/17 18:40, Gustavo Padovan wrote:
> 2017-11-17 Hans Verkuil :
> 
>> On 15/11/17 18:10, Gustavo Padovan 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 queue to the driver
>>> out of the order they were queued from userspace. That means that even if
>>> it fence signal it must wait all other buffers, ahead of it in the queue,
>>> to signal first.
>>>
>>> To make that possible we use fence_array to keep that ordering. Basically
>>> we create a fence_array that contains both the current fence and the fence
>>> from the previous buffer (which might be a fence array as well). The base
>>> fence class for the fence_array becomes the new buffer fence, waiting on
>>> that one guarantees that it won't be queued out of order.
>>>
>>> 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 
>>> ---
>>>  drivers/media/v4l2-core/Kconfig  |   1 +
>>>  drivers/media/v4l2-core/videobuf2-core.c | 202 
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
>>>  include/media/videobuf2-core.h   |  17 ++-
>>>  4 files changed, 231 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/Kconfig 
>>> b/drivers/media/v4l2-core/Kconfig
>>> index a35c33686abf..3f988c407c80 100644
>>> --- a/drivers/media/v4l2-core/Kconfig
>>> +++ b/drivers/media/v4l2-core/Kconfig
>>> @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
>>>  # Used by drivers that need Videobuf2 modules
>>>  config VIDEOBUF2_CORE
>>> select DMA_SHARED_BUFFER
>>> +   select SYNC_FILE
>>> tristate
>>>  
>>>  config VIDEOBUF2_MEMOPS
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 60f8b582396a..26de4c80717d 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -23,6 +23,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #include 
>>>  #include 
>>> @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
>>> vb2_memory memory,
>>> vb->index = q->num_buffers + buffer;
>>> vb->type = q->type;
>>> vb->memory = memory;
>>> +   spin_lock_init(>fence_cb_lock);
>>> for (plane = 0; plane < num_planes; ++plane) {
>>> vb->planes[plane].length = plane_sizes[plane];
>>> vb->planes[plane].min_length = plane_sizes[plane];
>>> @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>>>  {
>>> struct vb2_queue *q = vb->vb2_queue;
>>>  
>>> +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
>>> +   return;
>>> +
>>> vb->state = VB2_BUF_STATE_ACTIVE;
>>> atomic_inc(>owned_by_drv_count);
>>>  
>>> @@ -1273,6 +1278,23 @@ static int __buf_prepare(struct vb2_buffer *vb, 
>>> const void *pb)
>>> 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Gustavo Padovan :

> 2017-11-17 Hans Verkuil :
> 
> > On 15/11/17 18:10, Gustavo Padovan 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 queue to the driver
> > > out of the order they were queued from userspace. That means that even if
> > > it fence signal it must wait all other buffers, ahead of it in the queue,
> > > to signal first.
> > > 
> > > To make that possible we use fence_array to keep that ordering. Basically
> > > we create a fence_array that contains both the current fence and the fence
> > > from the previous buffer (which might be a fence array as well). The base
> > > fence class for the fence_array becomes the new buffer fence, waiting on
> > > that one guarantees that it won't be queued out of order.
> > > 
> > > 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 
> > > ---
> > >  drivers/media/v4l2-core/Kconfig  |   1 +
> > >  drivers/media/v4l2-core/videobuf2-core.c | 202 
> > > ---
> > >  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
> > >  include/media/videobuf2-core.h   |  17 ++-
> > >  4 files changed, 231 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/Kconfig 
> > > b/drivers/media/v4l2-core/Kconfig
> > > index a35c33686abf..3f988c407c80 100644
> > > --- a/drivers/media/v4l2-core/Kconfig
> > > +++ b/drivers/media/v4l2-core/Kconfig
> > > @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
> > >  # Used by drivers that need Videobuf2 modules
> > >  config VIDEOBUF2_CORE
> > >   select DMA_SHARED_BUFFER
> > > + select SYNC_FILE
> > >   tristate
> > >  
> > >  config VIDEOBUF2_MEMOPS
> > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > > b/drivers/media/v4l2-core/videobuf2-core.c
> > > index 60f8b582396a..26de4c80717d 100644
> > > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > > @@ -23,6 +23,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  #include 
> > > @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, 
> > > enum vb2_memory memory,
> > >   vb->index = q->num_buffers + buffer;
> > >   vb->type = q->type;
> > >   vb->memory = memory;
> > > + spin_lock_init(>fence_cb_lock);
> > >   for (plane = 0; plane < num_planes; ++plane) {
> > >   vb->planes[plane].length = plane_sizes[plane];
> > >   vb->planes[plane].min_length = plane_sizes[plane];
> > > @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer 
> > > *vb)
> > >  {
> > >   struct vb2_queue *q = vb->vb2_queue;
> > >  
> > > + if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > > + return;
> > > +
> > >   vb->state = 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Hans Verkuil :

> On 15/11/17 18:10, Gustavo Padovan 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 queue to the driver
> > out of the order they were queued from userspace. That means that even if
> > it fence signal it must wait all other buffers, ahead of it in the queue,
> > to signal first.
> > 
> > To make that possible we use fence_array to keep that ordering. Basically
> > we create a fence_array that contains both the current fence and the fence
> > from the previous buffer (which might be a fence array as well). The base
> > fence class for the fence_array becomes the new buffer fence, waiting on
> > that one guarantees that it won't be queued out of order.
> > 
> > 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 
> > ---
> >  drivers/media/v4l2-core/Kconfig  |   1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 202 
> > ---
> >  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
> >  include/media/videobuf2-core.h   |  17 ++-
> >  4 files changed, 231 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/Kconfig 
> > b/drivers/media/v4l2-core/Kconfig
> > index a35c33686abf..3f988c407c80 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
> >  # Used by drivers that need Videobuf2 modules
> >  config VIDEOBUF2_CORE
> > select DMA_SHARED_BUFFER
> > +   select SYNC_FILE
> > tristate
> >  
> >  config VIDEOBUF2_MEMOPS
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index 60f8b582396a..26de4c80717d 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -23,6 +23,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> > vb2_memory memory,
> > vb->index = q->num_buffers + buffer;
> > vb->type = q->type;
> > vb->memory = memory;
> > +   spin_lock_init(>fence_cb_lock);
> > for (plane = 0; plane < num_planes; ++plane) {
> > vb->planes[plane].length = plane_sizes[plane];
> > vb->planes[plane].min_length = plane_sizes[plane];
> > @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> > struct vb2_queue *q = vb->vb2_queue;
> >  
> > +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +   return;
> > +
> > vb->state = VB2_BUF_STATE_ACTIVE;
> > atomic_inc(>owned_by_drv_count);
> >  
> > @@ -1273,6 +1278,23 @@ static int __buf_prepare(struct vb2_buffer *vb, 
> > const void *pb)
> > return 0;
> >  }
> >  
> > +static int 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Mauro Carvalho Chehab :

> Em Fri, 17 Nov 2017 11:12:48 -0200
> Gustavo Padovan  escreveu:
> 
> > > > /*
> > > >  * If streamon has been called, and we haven't yet called
> > > >  * start_streaming() since not enough buffers were queued, and
> > > >  * we now have reached the minimum number of queued buffers,
> > > >  * then we can finally call start_streaming().
> > > > -*
> > > > -* If already streaming, give the buffer to driver for 
> > > > processing.
> > > > -* If not, the buffer will be given to driver on next streamon.
> > > >  */
> > > > if (q->streaming && !q->start_streaming_called &&
> > > > -   q->queued_count >= q->min_buffers_needed) {
> > > > +   __get_num_ready_buffers(q) >= q->min_buffers_needed) {  
> > > 
> > > I guess the case where fences is not used is not covered here.
> > > 
> > > You probably should add a check at __get_num_ready_buffers(q)
> > > as well, making it just return q->queued_count if fences isn't
> > > used.  
> > 
> > We can't know that beforehand, some buffer ahead may have a fence,
> > but there is already a check for !fence for each buffer. If none of
> > them have fences the return will be equal to q->queued_count.
> 
> Hmm... are we willing to support the case where just some
> buffers have fences? Why?

It may be that some fences are already signaled before the QBUF call
happens, so the app may just pass -1 instead.

> In any case, we should add a notice at the documentation telling
> about what happens if not all buffers have fences, and if fences
> are required to be setup before starting streaming, or if it is
> possible to dynamically change the fances behavior while streaming.

We don't have such thing. The fence behavior is tied to each QBUF call,
the stream can be setup without knowing anything about if fences are
going to be used or not. I think we need a good reason to do otherwise.
Yet, I can add something to the docs saying that fences are exclusively
per QBUF call.

Gustavo

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 15/11/17 18:10, Gustavo Padovan 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 queue to the driver
> out of the order they were queued from userspace. That means that even if
> it fence signal it must wait all other buffers, ahead of it in the queue,
> to signal first.
> 
> To make that possible we use fence_array to keep that ordering. Basically
> we create a fence_array that contains both the current fence and the fence
> from the previous buffer (which might be a fence array as well). The base
> fence class for the fence_array becomes the new buffer fence, waiting on
> that one guarantees that it won't be queued out of order.
> 
> 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 
> ---
>  drivers/media/v4l2-core/Kconfig  |   1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 202 
> ---
>  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
>  include/media/videobuf2-core.h   |  17 ++-
>  4 files changed, 231 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index a35c33686abf..3f988c407c80 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
>  # Used by drivers that need Videobuf2 modules
>  config VIDEOBUF2_CORE
>   select DMA_SHARED_BUFFER
> + select SYNC_FILE
>   tristate
>  
>  config VIDEOBUF2_MEMOPS
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 60f8b582396a..26de4c80717d 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> vb2_memory memory,
>   vb->index = q->num_buffers + buffer;
>   vb->type = q->type;
>   vb->memory = memory;
> + spin_lock_init(>fence_cb_lock);
>   for (plane = 0; plane < num_planes; ++plane) {
>   vb->planes[plane].length = plane_sizes[plane];
>   vb->planes[plane].min_length = plane_sizes[plane];
> @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>   struct vb2_queue *q = vb->vb2_queue;
>  
> + if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> + return;
> +
>   vb->state = VB2_BUF_STATE_ACTIVE;
>   atomic_inc(>owned_by_drv_count);
>  
> @@ -1273,6 +1278,23 @@ static int __buf_prepare(struct vb2_buffer *vb, const 
> void *pb)
>   return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> + struct vb2_buffer *vb;
> + int ready_count = 0;
> + unsigned long flags;
> +
> + /* count num of buffers ready in front 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Mauro Carvalho Chehab]

Em Fri, 17 Nov 2017 11:12:48 -0200
Gustavo Padovan  escreveu:

> > >   /*
> > >* If streamon has been called, and we haven't yet called
> > >* start_streaming() since not enough buffers were queued, and
> > >* we now have reached the minimum number of queued buffers,
> > >* then we can finally call start_streaming().
> > > -  *
> > > -  * If already streaming, give the buffer to driver for processing.
> > > -  * If not, the buffer will be given to driver on next streamon.
> > >*/
> > >   if (q->streaming && !q->start_streaming_called &&
> > > - q->queued_count >= q->min_buffers_needed) {
> > > + __get_num_ready_buffers(q) >= q->min_buffers_needed) {  
> > 
> > I guess the case where fences is not used is not covered here.
> > 
> > You probably should add a check at __get_num_ready_buffers(q)
> > as well, making it just return q->queued_count if fences isn't
> > used.  
> 
> We can't know that beforehand, some buffer ahead may have a fence,
> but there is already a check for !fence for each buffer. If none of
> them have fences the return will be equal to q->queued_count.

Hmm... are we willing to support the case where just some
buffers have fences? Why?

In any case, we should add a notice at the documentation telling
about what happens if not all buffers have fences, and if fences
are required to be setup before starting streaming, or if it is
possible to dynamically change the fances behavior while streaming.

-- 
Thanks,
Mauro

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Mauro Carvalho Chehab]

Em Fri, 17 Nov 2017 11:08:01 -0200
Gustavo Padovan  escreveu:

> 2017-11-17 Mauro Carvalho Chehab :
> 
> > Em Fri, 17 Nov 2017 15:49:23 +0900
> > Alexandre Courbot  escreveu:
> >   
> > > > @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct 
> > > > vb2_queue *q, struct v4l2_buffer *b,
> > > > return -EINVAL;
> > > > }
> > > >  
> > > > +   if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> > > 
> > > Why do we need to consider both values invalid? Can 0 ever be a valid 
> > > fence 
> > > fd?  
> > 
> > Programs that don't use fences will initialize reserved2/fence_fd field
> > at the uAPI call to zero.
> > 
> > So, I guess using fd=0 here could be a problem. Anyway, I would, instead,
> > do:
> > 
> > if ((b->fence_fd < 1) &&
> > ...
> > 
> > as other negative values are likely invalid as well.  
> 
> We are checking when the fence_fd is set but the flag wasn't. Checking
> for < 1 is exactly the opposite. so we keep as is or do it fence_fd > 0.

Ah, yes. Anyway, I would stick with:
if ((b->fence_fd > 0) &&
...

> 
> Gustavo


-- 
Thanks,
Mauro

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Mauro Carvalho Chehab :

> Em Wed, 15 Nov 2017 15:10:53 -0200
> Gustavo Padovan  escreveu:
> 
> > 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 queue to the driver
> > out of the order they were queued from userspace. That means that even if
> > it fence signal it must wait all other buffers, ahead of it in the queue,
> > to signal first.
> > 
> > To make that possible we use fence_array to keep that ordering. Basically
> > we create a fence_array that contains both the current fence and the fence
> > from the previous buffer (which might be a fence array as well). The base
> > fence class for the fence_array becomes the new buffer fence, waiting on
> > that one guarantees that it won't be queued out of order.
> > 
> > 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 
> > ---
> >  drivers/media/v4l2-core/Kconfig  |   1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 202 
> > ---
> >  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
> >  include/media/videobuf2-core.h   |  17 ++-
> >  4 files changed, 231 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/Kconfig 
> > b/drivers/media/v4l2-core/Kconfig
> > index a35c33686abf..3f988c407c80 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
> >  # Used by drivers that need Videobuf2 modules
> >  config VIDEOBUF2_CORE
> > select DMA_SHARED_BUFFER
> > +   select SYNC_FILE
> > tristate
> >  
> >  config VIDEOBUF2_MEMOPS
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index 60f8b582396a..26de4c80717d 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -23,6 +23,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> > vb2_memory memory,
> > vb->index = q->num_buffers + buffer;
> > vb->type = q->type;
> > vb->memory = memory;
> > +   spin_lock_init(>fence_cb_lock);
> > for (plane = 0; plane < num_planes; ++plane) {
> > vb->planes[plane].length = plane_sizes[plane];
> > vb->planes[plane].min_length = plane_sizes[plane];
> > @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> > struct vb2_queue *q = vb->vb2_queue;
> >  
> > +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +   return;
> > +
> > vb->state = VB2_BUF_STATE_ACTIVE;
> > atomic_inc(>owned_by_drv_count);
> >  
> > @@ -1273,6 +1278,23 @@ static int __buf_prepare(struct vb2_buffer *vb, 
> > const 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Mauro Carvalho Chehab :

> Em Fri, 17 Nov 2017 15:49:23 +0900
> Alexandre Courbot  escreveu:
> 
> > > @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct 
> > > vb2_queue *q, struct v4l2_buffer *b,
> > >   return -EINVAL;
> > >   }
> > >  
> > > + if ((b->fence_fd != 0 && b->fence_fd != -1) &&  
> > 
> > Why do we need to consider both values invalid? Can 0 ever be a valid fence 
> > fd?
> 
> Programs that don't use fences will initialize reserved2/fence_fd field
> at the uAPI call to zero.
> 
> So, I guess using fd=0 here could be a problem. Anyway, I would, instead,
> do:
> 
>   if ((b->fence_fd < 1) &&
>   ...
> 
> as other negative values are likely invalid as well.

We are checking when the fence_fd is set but the flag wasn't. Checking
for < 1 is exactly the opposite. so we keep as is or do it fence_fd > 0.

Gustavo

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-11-17 Alexandre Courbot :

> Hi Gustavo,
> 
> I am coming a bit late in this series' review, so apologies if some of my
> comments have already have been discussed in an earlier revision.
> 
> On Thursday, November 16, 2017 2:10:53 AM JST, Gustavo Padovan 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 queue to the driver
> > out of the order they were queued from userspace. That means that even if
> > it fence signal it must wait all other buffers, ahead of it in the queue,
> > to signal first.
> > 
> > To make that possible we use fence_array to keep that ordering. Basically
> > we create a fence_array that contains both the current fence and the fence
> > from the previous buffer (which might be a fence array as well). The base
> > fence class for the fence_array becomes the new buffer fence, waiting on
> > that one guarantees that it won't be queued out of order.
> > 
> > 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 
> > ---
> >  drivers/media/v4l2-core/Kconfig  |   1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 202
> > ---
> >  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
> >  include/media/videobuf2-core.h   |  17 ++-
> >  4 files changed, 231 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/Kconfig
> > b/drivers/media/v4l2-core/Kconfig
> > index a35c33686abf..3f988c407c80 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
> >  # Used by drivers that need Videobuf2 modules
> >  config VIDEOBUF2_CORE
> > select DMA_SHARED_BUFFER
> > +   select SYNC_FILE
> > tristate
> >  config VIDEOBUF2_MEMOPS
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index 60f8b582396a..26de4c80717d 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -23,6 +23,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> > @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q,
> > enum vb2_memory memory,
> > vb->index = q->num_buffers + buffer;
> > vb->type = q->type;
> > vb->memory = memory;
> > +   spin_lock_init(>fence_cb_lock);
> > for (plane = 0; plane < num_planes; ++plane) {
> > vb->planes[plane].length = plane_sizes[plane];
> > vb->planes[plane].min_length = plane_sizes[plane];
> > @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> > struct vb2_queue *q = vb->vb2_queue;
> > +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +   return;
> > +
> > vb->state = VB2_BUF_STATE_ACTIVE;
> > 

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Mauro Carvalho Chehab]

Em Fri, 17 Nov 2017 15:49:23 +0900
Alexandre Courbot  escreveu:

> > @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct 
> > vb2_queue *q, struct v4l2_buffer *b,
> > return -EINVAL;
> > }
> >  
> > +   if ((b->fence_fd != 0 && b->fence_fd != -1) &&  
> 
> Why do we need to consider both values invalid? Can 0 ever be a valid fence 
> fd?

Programs that don't use fences will initialize reserved2/fence_fd field
at the uAPI call to zero.

So, I guess using fd=0 here could be a problem. Anyway, I would, instead,
do:

if ((b->fence_fd < 1) &&
...

as other negative values are likely invalid as well.

-- 
Thanks,
Mauro

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Mauro Carvalho Chehab]

Em Wed, 15 Nov 2017 15:10:53 -0200
Gustavo Padovan  escreveu:

> 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 queue to the driver
> out of the order they were queued from userspace. That means that even if
> it fence signal it must wait all other buffers, ahead of it in the queue,
> to signal first.
> 
> To make that possible we use fence_array to keep that ordering. Basically
> we create a fence_array that contains both the current fence and the fence
> from the previous buffer (which might be a fence array as well). The base
> fence class for the fence_array becomes the new buffer fence, waiting on
> that one guarantees that it won't be queued out of order.
> 
> 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 
> ---
>  drivers/media/v4l2-core/Kconfig  |   1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 202 
> ---
>  drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
>  include/media/videobuf2-core.h   |  17 ++-
>  4 files changed, 231 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index a35c33686abf..3f988c407c80 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -83,6 +83,7 @@ config VIDEOBUF_DVB
>  # Used by drivers that need Videobuf2 modules
>  config VIDEOBUF2_CORE
>   select DMA_SHARED_BUFFER
> + select SYNC_FILE
>   tristate
>  
>  config VIDEOBUF2_MEMOPS
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 60f8b582396a..26de4c80717d 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> vb2_memory memory,
>   vb->index = q->num_buffers + buffer;
>   vb->type = q->type;
>   vb->memory = memory;
> + spin_lock_init(>fence_cb_lock);
>   for (plane = 0; plane < num_planes; ++plane) {
>   vb->planes[plane].length = plane_sizes[plane];
>   vb->planes[plane].min_length = plane_sizes[plane];
> @@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>   struct vb2_queue *q = vb->vb2_queue;
>  
> + if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> + return;
> +
>   vb->state = VB2_BUF_STATE_ACTIVE;
>   atomic_inc(>owned_by_drv_count);
>  
> @@ -1273,6 +1278,23 @@ static int __buf_prepare(struct vb2_buffer *vb, const 
> void *pb)
>   return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> + struct vb2_buffer *vb;
> + int ready_count = 0;
> + unsigned long flags;
> +
> +   

Re: [RFC v5 07/11] [media] vb2: add in-fence support to QBUF

[Alexandre Courbot]

Hi Gustavo,

I am coming a bit late in this series' review, so apologies if some of my
comments have already have been discussed in an earlier revision.

On Thursday, November 16, 2017 2:10:53 AM JST, Gustavo Padovan 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 queue to the driver
out of the order they were queued from userspace. That means that even if
it fence signal it must wait all other buffers, ahead of it in the queue,
to signal first.

To make that possible we use fence_array to keep that ordering. Basically
we create a fence_array that contains both the current fence and the fence
from the previous buffer (which might be a fence array as well). The base
fence class for the fence_array becomes the new buffer fence, waiting on
that one guarantees that it won't be queued out of order.

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 
---
 drivers/media/v4l2-core/Kconfig  |   1 +
 drivers/media/v4l2-core/videobuf2-core.c | 202 
---

 drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
 include/media/videobuf2-core.h   |  17 ++-
 4 files changed, 231 insertions(+), 18 deletions(-)

diff --git a/drivers/media/v4l2-core/Kconfig 
b/drivers/media/v4l2-core/Kconfig

index a35c33686abf..3f988c407c80 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -83,6 +83,7 @@ config VIDEOBUF_DVB
 # Used by drivers that need Videobuf2 modules
 config VIDEOBUF2_CORE
select DMA_SHARED_BUFFER
+   select SYNC_FILE
tristate
 
 config VIDEOBUF2_MEMOPS
diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c

index 60f8b582396a..26de4c80717d 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 

 #include 
@@ -346,6 +347,7 @@ static int __vb2_queue_alloc(struct 
vb2_queue *q, enum vb2_memory memory,

vb->index = q->num_buffers + buffer;
vb->type = q->type;
vb->memory = memory;
+   spin_lock_init(>fence_cb_lock);
for (plane = 0; plane < num_planes; ++plane) {
vb->planes[plane].length = plane_sizes[plane];
vb->planes[plane].min_length = plane_sizes[plane];
@@ -1222,6 +1224,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 {
struct vb2_queue *q = vb->vb2_queue;
 
+	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))

+   return;
+
vb->state = VB2_BUF_STATE_ACTIVE;
atomic_inc(>owned_by_drv_count);
 
@@ -1273,6 +1278,23 @@ static int __buf_prepare(struct 
vb2_buffer *vb, const void *pb)

return 0;
 }
 
+static int __get_num_ready_buffers(struct vb2_queue *q)

+{
+   struct vb2_buffer *vb;
+   int ready_count = 0;
+   unsigned long