by Gustavo Padovan

[permalink] [raw]

Subject: Re: [PATCH v6 4/6] [media] vb2: add in-fence support to QBUF

2017-12-21 Mauro Carvalho Chehab <[email protected]>:

> Em Mon, 11 Dec 2017 16:27:39 -0200
> Gustavo Padovan <[email protected]> escreveu:
>
> > From: Gustavo Padovan <[email protected]>
> >
> > 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.
> >
> > 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.
> >
> > 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 <[email protected]>
> > ---
> > drivers/media/v4l2-core/Kconfig | 1 +
> > drivers/media/v4l2-core/videobuf2-core.c | 154 +++++++++++++++++++++++++++----
> > drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++++-
> > include/media/videobuf2-core.h | 14 ++-
> > 4 files changed, 177 insertions(+), 21 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 a8589d96ef72..520aa3c7d9f0 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(&vb->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];
> > @@ -928,7 +929,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> >
> > switch (state) {
> > case VB2_BUF_STATE_QUEUED:
> > - return;
> > + break;
> > case VB2_BUF_STATE_REQUEUEING:
> > if (q->start_streaming_called)
> > __enqueue_in_driver(vb);
> > @@ -938,6 +939,16 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> > wake_up(&q->done_wq);
> > break;
> > }
> > +
> > + /*
> > + * If the in-fence fails the buffer is not queued to the driver
> > + * and we have to wait until the previous buffer is done before
> > + * we notify userspace that the buffer with error can be dequeued.
> > + * That way we maintain the ordering from userspace point of view.
> > + */
> > + vb = list_next_entry(vb, queued_entry);
> > + if (vb && vb->state == VB2_BUF_STATE_ERROR)
> > + vb2_buffer_done(vb, vb->state);
>
> This is not a comment for this specific hunk itself, but for all
> similar occurrences on patches 4 and 5.

It in related to the this piece, but it seems I need to improve the
quality of the comment.

>
> I really don't like the idea of unconditionally execute things at the
> core without checking if fences will be used or not. It seems risky, and,
> from my quick tests with dvb_vb2, at least one of such hunks seem
> broken for the case where either the VB2 API-specific interface doesn't
> implement fences or fences is not being used by the userspace application.

Yeah, I'll have this in mind for the next iteration of this patchset.

>
> I'm also wandering about the performance impacts for those things when
> fences aren't used/implemented at the API-specific interfaces (currently,
> videobuf2-v4l2 and dvb_vb2).

I'm writing a DRM <-> V4L2 tool at the moment to start playing with a
full pipeline and performance.

Gustavo