Re: [RFC v4 13/17] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-10-25 Brian Starkey :

> Hi Gustavo,
> 
> On Fri, Oct 20, 2017 at 07:50:08PM -0200, 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 are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > For queues that require vb2 to queue buffers to the v4l2 driver in same
> > order they are received from userspace 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.
> 
> The API sounds/looks good to me, makes sense to let driver opt in/out
> via the ordered_in_vb2 thing. Thanks for implementing it!
> 
> > 
> > v5: - use fence_array to keep buffers ordered in vb2 core when
> > needed (Brian Stark)
> > - keep backward compatibility on the reserved2 field (Brian Stark)
> > - protect fence callback removal with lock (Brian Stark)
> 
> Brian Starkey, but close ;-)
> 
> > 
> > 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 | 179 
> > ---
> > drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
> > include/media/videobuf2-core.h   |  17 ++-
> > 4 files changed, 208 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..78f369dba3e3 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -346,6 +346,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 +1223,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 +1277,20 @@ 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;
> > +
> > +   /* count num of buffers ready in front of the queued_list */
> > +   list_for_each_entry(vb, >queued_list, queued_entry) {
> > +   if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> > +   ready_count++;
> 
> I think there's still some races on vb->in_fence. Couldn't the
> 

Re: [RFC v4 13/17] [media] vb2: add in-fence support to QBUF

[Brian Starkey]

Hi Gustavo,

On Fri, Oct 20, 2017 at 07:50:08PM -0200, 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 are only queued
to the driver once they are ready. A buffer is ready when its
in-fence signals.

For queues that require vb2 to queue buffers to the v4l2 driver in same
order they are received from userspace 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.


The API sounds/looks good to me, makes sense to let driver opt in/out
via the ordered_in_vb2 thing. Thanks for implementing it!



v5: - use fence_array to keep buffers ordered in vb2 core when
needed (Brian Stark)
- keep backward compatibility on the reserved2 field (Brian Stark)
- protect fence callback removal with lock (Brian Stark)


Brian Starkey, but close ;-)



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 | 179 ---
drivers/media/v4l2-core/videobuf2-v4l2.c |  29 -
include/media/videobuf2-core.h   |  17 ++-
4 files changed, 208 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..78f369dba3e3 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -346,6 +346,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 +1223,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 +1277,20 @@ 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;
+
+   /* count num of buffers ready in front of the queued_list */
+   list_for_each_entry(vb, >queued_list, queued_entry) {
+   if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
+   ready_count++;


I think there's still some races on vb->in_fence. Couldn't the
callback on any of the buffers in the queued_list have their callback
run at any moment?

That could make this loop do bad things, and similarly the loop in
vb2_start_streaming() which calls __enqueue_in_driver().


+   }
+
+   return ready_count;
+}
+
int vb2_core_prepare_buf(struct vb2_queue *q,