Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan <gustavo@padovan.org>]

2017-07-10 Gustavo Padovan <gustavo@padovan.org>:

> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
> 
> > On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> > > 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> > > 
> > > > On 06/16/17 09:39, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > > 
> > > > > 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.
> > > > > 
> > > > > 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@collabora.com>
> > > > > ---
> > > > >   drivers/media/Kconfig                    |  1 +
> > > > >   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> > > > >   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> > > > >   include/media/videobuf2-core.h           |  7 ++-
> > > > >   4 files changed, 99 insertions(+), 21 deletions(-)
> > > > > 
> > > > 
> > > > <snip>
> > > > 
> > > > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > index 110fb45..e6ad77f 100644
> > > > > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > @@ -23,6 +23,7 @@
> > > > >   #include <linux/sched.h>
> > > > >   #include <linux/freezer.h>
> > > > >   #include <linux/kthread.h>
> > > > > +#include <linux/sync_file.h>
> > > > >   #include <media/v4l2-dev.h>
> > > > >   #include <media/v4l2-fh.h>
> > > > > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> > > > >   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > > >   {
> > > > > +	struct dma_fence *fence = NULL;
> > > > >   	int ret;
> > > > >   	if (vb2_fileio_is_active(q)) {
> > > > > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > > >   	}
> > > > >   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > > > > -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > > > > +		fence = sync_file_get_fence(b->fence_fd);
> > > > > +		if (!fence) {
> > > > > +			dprintk(1, "failed to get in-fence from fd\n");
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(vb2_qbuf);
> > > > 
> > > > You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> > > > if there is a fence pending. It should also fill in fence_fd.
> > > 
> > > It was userspace who sent the fence_fd in the first place. Why do we
> > > need to send it back? The initial plan was - from a userspace point of view
> > > - to send the in-fence in the fence_fd field and receive the out-fence
> > >   in the same field.
> > > 
> > > As per setting the IN_FENCE flag, that is racy, as the fence can signal
> > > just after we called __fill_v4l2_buffer. Why is it important to set it?
> > 
> > Because when running VIDIOC_QUERYBUF it should return the current state of
> > the buffer, including whether it has a fence. You can do something like
> > v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> > of each buffer. Can be useful to e.g. figure out why a video capture seems
> > to stall. Knowing that all queued buffers are all waiting for a fence is
> > very useful information. Whether the fence_fd should also be set here or
> > only in dqbuf is something I don't know (not enough knowledge about how
> > fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> > reported though.
> 
> I didn't know about this usecase. Thanks for explaining. It certainly
> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
> I would expect the application to know the fence fd associated to than
> buffer. If we expect an application other than the one which issued
> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
> for inspection purposes. Is that the case?

I just realized that if we want to also set the in-fence fd here we
actually need to get a new unused fd, as either it is a different pid or
the app already closed the fd it was using previously. Given this extra
complication I'd say we shouldn't set fence fd unless someone has an
usecase in mind.

	Gustavo