2017-07-11 Hans Verkuil <hverkuil@xxxxxxxxx>:
> On 10/07/17 22:26, Gustavo Padovan wrote:
> > 2017-07-10 Gustavo Padovan <gustavo@xxxxxxxxxxx>:
> >
> >> 2017-07-07 Hans Verkuil <hverkuil@xxxxxxxxx>:
> >>
> >>> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> >>>> 2017-07-06 Hans Verkuil <hverkuil@xxxxxxxxx>:
> >>>>
> >>>>> On 06/16/17 09:39, Gustavo Padovan wrote:
> >>>>>> 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 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@xxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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.
>
> Fair enough. Just make sure the fence_fd is some fixed value (-1?) in
> that case.
Sure. -1 is the default value for these cases.
Gustavo
