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

[Mauro Carvalho Chehab]

Em Mon, 11 Dec 2017 16:27:39 -0200
Gustavo Padovan <gustavo@xxxxxxxxxxx> escreveu:

> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> 
> 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 <gustavo.padovan@xxxxxxxxxxxxx>
> ---
>  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.

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.

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).

Thanks,
Mauro