Date:   Thu, 2 Nov 2017 22:03:13 -0200
From:   Gustavo Padovan <gustavo@...ovan.org>
To:     Brian Starkey <brian.starkey@....com>
Cc:     linux-media@...r.kernel.org, Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Shuah Khan <shuahkh@....samsung.com>,
        Pawel Osciak <pawel@...iak.com>,
        Alexandre Courbot <acourbot@...omium.org>,
        Sakari Ailus <sakari.ailus@....fi>,
        linux-kernel@...r.kernel.org,
        Gustavo Padovan <gustavo.padovan@...labora.com>
Subject: Re: [RFC v4 16/17] [media] vb2: add out-fence support to QBUF

Hi Brian,

2017-10-27 Brian Starkey <brian.starkey@....com>:

> Hi Gustavo,
> 
> On Fri, Oct 20, 2017 at 07:50:11PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@...labora.com>
> > 
> > If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
> > an out_fence and send to userspace on the V4L2_EVENT_OUT_FENCE when
> > the buffer is queued to the driver, or right away if the queue is ordered
> > both in VB2 and in the driver.
> > 
> > The fence is signaled on buffer_done(), when the job on the buffer is
> > finished.
> > 
> > v5:
> > 	- delay fd_install to DQ_EVENT (Hans)
> > 	- if queue is fully ordered send OUT_FENCE event right away
> > 	(Brian)
> > 	- rename 'q->ordered' to 'q->ordered_in_driver'
> > 	- merge change to implement OUT_FENCE event here
> > 
> > v4:
> > 	- return the out_fence_fd in the BUF_QUEUED event(Hans)
> > 
> > v3:	- add WARN_ON_ONCE(q->ordered) on requeueing (Hans)
> > 	- set the OUT_FENCE flag if there is a fence pending (Hans)
> > 	- call fd_install() after vb2_core_qbuf() (Hans)
> > 	- clean up fence if vb2_core_qbuf() fails (Hans)
> > 	- add list to store sync_file and fence for the next queued buffer
> > 
> > v2: check if the queue is ordered.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.com>
> > ---
> > drivers/media/v4l2-core/v4l2-event.c     |  2 ++
> > drivers/media/v4l2-core/videobuf2-core.c | 25 +++++++++++++++
> > drivers/media/v4l2-core/videobuf2-v4l2.c | 55 ++++++++++++++++++++++++++++++++
> > 3 files changed, 82 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> > index 6274e3e174e0..275da224ace4 100644
> > --- a/drivers/media/v4l2-core/v4l2-event.c
> > +++ b/drivers/media/v4l2-core/v4l2-event.c
> > @@ -385,6 +385,8 @@ int v4l2_subscribe_event_v4l2(struct v4l2_fh *fh,
> > 	switch (sub->type) {
> > 	case V4L2_EVENT_CTRL:
> > 		return v4l2_ctrl_subscribe_event(fh, sub);
> > +	case V4L2_EVENT_OUT_FENCE:
> > +		return v4l2_event_subscribe(fh, sub, VIDEO_MAX_FRAME, NULL);
> > 	}
> > 	return -EINVAL;
> > }
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index c7ba67bda5ac..21e2052776c1 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -354,6 +354,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> > 			vb->planes[plane].length = plane_sizes[plane];
> > 			vb->planes[plane].min_length = plane_sizes[plane];
> > 		}
> > +		vb->out_fence_fd = -1;
> > 		q->bufs[vb->index] = vb;
> > 
> > 		/* Allocate video buffer memory for the MMAP type */
> > @@ -934,10 +935,24 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> > 	case VB2_BUF_STATE_QUEUED:
> > 		return;
> > 	case VB2_BUF_STATE_REQUEUEING:
> > +		/*
> > +		 * Explicit synchronization requires ordered queues for now,
> > +		 * so WARN_ON if we are requeuing on an ordered queue.
> > +		 */
> > +		if (vb->out_fence)
> > +			WARN_ON_ONCE(q->ordered_in_driver);
> > +
> > 		if (q->start_streaming_called)
> > 			__enqueue_in_driver(vb);
> > 		return;
> > 	default:
> > +		if (state == VB2_BUF_STATE_ERROR)
> > +			dma_fence_set_error(vb->out_fence, -ENOENT);
> > +		dma_fence_signal(vb->out_fence);
> > +		dma_fence_put(vb->out_fence);
> > +		vb->out_fence = NULL;
> > +		vb->out_fence_fd = -1;
> > +
> > 		/* Inform any processes that may be waiting for buffers */
> > 		wake_up(&q->done_wq);
> > 		break;
> > @@ -1235,6 +1250,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> > 	trace_vb2_buf_queue(q, vb);
> > 
> > 	call_void_vb_qop(vb, buf_queue, vb);
> > +
> > +	if (!(q->is_output || q->ordered_in_vb2))
> > +		call_void_bufop(q, send_out_fence, vb);
> > }
> > 
> > static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> > @@ -1451,6 +1469,7 @@ static struct dma_fence *__set_in_fence(struct vb2_queue *q,
> > 		}
> > 
> > 		q->last_fence = dma_fence_get(fence);
> > +		call_void_bufop(q, send_out_fence, vb);
> > 	}
> > 
> > 	return fence;
> > @@ -1840,6 +1859,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> > 	}
> > 
> > 	/*
> > +	 * Renew out-fence context.
> > +	 */
> 
> Why is that? I don't think I understand the nuances of fence contexts.

Because inside each context we should maintain ordering of the fences.
If we cancel the stream and restart it with we need a new context. Look
at ordering between two different streams doesn't make sense.

> 
> > +	q->out_fence_context = dma_fence_context_alloc(1);
> > +
> > +	/*
> > 	 * Remove all buffers from videobuf's list...
> > 	 */
> > 	INIT_LIST_HEAD(&q->queued_list);
> > @@ -2171,6 +2195,7 @@ int vb2_core_queue_init(struct vb2_queue *q)
> > 	spin_lock_init(&q->done_lock);
> > 	mutex_init(&q->mmap_lock);
> > 	init_waitqueue_head(&q->done_wq);
> > +	q->out_fence_context = dma_fence_context_alloc(1);
> > 
> > 	if (q->buf_struct_size == 0)
> > 		q->buf_struct_size = sizeof(struct vb2_buffer);
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 4c09ea007d90..9fb01ddefdc9 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -32,6 +32,11 @@
> > 
> > #include <media/videobuf2-v4l2.h>
> > 
> > +struct out_fence_data {
> > +	int fence_fd;
> > +	struct file *file;
> > +};
> > +
> > static int debug;
> > module_param(debug, int, 0644);
> > 
> > @@ -138,6 +143,38 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
> > 	}
> > };
> > 
> > +static void __fd_install_at_dequeue_cb(void *data)
> > +{
> > +	struct out_fence_data *of = data;
> > +
> > +	fd_install(of->fence_fd, of->file);
> > +	kfree(of);
> 
> Is it possible for the user to never dequeue the event? In that case
> the fd and sync_file would leak I guess.


Yes. It is totally possible. That fell through...

> 
> > +}
> > +
> > +static void __send_out_fence(struct vb2_buffer *vb)
> > +{
> > +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> > +	struct v4l2_fh *fh = vdev->queue->owner;
> > +	struct v4l2_event event;
> > +	struct out_fence_data *of;
> > +
> > +	if (vb->out_fence_fd < 0)
> > +		return;
> > +
> > +	memset(&event, 0, sizeof(event));
> > +	event.type = V4L2_EVENT_OUT_FENCE;
> > +	event.u.out_fence.index = vb->index;
> > +	event.u.out_fence.out_fence_fd = vb->out_fence_fd;
> > +
> > +	of = kmalloc(sizeof(*of), GFP_KERNEL);
> > +	of->fence_fd = vb->out_fence_fd;
> > +	of->file = vb->sync_file->file;
> > +
> > +	v4l2_event_queue_fh_with_cb(fh, &event, __fd_install_at_dequeue_cb, of);
> > +
> > +	vb->sync_file = NULL;
> 
> See the comment above about never dequeuing the event - maybe you need
> to keep hold of the sync file to be able to clean it up in-case the
> user never dequeues. If not, then you could set vb->out_fence_fd = -1
> here too.

That is a good suggestion.

> I've been looking at your v4l2-fences branch on kernel.org, which
> looks quite different. Is this the newer version?

Yes. I wrote this in a hurry to send it to the mailing list before
the Media Summit, so I probably forgot to push and left a few issues
around.

Regards,

Gustavo

Powered by blists - more mailing lists