[PATCH v7 0/6] V4L2 Explicit Synchronization

[PATCH v7 0/6] V4L2 Explicit Synchronization

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Hi,

v7 bring a fix for a crash when not using fences and a uAPI fix.
I've done a bit more of testing on it and also measured some
performance. On a intel laptop a DRM<->V4L2 pipeline with fences is
runnning twice as faster than the same pipeline with no fences.

For more details on how fences work refer to patch 6 in this series.

The test tools I've been using are:
https://gitlab.collabora.com/padovan/drm-v4l2-test
https://gitlab.collabora.com/padovan/v4l2-fences-test

Please review,

Gustavo

Gustavo Padovan (6):
  [media] vb2: add is_unordered callback for drivers
  [media] v4l: add 'unordered' flag to format description ioctl
  [media] vb2: add explicit fence user API
  [media] vb2: add in-fence support to QBUF
  [media] vb2: add out-fence support to QBUF
  [media] v4l: Document explicit synchronization behavior

 Documentation/media/uapi/v4l/buffer.rst          |  15 ++
 Documentation/media/uapi/v4l/vidioc-enum-fmt.rst |   3 +
 Documentation/media/uapi/v4l/vidioc-qbuf.rst     |  47 ++++-
 Documentation/media/uapi/v4l/vidioc-querybuf.rst |   9 +-
 drivers/media/common/videobuf/videobuf2-core.c   | 253 +++++++++++++++++++++--
 drivers/media/common/videobuf/videobuf2-v4l2.c   |  51 ++++-
 drivers/media/v4l2-core/Kconfig                  |  33 +++
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c    |   4 +-
 include/media/videobuf2-core.h                   |  41 +++-
 include/uapi/linux/videodev2.h                   |   8 +-
 10 files changed, 437 insertions(+), 27 deletions(-)

-- 
2.14.3

[PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Explicit synchronization benefits a lot from ordered queues, they fit
better in a pipeline with DRM for example so create a opt-in way for
drivers notify videobuf2 that the queue is unordered.

Drivers don't need implement it if the queue is ordered.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 include/media/videobuf2-core.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f3ee4c7c2fb3..583cdc06de79 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -370,6 +370,9 @@ struct vb2_buffer {
  *			callback by calling vb2_buffer_done() with either
  *			%VB2_BUF_STATE_DONE or %VB2_BUF_STATE_ERROR; may use
  *			vb2_wait_for_all_buffers() function
+ * @is_unordered:	tell if the queue format is unordered. The default is
+ *			assumed to be ordered and this function only needs to
+ *			be implemented for unordered queues.
  * @buf_queue:		passes buffer vb to the driver; driver may start
  *			hardware operation on this buffer; driver should give
  *			the buffer back by calling vb2_buffer_done() function;
@@ -393,6 +396,7 @@ struct vb2_ops {
 
 	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
 	void (*stop_streaming)(struct vb2_queue *q);
+	int (*is_unordered)(struct vb2_queue *q);
 
 	void (*buf_queue)(struct vb2_buffer *vb);
 };
@@ -566,6 +570,7 @@ struct vb2_queue {
 	u32				cnt_wait_finish;
 	u32				cnt_start_streaming;
 	u32				cnt_stop_streaming;
+	u32				cnt_is_unordered;
 #endif
 };
 
-- 
2.14.3

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Explicit synchronization benefits a lot from ordered queues, they fit
> better in a pipeline with DRM for example so create a opt-in way for
> drivers notify videobuf2 that the queue is unordered.
> 
> Drivers don't need implement it if the queue is ordered.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  include/media/videobuf2-core.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f3ee4c7c2fb3..583cdc06de79 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -370,6 +370,9 @@ struct vb2_buffer {
>   *			callback by calling vb2_buffer_done() with either
>   *			%VB2_BUF_STATE_DONE or %VB2_BUF_STATE_ERROR; may use
>   *			vb2_wait_for_all_buffers() function
> + * @is_unordered:	tell if the queue format is unordered. The default is

I'd replace the first sentence by this:

"tell if the queue is unordered, i.e. buffers can be dequeued in a different
order from how they were queued."

Regards,

	Hans

> + *			assumed to be ordered and this function only needs to
> + *			be implemented for unordered queues.
>   * @buf_queue:		passes buffer vb to the driver; driver may start
>   *			hardware operation on this buffer; driver should give
>   *			the buffer back by calling vb2_buffer_done() function;
> @@ -393,6 +396,7 @@ struct vb2_ops {
>  
>  	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
>  	void (*stop_streaming)(struct vb2_queue *q);
> +	int (*is_unordered)(struct vb2_queue *q);
>  
>  	void (*buf_queue)(struct vb2_buffer *vb);
>  };
> @@ -566,6 +570,7 @@ struct vb2_queue {
>  	u32				cnt_wait_finish;
>  	u32				cnt_start_streaming;
>  	u32				cnt_stop_streaming;
> +	u32				cnt_is_unordered;
>  #endif
>  };
>  
> 

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Alexandre Courbot]

On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> Explicit synchronization benefits a lot from ordered queues, they fit
> better in a pipeline with DRM for example so create a opt-in way for
> drivers notify videobuf2 that the queue is unordered.
>
> Drivers don't need implement it if the queue is ordered.

This is going to make user-space believe that *all* vb2 drivers use
ordered queues by default, at least until non-ordered drivers catch up
with this change. Wouldn't it be less dangerous to do the opposite
(make queues non-ordered by default)?

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Gustavo Padovan]

2018-01-15 Alexandre Courbot <acourbot@chromium.org>:

> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >
> > Explicit synchronization benefits a lot from ordered queues, they fit
> > better in a pipeline with DRM for example so create a opt-in way for
> > drivers notify videobuf2 that the queue is unordered.
> >
> > Drivers don't need implement it if the queue is ordered.
> 
> This is going to make user-space believe that *all* vb2 drivers use
> ordered queues by default, at least until non-ordered drivers catch up
> with this change. Wouldn't it be less dangerous to do the opposite
> (make queues non-ordered by default)?

The rational behind this decision was because most formats/drivers are
ordered so only a small amount of drivers need to changed. I think this
was proposed by Hans on the Media Summit.

I understand your concern. My question is how dangerous will it be. If
you are building a product you will make the changes in the driver if
they are not there yet, or if it is a distribution you'd never know
which driver/format you are using so you should be prepared for
everything.

AFAIK all Capture drivers are ordered and that is where I think fences
is most useful.

Gustavo

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Hans Verkuil]

On 01/15/2018 01:01 PM, Gustavo Padovan wrote:
> 2018-01-15 Alexandre Courbot <acourbot@chromium.org>:
> 
>> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> Explicit synchronization benefits a lot from ordered queues, they fit
>>> better in a pipeline with DRM for example so create a opt-in way for
>>> drivers notify videobuf2 that the queue is unordered.
>>>
>>> Drivers don't need implement it if the queue is ordered.
>>
>> This is going to make user-space believe that *all* vb2 drivers use
>> ordered queues by default, at least until non-ordered drivers catch up
>> with this change. Wouldn't it be less dangerous to do the opposite
>> (make queues non-ordered by default)?
> 
> The rational behind this decision was because most formats/drivers are
> ordered so only a small amount of drivers need to changed. I think this
> was proposed by Hans on the Media Summit.
> 
> I understand your concern. My question is how dangerous will it be. If
> you are building a product you will make the changes in the driver if
> they are not there yet, or if it is a distribution you'd never know
> which driver/format you are using so you should be prepared for
> everything.
> 
> AFAIK all Capture drivers are ordered and that is where I think fences
> is most useful.

Right. What could be done is to mark all codec drivers as unordered initially
ask the driver authors to verify this. All capture drivers using vb2 and not
using REQUEUE are ordered.

One thing we haven't looked at is what to do with drivers that do not use vb2.
Those won't support fences, but how will userspace know that fences are not
supported? I'm not sure what the best method is for that.

I am leaning towards a new capability since this has to be advertised clearly.

Regards,

	Hans

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Gustavo Padovan]

2018-01-15 Hans Verkuil <hverkuil@xs4all.nl>:

> On 01/15/2018 01:01 PM, Gustavo Padovan wrote:
> > 2018-01-15 Alexandre Courbot <acourbot@chromium.org>:
> > 
> >> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> >>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>
> >>> Explicit synchronization benefits a lot from ordered queues, they fit
> >>> better in a pipeline with DRM for example so create a opt-in way for
> >>> drivers notify videobuf2 that the queue is unordered.
> >>>
> >>> Drivers don't need implement it if the queue is ordered.
> >>
> >> This is going to make user-space believe that *all* vb2 drivers use
> >> ordered queues by default, at least until non-ordered drivers catch up
> >> with this change. Wouldn't it be less dangerous to do the opposite
> >> (make queues non-ordered by default)?
> > 
> > The rational behind this decision was because most formats/drivers are
> > ordered so only a small amount of drivers need to changed. I think this
> > was proposed by Hans on the Media Summit.
> > 
> > I understand your concern. My question is how dangerous will it be. If
> > you are building a product you will make the changes in the driver if
> > they are not there yet, or if it is a distribution you'd never know
> > which driver/format you are using so you should be prepared for
> > everything.
> > 
> > AFAIK all Capture drivers are ordered and that is where I think fences
> > is most useful.
> 
> Right. What could be done is to mark all codec drivers as unordered initially
> ask the driver authors to verify this. All capture drivers using vb2 and not
> using REQUEUE are ordered.

That is a good way out.

> 
> One thing we haven't looked at is what to do with drivers that do not use vb2.
> Those won't support fences, but how will userspace know that fences are not
> supported? I'm not sure what the best method is for that.
> 
> I am leaning towards a new capability since this has to be advertised clearly.

The capability flag makes sense to me, I'll incorporate it as part of my
next patchset.

Gustavo

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Alexandre Courbot]

On Mon, Jan 15, 2018 at 9:01 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> 2018-01-15 Alexandre Courbot <acourbot@chromium.org>:
>
>> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
>> >
>> > Explicit synchronization benefits a lot from ordered queues, they fit
>> > better in a pipeline with DRM for example so create a opt-in way for
>> > drivers notify videobuf2 that the queue is unordered.
>> >
>> > Drivers don't need implement it if the queue is ordered.
>>
>> This is going to make user-space believe that *all* vb2 drivers use
>> ordered queues by default, at least until non-ordered drivers catch up
>> with this change. Wouldn't it be less dangerous to do the opposite
>> (make queues non-ordered by default)?
>
> The rational behind this decision was because most formats/drivers are
> ordered so only a small amount of drivers need to changed. I think this
> was proposed by Hans on the Media Summit.

As long as all concerned drivers are updated we should be on the safe
side. At first I was surprised that we expose the ordering feature in
a negative tense, but if the vast majority of devices are ordered this
probably makes sense.

[PATCH v7 2/6] [media] v4l: add 'unordered' flag to format description ioctl

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

For explicit synchronization it important for userspace to know if the
format being used by the driver can deliver the buffers back to userspace
in the same order they were queued with QBUF.

Ordered streams fits nicely in a pipeline with DRM for example, where
ordered buffer are expected.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/vidioc-enum-fmt.rst | 3 +++
 include/uapi/linux/videodev2.h                   | 1 +
 2 files changed, 4 insertions(+)

diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
index 019c513df217..368115f44fc0 100644
--- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
@@ -116,6 +116,9 @@ one until ``EINVAL`` is returned.
       - This format is not native to the device but emulated through
 	software (usually libv4l2), where possible try to use a native
 	format instead for better performance.
+    * - ``V4L2_FMT_FLAG_UNORDERED``
+      - 0x0004
+      - This is a format that doesn't guarantee timely order of frames.
 
 
 Return Value
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 982718965180..58894cfe9479 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -716,6 +716,7 @@ struct v4l2_fmtdesc {
 
 #define V4L2_FMT_FLAG_COMPRESSED 0x0001
 #define V4L2_FMT_FLAG_EMULATED   0x0002
+#define V4L2_FMT_FLAG_UNORDERED  0x0004
 
 	/* Frame Size and frame rate enumeration */
 /*
-- 
2.14.3

Re: [PATCH v7 2/6] [media] v4l: add 'unordered' flag to format description ioctl

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> For explicit synchronization it important for userspace to know if the
> format being used by the driver can deliver the buffers back to userspace
> in the same order they were queued with QBUF.
> 
> Ordered streams fits nicely in a pipeline with DRM for example, where
> ordered buffer are expected.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst | 3 +++
>  include/uapi/linux/videodev2.h                   | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> index 019c513df217..368115f44fc0 100644
> --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> @@ -116,6 +116,9 @@ one until ``EINVAL`` is returned.
>        - This format is not native to the device but emulated through
>  	software (usually libv4l2), where possible try to use a native
>  	format instead for better performance.
> +    * - ``V4L2_FMT_FLAG_UNORDERED``
> +      - 0x0004
> +      - This is a format that doesn't guarantee timely order of frames.

I'd rephrase this:

"This format doesn't guarantee ordered buffer handling. I.e. the order in
which buffers are dequeued with VIDIOC_DQBUF may be different from the order
in which they were queued with VIDIOC_QBUF."

(Use proper links to VIDIOC_(D)QBUF)

I would also like to see an example of a driver that uses this. The cobalt
driver is a candidate for this.

Regards,

	Hans

>  
>  
>  Return Value
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 982718965180..58894cfe9479 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -716,6 +716,7 @@ struct v4l2_fmtdesc {
>  
>  #define V4L2_FMT_FLAG_COMPRESSED 0x0001
>  #define V4L2_FMT_FLAG_EMULATED   0x0002
> +#define V4L2_FMT_FLAG_UNORDERED  0x0004
>  
>  	/* Frame Size and frame rate enumeration */
>  /*
> 

[PATCH v7 3/6] [media] vb2: add explicit fence user API

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Turn the reserved2 field into fence_fd that we will use to send
an in-fence to the kernel and return an out-fence from the kernel to
userspace.

Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
when sending a fence to the kernel to be waited on, and
V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.

v5:
	- keep using reserved2 field for cpia2
	- set fence_fd to 0 for now, for compat with userspace(Mauro)

v4:
	- make it a union with reserved2 and fence_fd (Hans Verkuil)

v3:
	- make the out_fence refer to the current buffer (Hans Verkuil)

v2: add documentation

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/buffer.rst        | 15 +++++++++++++++
 drivers/media/common/videobuf/videobuf2-v4l2.c |  2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c  |  4 ++--
 include/uapi/linux/videodev2.h                 |  7 ++++++-
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ae6ee73f151c..eeefbd2547e7 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -648,6 +648,21 @@ Buffer Flags
       - Start Of Exposure. The buffer timestamp has been taken when the
 	exposure of the frame has begun. This is only valid for the
 	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
+    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
+
+      - ``V4L2_BUF_FLAG_IN_FENCE``
+      - 0x00200000
+      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
+	won't be queued to the driver until the fence signals.
+
+    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
+
+      - ``V4L2_BUF_FLAG_OUT_FENCE``
+      - 0x00400000
+      - Request a fence to be attached to the buffer. The ``fence_fd``
+	field on
+	:ref:`VIDIOC_QBUF` is used as a return argument to send the out-fence
+	fd to userspace.
 
 
 
diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
index fac3cd6f901d..d838524a459e 100644
--- a/drivers/media/common/videobuf/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
@@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->timestamp = ns_to_timeval(vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
-	b->reserved2 = 0;
+	b->fence_fd = 0;
 	b->reserved = 0;
 
 	if (q->is_multiplanar) {
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index e48d59046086..a11a0a2bed47 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -370,7 +370,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			fence_fd;
 	__u32			reserved;
 };
 
@@ -533,7 +533,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_user(kp->reserved2, &up->reserved2) ||
+		put_user(kp->fence_fd, &up->fence_fd) ||
 		put_user(kp->reserved, &up->reserved) ||
 		put_user(kp->length, &up->length))
 			return -EFAULT;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 58894cfe9479..2d424aebdd1e 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -933,7 +933,10 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	union {
+		__s32		fence_fd;
+		__u32		reserved2;
+	};
 	__u32			reserved;
 };
 
@@ -970,6 +973,8 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
 /* mem2mem encoder/decoder */
 #define V4L2_BUF_FLAG_LAST			0x00100000
+#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
+#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
2.14.3

Re: [PATCH v7 3/6] [media] vb2: add explicit fence user API

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Turn the reserved2 field into fence_fd that we will use to send
> an in-fence to the kernel and return an out-fence from the kernel to

s/and/or/

> userspace.
> 
> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
> when sending a fence to the kernel to be waited on, and
> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
> 
> v5:
> 	- keep using reserved2 field for cpia2
> 	- set fence_fd to 0 for now, for compat with userspace(Mauro)
> 
> v4:
> 	- make it a union with reserved2 and fence_fd (Hans Verkuil)
> 
> v3:
> 	- make the out_fence refer to the current buffer (Hans Verkuil)
> 
> v2: add documentation
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst        | 15 +++++++++++++++
>  drivers/media/common/videobuf/videobuf2-v4l2.c |  2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c  |  4 ++--
>  include/uapi/linux/videodev2.h                 |  7 ++++++-
>  4 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ae6ee73f151c..eeefbd2547e7 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst

I'm missing documentation for the new fence_fd field in struct v4l2_buffer.
Make sure to mention what value should be used if you don't set the IN_FENCE
flag.

> @@ -648,6 +648,21 @@ Buffer Flags
>        - Start Of Exposure. The buffer timestamp has been taken when the
>  	exposure of the frame has begun. This is only valid for the
>  	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
> +    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_IN_FENCE``
> +      - 0x00200000
> +      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer

s/fence/the fence/
s/in/in the/

> +	won't be queued to the driver until the fence signals.
> +
> +    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_OUT_FENCE``
> +      - 0x00400000
> +      - Request a fence to be attached to the buffer. The ``fence_fd``

s/Request/Request for/

> +	field on

This very short line looks weird.

> +	:ref:`VIDIOC_QBUF` is used as a return argument to send the out-fence
> +	fd to userspace.

I'd rephrase this:

"The driver will fill in the out-fence fd in the ``fence_fd`` field when
:ref:`VIDIOC_QBUF` returns."

For both flags you also need to be explicit in mentioning that the application
sets these flags before calling VIDIOC_QBUF. For other ioctls the driver just
reports these flags.

Also mention what happens if the fence can't be found or can't be created.

Regards,

	Hans

>  
>  
>  
> diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> index fac3cd6f901d..d838524a459e 100644
> --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	b->timestamp = ns_to_timeval(vb->timestamp);
>  	b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
> -	b->reserved2 = 0;
> +	b->fence_fd = 0;
>  	b->reserved = 0;
>  
>  	if (q->is_multiplanar) {
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index e48d59046086..a11a0a2bed47 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -533,7 +533,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>  		put_user(kp->sequence, &up->sequence) ||
> -		put_user(kp->reserved2, &up->reserved2) ||
> +		put_user(kp->fence_fd, &up->fence_fd) ||
>  		put_user(kp->reserved, &up->reserved) ||
>  		put_user(kp->length, &up->length))
>  			return -EFAULT;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 58894cfe9479..2d424aebdd1e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -933,7 +933,10 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	union {
> +		__s32		fence_fd;
> +		__u32		reserved2;
> +	};
>  	__u32			reserved;
>  };
>  
> @@ -970,6 +973,8 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  /* mem2mem encoder/decoder */
>  #define V4L2_BUF_FLAG_LAST			0x00100000
> +#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
> +#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Re: [PATCH v7 3/6] [media] vb2: add explicit fence user API

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Turn the reserved2 field into fence_fd that we will use to send
> an in-fence to the kernel and return an out-fence from the kernel to
> userspace.
> 
> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
> when sending a fence to the kernel to be waited on, and
> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
> 
> v5:
> 	- keep using reserved2 field for cpia2
> 	- set fence_fd to 0 for now, for compat with userspace(Mauro)
> 
> v4:
> 	- make it a union with reserved2 and fence_fd (Hans Verkuil)
> 
> v3:
> 	- make the out_fence refer to the current buffer (Hans Verkuil)
> 
> v2: add documentation
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst        | 15 +++++++++++++++
>  drivers/media/common/videobuf/videobuf2-v4l2.c |  2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c  |  4 ++--
>  include/uapi/linux/videodev2.h                 |  7 ++++++-
>  4 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ae6ee73f151c..eeefbd2547e7 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -648,6 +648,21 @@ Buffer Flags
>        - Start Of Exposure. The buffer timestamp has been taken when the
>  	exposure of the frame has begun. This is only valid for the
>  	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
> +    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_IN_FENCE``
> +      - 0x00200000
> +      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
> +	won't be queued to the driver until the fence signals.

I'd also add: "The order in which buffers are queued is guaranteed to be
preserved, so any buffers queued after this buffer will also be blocked until this
fence signals."

Regards,

	Hans

> +
> +    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_OUT_FENCE``
> +      - 0x00400000
> +      - Request a fence to be attached to the buffer. The ``fence_fd``
> +	field on
> +	:ref:`VIDIOC_QBUF` is used as a return argument to send the out-fence
> +	fd to userspace.
>  
>  
>  
> diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> index fac3cd6f901d..d838524a459e 100644
> --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	b->timestamp = ns_to_timeval(vb->timestamp);
>  	b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
> -	b->reserved2 = 0;
> +	b->fence_fd = 0;
>  	b->reserved = 0;
>  
>  	if (q->is_multiplanar) {
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index e48d59046086..a11a0a2bed47 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -533,7 +533,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>  		put_user(kp->sequence, &up->sequence) ||
> -		put_user(kp->reserved2, &up->reserved2) ||
> +		put_user(kp->fence_fd, &up->fence_fd) ||
>  		put_user(kp->reserved, &up->reserved) ||
>  		put_user(kp->length, &up->length))
>  			return -EFAULT;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 58894cfe9479..2d424aebdd1e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -933,7 +933,10 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	union {
> +		__s32		fence_fd;
> +		__u32		reserved2;
> +	};
>  	__u32			reserved;
>  };
>  
> @@ -970,6 +973,8 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  /* mem2mem encoder/decoder */
>  #define V4L2_BUF_FLAG_LAST			0x00100000
> +#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
> +#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

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

[Gustavo Padovan]

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

v8:	- improve comments about fences with errors

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@collabora.com>
---
 drivers/media/common/videobuf/videobuf2-core.c | 166 ++++++++++++++++++++++---
 drivers/media/common/videobuf/videobuf2-v4l2.c |  29 ++++-
 drivers/media/v4l2-core/Kconfig                |  33 +++++
 include/media/videobuf2-core.h                 |  14 ++-
 4 files changed, 221 insertions(+), 21 deletions(-)

diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
index f7109f827f6e..777e3a2bc746 100644
--- a/drivers/media/common/videobuf/videobuf2-core.c
+++ b/drivers/media/common/videobuf/videobuf2-core.c
@@ -352,6 +352,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];
@@ -936,7 +937,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);
@@ -946,6 +947,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 		wake_up(&q->done_wq);
 		break;
 	}
+
+	/*
+	 * The check below verifies if there is a buffer in queue with an
+	 * error state. They are added to queue in the error state when
+	 * their in-fence fails to signal.
+	 * To not mess with buffer ordering we wait until the previous buffer
+	 * is done to mark the buffer in the error state as done and notify
+	 * userspace. So everytime a buffer is done we check the next one for
+	 * VB2_BUF_STATE_ERROR.
+	 */
+	vb = list_next_entry(vb, queued_entry);
+	if (vb && vb->state == VB2_BUF_STATE_ERROR)
+		vb2_buffer_done(vb, vb->state);
 }
 EXPORT_SYMBOL_GPL(vb2_buffer_done);
 
@@ -1230,6 +1244,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 
+	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
+		return;
+
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->owned_by_drv_count);
 
@@ -1281,6 +1298,24 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
 	return 0;
 }
 
+static int __get_num_ready_buffers(struct vb2_queue *q)
+{
+	struct vb2_buffer *vb;
+	int ready_count = 0;
+	unsigned long flags;
+
+	/* count num of buffers ready in front of the queued_list */
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		spin_lock_irqsave(&vb->fence_cb_lock, flags);
+		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
+			break;
+		ready_count++;
+		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+	}
+
+	return ready_count;
+}
+
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
@@ -1369,9 +1404,43 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	return ret;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
+	struct vb2_queue *q = vb->vb2_queue;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vb->fence_cb_lock, flags);
+	/*
+	 * If the fence signal with an error we mark the buffer as such
+	 * and avoid using it by setting it to VB2_BUF_STATE_ERROR and
+	 * not queueing it to the driver. However we can't notify the error
+	 * to userspace right now because, at the time this callback run, QBUF
+	 * returned already.
+	 * So we delay that to DQBUF time. See comments in vb2_buffer_done()
+	 * as well.
+	 */
+	if (vb->in_fence->error)
+		vb->state = VB2_BUF_STATE_ERROR;
+
+	dma_fence_put(vb->in_fence);
+	vb->in_fence = NULL;
+
+	if (vb->state == VB2_BUF_STATE_ERROR) {
+		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+		return;
+	}
+
+	if (q->start_streaming_called)
+		__enqueue_in_driver(vb);
+	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+		  struct dma_fence *fence)
 {
 	struct vb2_buffer *vb;
+	unsigned long flags;
 	int ret;
 
 	vb = q->bufs[index];
@@ -1380,16 +1449,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	case VB2_BUF_STATE_DEQUEUED:
 		ret = __buf_prepare(vb, pb);
 		if (ret)
-			return ret;
+			goto err;
 		break;
 	case VB2_BUF_STATE_PREPARED:
 		break;
 	case VB2_BUF_STATE_PREPARING:
 		dprintk(1, "buffer still being prepared\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	default:
 		dprintk(1, "invalid buffer state %d\n", vb->state);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	/*
@@ -1400,6 +1471,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	q->queued_count++;
 	q->waiting_for_buffers = false;
 	vb->state = VB2_BUF_STATE_QUEUED;
+	vb->in_fence = fence;
 
 	if (pb)
 		call_void_bufop(q, copy_timestamp, vb, pb);
@@ -1407,15 +1479,42 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	trace_vb2_qbuf(q, vb);
 
 	/*
-	 * If already streaming, give the buffer to driver for processing.
-	 * If not, the buffer will be given to driver on next streamon.
+	 * For explicit synchronization: If the fence didn't signal
+	 * yet we setup a callback to queue the buffer once the fence
+	 * signals, and then, return successfully. But if the fence
+	 * already signaled we lose the reference we held and queue the
+	 * buffer to the driver.
 	 */
-	if (q->start_streaming_called)
-		__enqueue_in_driver(vb);
+	spin_lock_irqsave(&vb->fence_cb_lock, flags);
+	if (vb->in_fence) {
+		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
+					     vb2_qbuf_fence_cb);
+		if (ret == -EINVAL) {
+			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+			goto err;
+		} else if (!ret) {
+			goto fill;
+		}
 
-	/* Fill buffer information for the userspace */
-	if (pb)
-		call_void_bufop(q, fill_user_buffer, vb, pb);
+		dma_fence_put(vb->in_fence);
+		vb->in_fence = NULL;
+	}
+
+fill:
+	/*
+	 * If already streaming and there is no fence to wait on
+	 * give the buffer to driver for processing.
+	 */
+	if (q->start_streaming_called) {
+		struct vb2_buffer *b;
+		list_for_each_entry(b, &q->queued_list, queued_entry) {
+			if (b->state != VB2_BUF_STATE_QUEUED)
+				continue;
+			if (b->in_fence)
+				break;
+			__enqueue_in_driver(b);
+		}
+	}
 
 	/*
 	 * If streamon has been called, and we haven't yet called
@@ -1424,14 +1523,33 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	 * then we can finally call start_streaming().
 	 */
 	if (q->streaming && !q->start_streaming_called &&
-	    q->queued_count >= q->min_buffers_needed) {
+	    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
 		ret = vb2_start_streaming(q);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
+	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+
+	/* Fill buffer information for the userspace */
+	if (pb)
+		call_void_bufop(q, fill_user_buffer, vb, pb);
+
 	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
 	return 0;
+
+err:
+	/* Fill buffer information for the userspace */
+	if (pb)
+		call_void_bufop(q, fill_user_buffer, vb, pb);
+
+	if (vb->in_fence) {
+		dma_fence_put(vb->in_fence);
+		vb->in_fence = NULL;
+	}
+
+	return ret;
+
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
 
@@ -1642,6 +1760,8 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
 static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
+	struct vb2_buffer *vb;
+	unsigned long flags;
 
 	/*
 	 * Tell driver to stop all transactions and release all queued
@@ -1672,6 +1792,16 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 	q->queued_count = 0;
 	q->error = 0;
 
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		spin_lock_irqsave(&vb->fence_cb_lock, flags);
+		if (vb->in_fence) {
+			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
+			dma_fence_put(vb->in_fence);
+			vb->in_fence = NULL;
+		}
+		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
+	}
+
 	/*
 	 * Remove all buffers from videobuf's list...
 	 */
@@ -1733,7 +1863,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
 	 * Tell driver to start streaming provided sufficient buffers
 	 * are available.
 	 */
-	if (q->queued_count >= q->min_buffers_needed) {
+	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
 		ret = v4l_vb2q_enable_media_source(q);
 		if (ret)
 			return ret;
@@ -2255,7 +2385,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 		 * Queue all buffers.
 		 */
 		for (i = 0; i < q->num_buffers; i++) {
-			ret = vb2_core_qbuf(q, i, NULL);
+			ret = vb2_core_qbuf(q, i, NULL, NULL);
 			if (ret)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
@@ -2434,7 +2564,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 
 		if (copy_timestamp)
 			b->timestamp = ktime_get_ns();
-		ret = vb2_core_qbuf(q, index, NULL);
+		ret = vb2_core_qbuf(q, index, NULL, NULL);
 		dprintk(5, "vb2_dbuf result: %d\n", ret);
 		if (ret)
 			return ret;
@@ -2537,7 +2667,7 @@ static int vb2_thread(void *data)
 		if (copy_timestamp)
 			vb->timestamp = ktime_get_ns();
 		if (!threadio->stop)
-			ret = vb2_core_qbuf(q, vb->index, NULL);
+			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
 		call_void_qop(q, wait_prepare, q);
 		if (ret || threadio->stop)
 			break;
diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
index d838524a459e..0a41e3bb7733 100644
--- a/drivers/media/common/videobuf/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf/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>
@@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
+	if ((b->fence_fd != 0 && b->fence_fd != -1) &&
+	    !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
+		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
+		return -EINVAL;
+	}
+
 	return __verify_planes_array(q->bufs[b->index], b);
 }
 
@@ -203,9 +210,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->timestamp = ns_to_timeval(vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
-	b->fence_fd = 0;
 	b->reserved = 0;
 
+	b->fence_fd = 0;
+	if (vb->in_fence)
+		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
+	else
+		b->flags &= ~V4L2_BUF_FLAG_IN_FENCE;
+
 	if (q->is_multiplanar) {
 		/*
 		 * Fill in plane-related data if userspace provided an array
@@ -562,6 +574,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)) {
@@ -570,7 +583,19 @@ 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 %d\n",
+				b->fence_fd);
+			return -EINVAL;
+		}
+	}
+
+	return vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index bf52fbd07aed..a39968eb1d32 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -79,3 +79,36 @@ config VIDEOBUF_DMA_CONTIG
 config VIDEOBUF_DVB
 	tristate
 	select VIDEOBUF_GEN
+
+# Used by drivers that need Videobuf2 modules
+config VIDEOBUF2_CORE
+	select DMA_SHARED_BUFFER
+	select SYNC_FILE
+	tristate
+
+config VIDEOBUF2_MEMOPS
+	tristate
+	select FRAME_VECTOR
+
+config VIDEOBUF2_DMA_CONTIG
+	tristate
+	depends on HAS_DMA
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_MEMOPS
+	select DMA_SHARED_BUFFER
+
+config VIDEOBUF2_VMALLOC
+	tristate
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_MEMOPS
+	select DMA_SHARED_BUFFER
+
+config VIDEOBUF2_DMA_SG
+	tristate
+	depends on HAS_DMA
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_MEMOPS
+
+config VIDEOBUF2_DVB
+	tristate
+	select VIDEOBUF2_CORE
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 583cdc06de79..0a2b1ac12dd0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -17,6 +17,7 @@
 #include <linux/poll.h>
 #include <linux/dma-buf.h>
 #include <linux/bitops.h>
+#include <linux/dma-fence.h>
 
 #define VB2_MAX_FRAME	(32)
 #define VB2_MAX_PLANES	(8)
@@ -255,12 +256,21 @@ struct vb2_buffer {
 	 * done_entry:		entry on the list that stores all buffers ready
 	 *			to be dequeued to userspace
 	 * vb2_plane:		per-plane information; do not change
+	 * in_fence:		fence receive from vb2 client to wait on before
+	 *			using the buffer (queueing to the driver)
+	 * fence_cb:		fence callback information
+	 * fence_cb_lock:	protect callback signal/remove
 	 */
 	enum vb2_buffer_state	state;
 
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	struct list_head	queued_entry;
 	struct list_head	done_entry;
+
+	struct dma_fence	*in_fence;
+	struct dma_fence_cb	fence_cb;
+	spinlock_t              fence_cb_lock;
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -750,6 +760,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
  * @index:	id number of the buffer
  * @pb:		buffer structure passed from userspace to
  *		v4l2_ioctl_ops->vidioc_qbuf handler in driver
+ * @fence:	in-fence to wait on before queueing the buffer
  *
  * Videobuf2 core helper to implement VIDIOC_QBUF() operation. It is called
  * internally by VB2 by an API-specific handler, like ``videobuf2-v4l2.h``.
@@ -764,7 +775,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
  *
  * Return: returns zero on success; an error code otherwise.
  */
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+		  struct dma_fence *fence);
 
 /**
  * vb2_core_dqbuf() - Dequeue a buffer to the userspace
-- 
2.14.3

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

[Hans Verkuil]

On 01/10/18 17:07, 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 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.
> 
> v8:	- improve comments about fences with errors
> 
> 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@collabora.com>
> ---
>  drivers/media/common/videobuf/videobuf2-core.c | 166 ++++++++++++++++++++++---
>  drivers/media/common/videobuf/videobuf2-v4l2.c |  29 ++++-
>  drivers/media/v4l2-core/Kconfig                |  33 +++++
>  include/media/videobuf2-core.h                 |  14 ++-
>  4 files changed, 221 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
> index f7109f827f6e..777e3a2bc746 100644
> --- a/drivers/media/common/videobuf/videobuf2-core.c
> +++ b/drivers/media/common/videobuf/videobuf2-core.c
> @@ -352,6 +352,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];
> @@ -936,7 +937,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);
> @@ -946,6 +947,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  		wake_up(&q->done_wq);
>  		break;
>  	}
> +
> +	/*
> +	 * The check below verifies if there is a buffer in queue with an

s/in/in the/

> +	 * error state. They are added to queue in the error state when
> +	 * their in-fence fails to signal.
> +	 * To not mess with buffer ordering we wait until the previous buffer
> +	 * is done to mark the buffer in the error state as done and notify
> +	 * userspace. So everytime a buffer is done we check the next one for

s/everytime/every time/

> +	 * VB2_BUF_STATE_ERROR.
> +	 */
> +	vb = list_next_entry(vb, queued_entry);
> +	if (vb && vb->state == VB2_BUF_STATE_ERROR)
> +		vb2_buffer_done(vb, vb->state);

Hmm. I'm not sure this is correct. Please test this and make sure you have
enabled CONFIG_VIDEO_ADV_DEBUG. This enables additional instrumentation that
will warn if the internal vb2 state becomes unbalanced.

An additional problem I have with this recursive call is what happens when
a whole bunch of in fences signal an error, so you have e.g. 10 consecutive
buffers with state ERROR. You'd recurse 10 times in that case.

I think you are better off handling this corner case explicitly, e.g. something
like this:

	for (;;) {
		vb = list_next_entry(vb, queued_entry);
		if (!vb || vb->state != VB2_BUF_STATE_ERROR)
			break;

	        dprintk(4, "done processing on buffer %d, state: %d\n",
        	        vb->index, vb->state);

		if (state == VB2_BUF_STATE_QUEUED) {
			vb->state = state;
		} else {
		        spin_lock_irqsave(&q->done_lock, flags);
                	/* Add the buffer to the done buffers list */
        	        list_add_tail(&vb->done_entry, &q->done_list);
		        spin_unlock_irqrestore(&q->done_lock, flags);

	                /* Inform any processes that may be waiting for buffers */
        	        wake_up(&q->done_wq);
		}

        	trace_vb2_buf_done(q, vb);
	}

I *think* this ensures all counters remain balanced, but this really needs
to be tested. I haven't compiled this code, so I hope I got it right.

>  }
>  EXPORT_SYMBOL_GPL(vb2_buffer_done);
>  
> @@ -1230,6 +1244,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
>  
> +	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +		return;
> +
>  	vb->state = VB2_BUF_STATE_ACTIVE;
>  	atomic_inc(&q->owned_by_drv_count);
>  
> @@ -1281,6 +1298,24 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
>  	return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> +	struct vb2_buffer *vb;
> +	int ready_count = 0;
> +	unsigned long flags;
> +
> +	/* count num of buffers ready in front of the queued_list */
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		spin_lock_irqsave(&vb->fence_cb_lock, flags);
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +		ready_count++;
> +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +	}
> +
> +	return ready_count;
> +}
> +
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1369,9 +1404,43 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> +{
> +	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
> +	struct vb2_queue *q = vb->vb2_queue;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vb->fence_cb_lock, flags);
> +	/*
> +	 * If the fence signal with an error we mark the buffer as such

s/signal/signals/

> +	 * and avoid using it by setting it to VB2_BUF_STATE_ERROR and
> +	 * not queueing it to the driver. However we can't notify the error
> +	 * to userspace right now because, at the time this callback run, QBUF
> +	 * returned already.
> +	 * So we delay that to DQBUF time. See comments in vb2_buffer_done()
> +	 * as well.
> +	 */
> +	if (vb->in_fence->error)
> +		vb->state = VB2_BUF_STATE_ERROR;
> +
> +	dma_fence_put(vb->in_fence);
> +	vb->in_fence = NULL;
> +
> +	if (vb->state == VB2_BUF_STATE_ERROR) {
> +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +		return;
> +	}
> +
> +	if (q->start_streaming_called)
> +		__enqueue_in_driver(vb);
> +	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence)
>  {
>  	struct vb2_buffer *vb;
> +	unsigned long flags;
>  	int ret;
>  
>  	vb = q->bufs[index];
> @@ -1380,16 +1449,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	case VB2_BUF_STATE_DEQUEUED:
>  		ret = __buf_prepare(vb, pb);
>  		if (ret)
> -			return ret;
> +			goto err;
>  		break;
>  	case VB2_BUF_STATE_PREPARED:
>  		break;
>  	case VB2_BUF_STATE_PREPARING:
>  		dprintk(1, "buffer still being prepared\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err;
>  	default:
>  		dprintk(1, "invalid buffer state %d\n", vb->state);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err;
>  	}
>  
>  	/*
> @@ -1400,6 +1471,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	q->queued_count++;
>  	q->waiting_for_buffers = false;
>  	vb->state = VB2_BUF_STATE_QUEUED;
> +	vb->in_fence = fence;
>  
>  	if (pb)
>  		call_void_bufop(q, copy_timestamp, vb, pb);
> @@ -1407,15 +1479,42 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	trace_vb2_qbuf(q, vb);
>  
>  	/*
> -	 * If already streaming, give the buffer to driver for processing.
> -	 * If not, the buffer will be given to driver on next streamon.
> +	 * For explicit synchronization: If the fence didn't signal
> +	 * yet we setup a callback to queue the buffer once the fence
> +	 * signals, and then, return successfully. But if the fence
> +	 * already signaled we lose the reference we held and queue the
> +	 * buffer to the driver.
>  	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> +	spin_lock_irqsave(&vb->fence_cb_lock, flags);
> +	if (vb->in_fence) {
> +		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
> +					     vb2_qbuf_fence_cb);
> +		if (ret == -EINVAL) {
> +			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +			goto err;
> +		} else if (!ret) {
> +			goto fill;
> +		}
>  
> -	/* Fill buffer information for the userspace */
> -	if (pb)
> -		call_void_bufop(q, fill_user_buffer, vb, pb);
> +		dma_fence_put(vb->in_fence);
> +		vb->in_fence = NULL;
> +	}
> +
> +fill:
> +	/*
> +	 * If already streaming and there is no fence to wait on
> +	 * give the buffer to driver for processing.
> +	 */
> +	if (q->start_streaming_called) {
> +		struct vb2_buffer *b;

Add empty line.

> +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> +			if (b->state != VB2_BUF_STATE_QUEUED)
> +				continue;
> +			if (b->in_fence)
> +				break;
> +			__enqueue_in_driver(b);
> +		}
> +	}
>  
>  	/*
>  	 * If streamon has been called, and we haven't yet called
> @@ -1424,14 +1523,33 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	 * then we can finally call start_streaming().
>  	 */
>  	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> +	    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
>  		ret = vb2_start_streaming(q);
>  		if (ret)
> -			return ret;
> +			goto err;

You're missing a spin_unlock_irqrestore() call in this error path.

>  	}
>  
> +	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +
> +	/* Fill buffer information for the userspace */
> +	if (pb)
> +		call_void_bufop(q, fill_user_buffer, vb, pb);
> +
>  	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
> +
> +err:
> +	/* Fill buffer information for the userspace */
> +	if (pb)
> +		call_void_bufop(q, fill_user_buffer, vb, pb);
> +
> +	if (vb->in_fence) {
> +		dma_fence_put(vb->in_fence);
> +		vb->in_fence = NULL;
> +	}
> +
> +	return ret;
> +
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  
> @@ -1642,6 +1760,8 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
>  static void __vb2_queue_cancel(struct vb2_queue *q)
>  {
>  	unsigned int i;
> +	struct vb2_buffer *vb;
> +	unsigned long flags;
>  
>  	/*
>  	 * Tell driver to stop all transactions and release all queued
> @@ -1672,6 +1792,16 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  	q->queued_count = 0;
>  	q->error = 0;
>  
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		spin_lock_irqsave(&vb->fence_cb_lock, flags);
> +		if (vb->in_fence) {
> +			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
> +			dma_fence_put(vb->in_fence);
> +			vb->in_fence = NULL;
> +		}
> +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> +	}
> +
>  	/*
>  	 * Remove all buffers from videobuf's list...
>  	 */
> @@ -1733,7 +1863,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  	 * Tell driver to start streaming provided sufficient buffers
>  	 * are available.
>  	 */
> -	if (q->queued_count >= q->min_buffers_needed) {
> +	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
>  		ret = v4l_vb2q_enable_media_source(q);
>  		if (ret)
>  			return ret;
> @@ -2255,7 +2385,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  		 * Queue all buffers.
>  		 */
>  		for (i = 0; i < q->num_buffers; i++) {
> -			ret = vb2_core_qbuf(q, i, NULL);
> +			ret = vb2_core_qbuf(q, i, NULL, NULL);
>  			if (ret)
>  				goto err_reqbufs;
>  			fileio->bufs[i].queued = 1;
> @@ -2434,7 +2564,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>  
>  		if (copy_timestamp)
>  			b->timestamp = ktime_get_ns();
> -		ret = vb2_core_qbuf(q, index, NULL);
> +		ret = vb2_core_qbuf(q, index, NULL, NULL);
>  		dprintk(5, "vb2_dbuf result: %d\n", ret);
>  		if (ret)
>  			return ret;
> @@ -2537,7 +2667,7 @@ static int vb2_thread(void *data)
>  		if (copy_timestamp)
>  			vb->timestamp = ktime_get_ns();
>  		if (!threadio->stop)
> -			ret = vb2_core_qbuf(q, vb->index, NULL);
> +			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
>  		call_void_qop(q, wait_prepare, q);
>  		if (ret || threadio->stop)
>  			break;
> diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> index d838524a459e..0a41e3bb7733 100644
> --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf/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>
> @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
>  		return -EINVAL;
>  	}
>  
> +	if ((b->fence_fd != 0 && b->fence_fd != -1) &&

Wouldn't 'b->fence_fd > 0' be more sensible?

And possibly just set fence_fd to 0 if it is < 0.

> +	    !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
> +		return -EINVAL;
> +	}
> +
>  	return __verify_planes_array(q->bufs[b->index], b);
>  }
>  
> @@ -203,9 +210,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	b->timestamp = ns_to_timeval(vb->timestamp);
>  	b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
> -	b->fence_fd = 0;
>  	b->reserved = 0;
>  
> +	b->fence_fd = 0;
> +	if (vb->in_fence)
> +		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
> +	else
> +		b->flags &= ~V4L2_BUF_FLAG_IN_FENCE;
> +
>  	if (q->is_multiplanar) {
>  		/*
>  		 * Fill in plane-related data if userspace provided an array
> @@ -562,6 +574,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)) {
> @@ -570,7 +583,19 @@ 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 %d\n",
> +				b->fence_fd);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return vb2_core_qbuf(q, b->index, b, fence);
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
>  
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index bf52fbd07aed..a39968eb1d32 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -79,3 +79,36 @@ config VIDEOBUF_DMA_CONTIG
>  config VIDEOBUF_DVB
>  	tristate
>  	select VIDEOBUF_GEN
> +
> +# Used by drivers that need Videobuf2 modules
> +config VIDEOBUF2_CORE
> +	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
> +	tristate
> +
> +config VIDEOBUF2_MEMOPS
> +	tristate
> +	select FRAME_VECTOR
> +
> +config VIDEOBUF2_DMA_CONTIG
> +	tristate
> +	depends on HAS_DMA
> +	select VIDEOBUF2_CORE
> +	select VIDEOBUF2_MEMOPS
> +	select DMA_SHARED_BUFFER
> +
> +config VIDEOBUF2_VMALLOC
> +	tristate
> +	select VIDEOBUF2_CORE
> +	select VIDEOBUF2_MEMOPS
> +	select DMA_SHARED_BUFFER
> +
> +config VIDEOBUF2_DMA_SG
> +	tristate
> +	depends on HAS_DMA
> +	select VIDEOBUF2_CORE
> +	select VIDEOBUF2_MEMOPS
> +
> +config VIDEOBUF2_DVB
> +	tristate
> +	select VIDEOBUF2_CORE
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 583cdc06de79..0a2b1ac12dd0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -17,6 +17,7 @@
>  #include <linux/poll.h>
>  #include <linux/dma-buf.h>
>  #include <linux/bitops.h>
> +#include <linux/dma-fence.h>
>  
>  #define VB2_MAX_FRAME	(32)
>  #define VB2_MAX_PLANES	(8)
> @@ -255,12 +256,21 @@ struct vb2_buffer {
>  	 * done_entry:		entry on the list that stores all buffers ready
>  	 *			to be dequeued to userspace
>  	 * vb2_plane:		per-plane information; do not change
> +	 * in_fence:		fence receive from vb2 client to wait on before

receive -> received

> +	 *			using the buffer (queueing to the driver)
> +	 * fence_cb:		fence callback information
> +	 * fence_cb_lock:	protect callback signal/remove
>  	 */
>  	enum vb2_buffer_state	state;
>  
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;
>  	struct list_head	done_entry;
> +
> +	struct dma_fence	*in_fence;
> +	struct dma_fence_cb	fence_cb;
> +	spinlock_t              fence_cb_lock;
> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these buffer-related ops are
> @@ -750,6 +760,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>   * @index:	id number of the buffer
>   * @pb:		buffer structure passed from userspace to
>   *		v4l2_ioctl_ops->vidioc_qbuf handler in driver
> + * @fence:	in-fence to wait on before queueing the buffer
>   *
>   * Videobuf2 core helper to implement VIDIOC_QBUF() operation. It is called
>   * internally by VB2 by an API-specific handler, like ``videobuf2-v4l2.h``.
> @@ -764,7 +775,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>   *
>   * Return: returns zero on success; an error code otherwise.
>   */
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence);
>  
>  /**
>   * vb2_core_dqbuf() - Dequeue a buffer to the userspace
> 

Regards,

	Hans

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

[Gustavo Padovan]

Hi Hans,

2018-01-12 Hans Verkuil <hverkuil@xs4all.nl>:

> On 01/10/18 17:07, 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 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.
> > 
> > v8:	- improve comments about fences with errors
> > 
> > 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@collabora.com>
> > ---
> >  drivers/media/common/videobuf/videobuf2-core.c | 166 ++++++++++++++++++++++---
> >  drivers/media/common/videobuf/videobuf2-v4l2.c |  29 ++++-
> >  drivers/media/v4l2-core/Kconfig                |  33 +++++
> >  include/media/videobuf2-core.h                 |  14 ++-
> >  4 files changed, 221 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
> > index f7109f827f6e..777e3a2bc746 100644
> > --- a/drivers/media/common/videobuf/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf/videobuf2-core.c
> > @@ -352,6 +352,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];
> > @@ -936,7 +937,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);
> > @@ -946,6 +947,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> >  		wake_up(&q->done_wq);
> >  		break;
> >  	}
> > +
> > +	/*
> > +	 * The check below verifies if there is a buffer in queue with an
> 
> s/in/in the/
> 
> > +	 * error state. They are added to queue in the error state when
> > +	 * their in-fence fails to signal.
> > +	 * To not mess with buffer ordering we wait until the previous buffer
> > +	 * is done to mark the buffer in the error state as done and notify
> > +	 * userspace. So everytime a buffer is done we check the next one for
> 
> s/everytime/every time/
> 
> > +	 * VB2_BUF_STATE_ERROR.
> > +	 */
> > +	vb = list_next_entry(vb, queued_entry);
> > +	if (vb && vb->state == VB2_BUF_STATE_ERROR)
> > +		vb2_buffer_done(vb, vb->state);
> 
> Hmm. I'm not sure this is correct. Please test this and make sure you have
> enabled CONFIG_VIDEO_ADV_DEBUG. This enables additional instrumentation that
> will warn if the internal vb2 state becomes unbalanced.
> 
> An additional problem I have with this recursive call is what happens when
> a whole bunch of in fences signal an error, so you have e.g. 10 consecutive
> buffers with state ERROR. You'd recurse 10 times in that case.
> 
> I think you are better off handling this corner case explicitly, e.g. something
> like this:
> 
> 	for (;;) {
> 		vb = list_next_entry(vb, queued_entry);
> 		if (!vb || vb->state != VB2_BUF_STATE_ERROR)
> 			break;
> 
> 	        dprintk(4, "done processing on buffer %d, state: %d\n",
>         	        vb->index, vb->state);
> 
> 		if (state == VB2_BUF_STATE_QUEUED) {
> 			vb->state = state;
> 		} else {
> 		        spin_lock_irqsave(&q->done_lock, flags);
>                 	/* Add the buffer to the done buffers list */
>         	        list_add_tail(&vb->done_entry, &q->done_list);
> 		        spin_unlock_irqrestore(&q->done_lock, flags);
> 
> 	                /* Inform any processes that may be waiting for buffers */
>         	        wake_up(&q->done_wq);
> 		}
> 
>         	trace_vb2_buf_done(q, vb);
> 	}
> 
> I *think* this ensures all counters remain balanced, but this really needs
> to be tested. I haven't compiled this code, so I hope I got it right.

I'll play with this and make sure I get it right.

> 
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_buffer_done);
> >  
> > @@ -1230,6 +1244,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> >  	struct vb2_queue *q = vb->vb2_queue;
> >  
> > +	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +		return;
> > +
> >  	vb->state = VB2_BUF_STATE_ACTIVE;
> >  	atomic_inc(&q->owned_by_drv_count);
> >  
> > @@ -1281,6 +1298,24 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> >  	return 0;
> >  }
> >  
> > +static int __get_num_ready_buffers(struct vb2_queue *q)
> > +{
> > +	struct vb2_buffer *vb;
> > +	int ready_count = 0;
> > +	unsigned long flags;
> > +
> > +	/* count num of buffers ready in front of the queued_list */
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		spin_lock_irqsave(&vb->fence_cb_lock, flags);
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +		ready_count++;
> > +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +	}
> > +
> > +	return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1369,9 +1404,43 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	return ret;
> >  }
> >  
> > -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> > +static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> > +{
> > +	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
> > +	struct vb2_queue *q = vb->vb2_queue;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&vb->fence_cb_lock, flags);
> > +	/*
> > +	 * If the fence signal with an error we mark the buffer as such
> 
> s/signal/signals/
> 
> > +	 * and avoid using it by setting it to VB2_BUF_STATE_ERROR and
> > +	 * not queueing it to the driver. However we can't notify the error
> > +	 * to userspace right now because, at the time this callback run, QBUF
> > +	 * returned already.
> > +	 * So we delay that to DQBUF time. See comments in vb2_buffer_done()
> > +	 * as well.
> > +	 */
> > +	if (vb->in_fence->error)
> > +		vb->state = VB2_BUF_STATE_ERROR;
> > +
> > +	dma_fence_put(vb->in_fence);
> > +	vb->in_fence = NULL;
> > +
> > +	if (vb->state == VB2_BUF_STATE_ERROR) {
> > +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +		return;
> > +	}
> > +
> > +	if (q->start_streaming_called)
> > +		__enqueue_in_driver(vb);
> > +	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +}
> > +
> > +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> > +		  struct dma_fence *fence)
> >  {
> >  	struct vb2_buffer *vb;
> > +	unsigned long flags;
> >  	int ret;
> >  
> >  	vb = q->bufs[index];
> > @@ -1380,16 +1449,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	case VB2_BUF_STATE_DEQUEUED:
> >  		ret = __buf_prepare(vb, pb);
> >  		if (ret)
> > -			return ret;
> > +			goto err;
> >  		break;
> >  	case VB2_BUF_STATE_PREPARED:
> >  		break;
> >  	case VB2_BUF_STATE_PREPARING:
> >  		dprintk(1, "buffer still being prepared\n");
> > -		return -EINVAL;
> > +		ret = -EINVAL;
> > +		goto err;
> >  	default:
> >  		dprintk(1, "invalid buffer state %d\n", vb->state);
> > -		return -EINVAL;
> > +		ret = -EINVAL;
> > +		goto err;
> >  	}
> >  
> >  	/*
> > @@ -1400,6 +1471,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	q->queued_count++;
> >  	q->waiting_for_buffers = false;
> >  	vb->state = VB2_BUF_STATE_QUEUED;
> > +	vb->in_fence = fence;
> >  
> >  	if (pb)
> >  		call_void_bufop(q, copy_timestamp, vb, pb);
> > @@ -1407,15 +1479,42 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	trace_vb2_qbuf(q, vb);
> >  
> >  	/*
> > -	 * If already streaming, give the buffer to driver for processing.
> > -	 * If not, the buffer will be given to driver on next streamon.
> > +	 * For explicit synchronization: If the fence didn't signal
> > +	 * yet we setup a callback to queue the buffer once the fence
> > +	 * signals, and then, return successfully. But if the fence
> > +	 * already signaled we lose the reference we held and queue the
> > +	 * buffer to the driver.
> >  	 */
> > -	if (q->start_streaming_called)
> > -		__enqueue_in_driver(vb);
> > +	spin_lock_irqsave(&vb->fence_cb_lock, flags);
> > +	if (vb->in_fence) {
> > +		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
> > +					     vb2_qbuf_fence_cb);
> > +		if (ret == -EINVAL) {
> > +			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +			goto err;
> > +		} else if (!ret) {
> > +			goto fill;
> > +		}
> >  
> > -	/* Fill buffer information for the userspace */
> > -	if (pb)
> > -		call_void_bufop(q, fill_user_buffer, vb, pb);
> > +		dma_fence_put(vb->in_fence);
> > +		vb->in_fence = NULL;
> > +	}
> > +
> > +fill:
> > +	/*
> > +	 * If already streaming and there is no fence to wait on
> > +	 * give the buffer to driver for processing.
> > +	 */
> > +	if (q->start_streaming_called) {
> > +		struct vb2_buffer *b;
> 
> Add empty line.
> 
> > +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> > +			if (b->state != VB2_BUF_STATE_QUEUED)
> > +				continue;
> > +			if (b->in_fence)
> > +				break;
> > +			__enqueue_in_driver(b);
> > +		}
> > +	}
> >  
> >  	/*
> >  	 * If streamon has been called, and we haven't yet called
> > @@ -1424,14 +1523,33 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	 * then we can finally call start_streaming().
> >  	 */
> >  	if (q->streaming && !q->start_streaming_called &&
> > -	    q->queued_count >= q->min_buffers_needed) {
> > +	    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> >  		ret = vb2_start_streaming(q);
> >  		if (ret)
> > -			return ret;
> > +			goto err;
> 
> You're missing a spin_unlock_irqrestore() call in this error path.
> 
> >  	}
> >  
> > +	spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +
> > +	/* Fill buffer information for the userspace */
> > +	if (pb)
> > +		call_void_bufop(q, fill_user_buffer, vb, pb);
> > +
> >  	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
> >  	return 0;
> > +
> > +err:
> > +	/* Fill buffer information for the userspace */
> > +	if (pb)
> > +		call_void_bufop(q, fill_user_buffer, vb, pb);
> > +
> > +	if (vb->in_fence) {
> > +		dma_fence_put(vb->in_fence);
> > +		vb->in_fence = NULL;
> > +	}
> > +
> > +	return ret;
> > +
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> >  
> > @@ -1642,6 +1760,8 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
> >  static void __vb2_queue_cancel(struct vb2_queue *q)
> >  {
> >  	unsigned int i;
> > +	struct vb2_buffer *vb;
> > +	unsigned long flags;
> >  
> >  	/*
> >  	 * Tell driver to stop all transactions and release all queued
> > @@ -1672,6 +1792,16 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >  	q->queued_count = 0;
> >  	q->error = 0;
> >  
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		spin_lock_irqsave(&vb->fence_cb_lock, flags);
> > +		if (vb->in_fence) {
> > +			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
> > +			dma_fence_put(vb->in_fence);
> > +			vb->in_fence = NULL;
> > +		}
> > +		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> > +	}
> > +
> >  	/*
> >  	 * Remove all buffers from videobuf's list...
> >  	 */
> > @@ -1733,7 +1863,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
> >  	 * Tell driver to start streaming provided sufficient buffers
> >  	 * are available.
> >  	 */
> > -	if (q->queued_count >= q->min_buffers_needed) {
> > +	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
> >  		ret = v4l_vb2q_enable_media_source(q);
> >  		if (ret)
> >  			return ret;
> > @@ -2255,7 +2385,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >  		 * Queue all buffers.
> >  		 */
> >  		for (i = 0; i < q->num_buffers; i++) {
> > -			ret = vb2_core_qbuf(q, i, NULL);
> > +			ret = vb2_core_qbuf(q, i, NULL, NULL);
> >  			if (ret)
> >  				goto err_reqbufs;
> >  			fileio->bufs[i].queued = 1;
> > @@ -2434,7 +2564,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> >  
> >  		if (copy_timestamp)
> >  			b->timestamp = ktime_get_ns();
> > -		ret = vb2_core_qbuf(q, index, NULL);
> > +		ret = vb2_core_qbuf(q, index, NULL, NULL);
> >  		dprintk(5, "vb2_dbuf result: %d\n", ret);
> >  		if (ret)
> >  			return ret;
> > @@ -2537,7 +2667,7 @@ static int vb2_thread(void *data)
> >  		if (copy_timestamp)
> >  			vb->timestamp = ktime_get_ns();
> >  		if (!threadio->stop)
> > -			ret = vb2_core_qbuf(q, vb->index, NULL);
> > +			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
> >  		call_void_qop(q, wait_prepare, q);
> >  		if (ret || threadio->stop)
> >  			break;
> > diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> > index d838524a459e..0a41e3bb7733 100644
> > --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf/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>
> > @@ -178,6 +179,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
> >  		return -EINVAL;
> >  	}
> >  
> > +	if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> 
> Wouldn't 'b->fence_fd > 0' be more sensible?

This is the case where the in fence flag is not set, I think we also
want to cover < -1 values userspace may set.

Gustavo

[PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
an out_fence and send its fd to userspace on the fence_fd field as a
return arg for the QBUF call.

The fence is signaled on buffer_done(), when the job on the buffer is
finished.

v8:
	- return 0 as fence_fd if OUT_FENCE flag not used (Mauro)
	- fix crash when checking not using fences in vb2_buffer_done()

v7:
	- merge patch that add the infrastructure to out-fences into
	this one (Alex Courbot)
	- Do not install the fd if there is no fence. (Alex Courbot)
	- do not report error on requeueing, just WARN_ON_ONCE() (Hans)

v6
	- get rid of the V4L2_EVENT_OUT_FENCE event. We always keep the
	ordering in vb2 for queueing in the driver, so the event is not
	necessary anymore and the out_fence_fd is sent back to userspace
	on QBUF call return arg
	- do not allow requeueing with out-fences, instead mark the buffer
	with an error and wake up to userspace.
	- send the out_fence_fd back to userspace on the fence_fd field

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@collabora.com>
---
 drivers/media/common/videobuf/videobuf2-core.c | 101 +++++++++++++++++++++++--
 drivers/media/common/videobuf/videobuf2-v4l2.c |  28 ++++++-
 include/media/videobuf2-core.h                 |  22 ++++++
 3 files changed, 140 insertions(+), 11 deletions(-)

diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
index 777e3a2bc746..1f30d9efb7c8 100644
--- a/drivers/media/common/videobuf/videobuf2-core.c
+++ b/drivers/media/common/videobuf/videobuf2-core.c
@@ -25,6 +25,7 @@
 #include <linux/sched.h>
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/sync_file.h>
 
 #include <media/videobuf2-core.h>
 #include <media/v4l2-mc.h>
@@ -357,6 +358,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 */
@@ -939,10 +941,22 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	case VB2_BUF_STATE_QUEUED:
 		break;
 	case VB2_BUF_STATE_REQUEUEING:
+		/* Requeuing with explicit synchronization, spit warning */
+		WARN_ON_ONCE(vb->out_fence);
+
 		if (q->start_streaming_called)
 			__enqueue_in_driver(vb);
-		return;
+		break;
 	default:
+		if (vb->out_fence) {
+			if (state == VB2_BUF_STATE_ERROR)
+				dma_fence_set_error(vb->out_fence, -EFAULT);
+			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;
@@ -1341,6 +1355,65 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
 
+static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
+{
+	return "vb2_fence";
+}
+
+static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
+{
+	return "vb2_fence_timeline";
+}
+
+static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
+{
+	return true;
+}
+
+static const struct dma_fence_ops vb2_fence_ops = {
+	.get_driver_name = vb2_fence_get_driver_name,
+	.get_timeline_name = vb2_fence_get_timeline_name,
+	.enable_signaling = vb2_fence_enable_signaling,
+	.wait = dma_fence_default_wait,
+};
+
+int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index)
+{
+	struct vb2_buffer *vb;
+	u64 context;
+
+	vb = q->bufs[index];
+
+	vb->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+
+	if (call_qop(q, is_unordered, q))
+		context = dma_fence_context_alloc(1);
+	else
+		context = q->out_fence_context;
+
+	vb->out_fence = kzalloc(sizeof(*vb->out_fence), GFP_KERNEL);
+	if (!vb->out_fence)
+		return -ENOMEM;
+
+	dma_fence_init(vb->out_fence, &vb2_fence_ops, &q->out_fence_lock,
+		       context, 1);
+	if (!vb->out_fence) {
+		put_unused_fd(vb->out_fence_fd);
+		return -ENOMEM;
+	}
+
+	vb->sync_file = sync_file_create(vb->out_fence);
+	if (!vb->sync_file) {
+		put_unused_fd(vb->out_fence_fd);
+		dma_fence_put(vb->out_fence);
+		vb->out_fence = NULL;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
+
 /*
  * vb2_start_streaming() - Attempt to start streaming.
  * @q:		videobuf2 queue
@@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 	if (vb->in_fence) {
 		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
 					     vb2_qbuf_fence_cb);
-		if (ret == -EINVAL) {
+		/* is the fence signaled? */
+		if (ret == -ENOENT) {
+			dma_fence_put(vb->in_fence);
+			vb->in_fence = NULL;
+		} else if (ret) {
 			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
 			goto err;
-		} else if (!ret) {
-			goto fill;
 		}
-
-		dma_fence_put(vb->in_fence);
-		vb->in_fence = NULL;
 	}
 
-fill:
 	/*
 	 * If already streaming and there is no fence to wait on
 	 * give the buffer to driver for processing.
@@ -1535,6 +1606,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 	if (pb)
 		call_void_bufop(q, fill_user_buffer, vb, pb);
 
+	if (vb->out_fence) {
+		fd_install(vb->out_fence_fd, vb->sync_file->file);
+		vb->sync_file = NULL;
+	}
+
 	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
 	return 0;
 
@@ -1802,6 +1878,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
 	}
 
+	/*
+	 * Renew out-fence context.
+	 */
+	if (!call_qop(q, is_unordered, q))
+		q->out_fence_context = dma_fence_context_alloc(1);
+
 	/*
 	 * Remove all buffers from videobuf's list...
 	 */
@@ -2134,6 +2216,9 @@ 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);
+	if (!call_qop(q, is_unordered, q))
+		q->out_fence_context = dma_fence_context_alloc(1);
+	spin_lock_init(&q->out_fence_lock);
 
 	q->memory = VB2_MEMORY_UNKNOWN;
 
diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
index 0a41e3bb7733..f1291c25323f 100644
--- a/drivers/media/common/videobuf/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
@@ -179,12 +179,16 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
-	if ((b->fence_fd != 0 && b->fence_fd != -1) &&
-	    !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
+	if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
 		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
 		return -EINVAL;
 	}
 
+	if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
+		dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
+		return -EINVAL;
+	}
+
 	return __verify_planes_array(q->bufs[b->index], b);
 }
 
@@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->sequence = vbuf->sequence;
 	b->reserved = 0;
 
-	b->fence_fd = 0;
+	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+		b->fence_fd = vb->out_fence_fd;
+	} else {
+		b->fence_fd = 0;
+	}
+
 	if (vb->in_fence)
 		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
 	else
@@ -491,6 +500,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	ret = __verify_planes_array(vb, b);
 	if (!ret)
 		vb2_core_querybuf(q, b->index, b);
+
+	/* Do not return the out-fence fd on querybuf */
+	if (vb->out_fence)
+		b->fence_fd = -1;
 	return ret;
 }
 EXPORT_SYMBOL(vb2_querybuf);
@@ -595,6 +608,15 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		}
 	}
 
+	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+		ret = vb2_setup_out_fence(q, b->index);
+		if (ret) {
+			dprintk(1, "failed to set up out-fence\n");
+			dma_fence_put(fence);
+			return ret;
+		}
+	}
+
 	return vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 0a2b1ac12dd0..260435dfc301 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -260,6 +260,10 @@ struct vb2_buffer {
 	 *			using the buffer (queueing to the driver)
 	 * fence_cb:		fence callback information
 	 * fence_cb_lock:	protect callback signal/remove
+	 * out_fence_fd:	the out_fence_fd to be shared with userspace.
+	 * out_fence:		the out-fence associated with the buffer once
+	 *			it is queued to the driver.
+	 * sync_file:		the sync file to wrap the out fence
 	 */
 	enum vb2_buffer_state	state;
 
@@ -271,6 +275,10 @@ struct vb2_buffer {
 	struct dma_fence_cb	fence_cb;
 	spinlock_t              fence_cb_lock;
 
+	int			out_fence_fd;
+	struct dma_fence	*out_fence;
+	struct sync_file	*sync_file;
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -514,6 +522,7 @@ struct vb2_buf_ops {
  * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
  *		last decoded buffer was already dequeued. Set for capture queues
  *		when a buffer with the %V4L2_BUF_FLAG_LAST is dequeued.
+ * @out_fence_context: the fence context for the out fences
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
  */
@@ -567,6 +576,9 @@ struct vb2_queue {
 	unsigned int			copy_timestamp:1;
 	unsigned int			last_buffer_dequeued:1;
 
+	u64				out_fence_context;
+	spinlock_t			out_fence_lock;
+
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
 
@@ -753,6 +765,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
  */
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
 
+/**
+ * vb2_setup_out_fence() - setup new out-fence
+ * @q:		The vb2_queue where to setup it
+ * @index:	index of the buffer
+ *
+ * Setup the file descriptor, the fence and the sync_file for the next
+ * buffer to be queued and add everything to the tail of the q->out_fence_list.
+ */
+int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index);
+
 /**
  * vb2_core_qbuf() - Queue a buffer from userspace
  *
-- 
2.14.3

Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
> an out_fence and send its fd to userspace on the fence_fd field as a
> return arg for the QBUF call.
> 
> The fence is signaled on buffer_done(), when the job on the buffer is
> finished.
> 
> v8:
> 	- return 0 as fence_fd if OUT_FENCE flag not used (Mauro)
> 	- fix crash when checking not using fences in vb2_buffer_done()
> 
> v7:
> 	- merge patch that add the infrastructure to out-fences into
> 	this one (Alex Courbot)
> 	- Do not install the fd if there is no fence. (Alex Courbot)
> 	- do not report error on requeueing, just WARN_ON_ONCE() (Hans)
> 
> v6
> 	- get rid of the V4L2_EVENT_OUT_FENCE event. We always keep the
> 	ordering in vb2 for queueing in the driver, so the event is not
> 	necessary anymore and the out_fence_fd is sent back to userspace
> 	on QBUF call return arg
> 	- do not allow requeueing with out-fences, instead mark the buffer
> 	with an error and wake up to userspace.
> 	- send the out_fence_fd back to userspace on the fence_fd field
> 
> 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@collabora.com>
> ---
>  drivers/media/common/videobuf/videobuf2-core.c | 101 +++++++++++++++++++++++--
>  drivers/media/common/videobuf/videobuf2-v4l2.c |  28 ++++++-
>  include/media/videobuf2-core.h                 |  22 ++++++
>  3 files changed, 140 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
> index 777e3a2bc746..1f30d9efb7c8 100644
> --- a/drivers/media/common/videobuf/videobuf2-core.c
> +++ b/drivers/media/common/videobuf/videobuf2-core.c
> @@ -25,6 +25,7 @@
>  #include <linux/sched.h>
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
> +#include <linux/sync_file.h>
>  
>  #include <media/videobuf2-core.h>
>  #include <media/v4l2-mc.h>
> @@ -357,6 +358,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 */
> @@ -939,10 +941,22 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  	case VB2_BUF_STATE_QUEUED:
>  		break;
>  	case VB2_BUF_STATE_REQUEUEING:
> +		/* Requeuing with explicit synchronization, spit warning */
> +		WARN_ON_ONCE(vb->out_fence);
> +
>  		if (q->start_streaming_called)
>  			__enqueue_in_driver(vb);
> -		return;
> +		break;
>  	default:
> +		if (vb->out_fence) {
> +			if (state == VB2_BUF_STATE_ERROR)
> +				dma_fence_set_error(vb->out_fence, -EFAULT);
> +			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;
> @@ -1341,6 +1355,65 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>  
> +static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
> +{
> +	return "vb2_fence";
> +}
> +
> +static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
> +{
> +	return "vb2_fence_timeline";
> +}
> +
> +static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
> +{
> +	return true;
> +}
> +
> +static const struct dma_fence_ops vb2_fence_ops = {
> +	.get_driver_name = vb2_fence_get_driver_name,
> +	.get_timeline_name = vb2_fence_get_timeline_name,
> +	.enable_signaling = vb2_fence_enable_signaling,
> +	.wait = dma_fence_default_wait,
> +};
> +
> +int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index)
> +{
> +	struct vb2_buffer *vb;
> +	u64 context;
> +
> +	vb = q->bufs[index];
> +
> +	vb->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> +
> +	if (call_qop(q, is_unordered, q))
> +		context = dma_fence_context_alloc(1);
> +	else
> +		context = q->out_fence_context;
> +
> +	vb->out_fence = kzalloc(sizeof(*vb->out_fence), GFP_KERNEL);
> +	if (!vb->out_fence)
> +		return -ENOMEM;
> +
> +	dma_fence_init(vb->out_fence, &vb2_fence_ops, &q->out_fence_lock,
> +		       context, 1);
> +	if (!vb->out_fence) {
> +		put_unused_fd(vb->out_fence_fd);
> +		return -ENOMEM;
> +	}
> +
> +	vb->sync_file = sync_file_create(vb->out_fence);
> +	if (!vb->sync_file) {
> +		put_unused_fd(vb->out_fence_fd);
> +		dma_fence_put(vb->out_fence);
> +		vb->out_fence = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
> +
>  /*
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:		videobuf2 queue
> @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,

This function is now very confusing. The last argument is a fence, but it is set
for both in and out fences, and the previous patch sets vb->in_fence to fence.
Even though the fence passed in this case is an out fence.

I think it would be much easier to understand if you add both an in_fence and
out_fence argument to this function.

Is it possible to have both and in and out fence for the same buffer? I think
it is and in that case you really need two fences.

>  	if (vb->in_fence) {
>  		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
>  					     vb2_qbuf_fence_cb);
> -		if (ret == -EINVAL) {
> +		/* is the fence signaled? */
> +		if (ret == -ENOENT) {
> +			dma_fence_put(vb->in_fence);
> +			vb->in_fence = NULL;
> +		} else if (ret) {
>  			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
>  			goto err;
> -		} else if (!ret) {
> -			goto fill;
>  		}
> -
> -		dma_fence_put(vb->in_fence);
> -		vb->in_fence = NULL;
>  	}
>  
> -fill:
>  	/*
>  	 * If already streaming and there is no fence to wait on
>  	 * give the buffer to driver for processing.
> @@ -1535,6 +1606,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> +	if (vb->out_fence) {
> +		fd_install(vb->out_fence_fd, vb->sync_file->file);
> +		vb->sync_file = NULL;
> +	}
> +
>  	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
>  
> @@ -1802,6 +1878,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
>  	}
>  
> +	/*
> +	 * Renew out-fence context.
> +	 */
> +	if (!call_qop(q, is_unordered, q))
> +		q->out_fence_context = dma_fence_context_alloc(1);

I don't think this is the right place. If a driver implements is_unordered, then
the return value depends on the format. And that isn't locked in until buffers
are allocated (i.e. reqbufs or create_bufs is called). So that's the moment that
you can set this up.

BTW: I noticed is_unordered() returned an int instead of a bool. I think a bool
makes more sense.

> +
>  	/*
>  	 * Remove all buffers from videobuf's list...
>  	 */
> @@ -2134,6 +2216,9 @@ 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);
> +	if (!call_qop(q, is_unordered, q))
> +		q->out_fence_context = dma_fence_context_alloc(1);

Same here: at this moment you do not know if the queue is ordered or
unordered.

> +	spin_lock_init(&q->out_fence_lock);
>  
>  	q->memory = VB2_MEMORY_UNKNOWN;
>  
> diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> index 0a41e3bb7733..f1291c25323f 100644
> --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
> @@ -179,12 +179,16 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
>  		return -EINVAL;
>  	}
>  
> -	if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> -	    !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +	if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
>  		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
>  		return -EINVAL;
>  	}
>  
> +	if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {

Shouldn't this be b->fence_fd <= 0?

> +		dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> +		return -EINVAL;
> +	}
> +
>  	return __verify_planes_array(q->bufs[b->index], b);
>  }
>  
> @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	b->sequence = vbuf->sequence;
>  	b->reserved = 0;
>  
> -	b->fence_fd = 0;
> +	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +		b->fence_fd = vb->out_fence_fd;
> +	} else {
> +		b->fence_fd = 0;
> +	}
> +
>  	if (vb->in_fence)
>  		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
>  	else
> @@ -491,6 +500,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  	ret = __verify_planes_array(vb, b);
>  	if (!ret)
>  		vb2_core_querybuf(q, b->index, b);
> +
> +	/* Do not return the out-fence fd on querybuf */
> +	if (vb->out_fence)
> +		b->fence_fd = -1;

This does not hurt. We also return dmabuf fds in the same way in querybuf.

I would drop this.

>  	return ret;
>  }
>  EXPORT_SYMBOL(vb2_querybuf);
> @@ -595,6 +608,15 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		}
>  	}
>  
> +	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +		ret = vb2_setup_out_fence(q, b->index);
> +		if (ret) {
> +			dprintk(1, "failed to set up out-fence\n");
> +			dma_fence_put(fence);
> +			return ret;
> +		}
> +	}
> +
>  	return vb2_core_qbuf(q, b->index, b, fence);

Hmm. So if vb2_core_qbuf returns an error, we still have created the out fence?
As mentioned above, it is very confusing that the same vb2_core_qbuf 'fence'
argument is used for both in and out fences.

>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 0a2b1ac12dd0..260435dfc301 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -260,6 +260,10 @@ struct vb2_buffer {
>  	 *			using the buffer (queueing to the driver)
>  	 * fence_cb:		fence callback information
>  	 * fence_cb_lock:	protect callback signal/remove
> +	 * out_fence_fd:	the out_fence_fd to be shared with userspace.
> +	 * out_fence:		the out-fence associated with the buffer once
> +	 *			it is queued to the driver.
> +	 * sync_file:		the sync file to wrap the out fence
>  	 */
>  	enum vb2_buffer_state	state;
>  
> @@ -271,6 +275,10 @@ struct vb2_buffer {
>  	struct dma_fence_cb	fence_cb;
>  	spinlock_t              fence_cb_lock;
>  
> +	int			out_fence_fd;
> +	struct dma_fence	*out_fence;
> +	struct sync_file	*sync_file;
> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these buffer-related ops are
> @@ -514,6 +522,7 @@ struct vb2_buf_ops {
>   * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
>   *		last decoded buffer was already dequeued. Set for capture queues
>   *		when a buffer with the %V4L2_BUF_FLAG_LAST is dequeued.
> + * @out_fence_context: the fence context for the out fences
>   * @fileio:	file io emulator internal data, used only if emulator is active
>   * @threadio:	thread io internal data, used only if thread is active
>   */
> @@ -567,6 +576,9 @@ struct vb2_queue {
>  	unsigned int			copy_timestamp:1;
>  	unsigned int			last_buffer_dequeued:1;
>  
> +	u64				out_fence_context;
> +	spinlock_t			out_fence_lock;
> +
>  	struct vb2_fileio_data		*fileio;
>  	struct vb2_threadio_data	*threadio;
>  
> @@ -753,6 +765,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>   */
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>  
> +/**
> + * vb2_setup_out_fence() - setup new out-fence
> + * @q:		The vb2_queue where to setup it
> + * @index:	index of the buffer
> + *
> + * Setup the file descriptor, the fence and the sync_file for the next
> + * buffer to be queued and add everything to the tail of the q->out_fence_list.
> + */
> +int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index);
> +
>  /**
>   * vb2_core_qbuf() - Queue a buffer from userspace
>   *
> 

Regards,

	Hans

Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

2018-01-12 Hans Verkuil <hverkuil@xs4all.nl>:

> On 01/10/18 17:07, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
> > an out_fence and send its fd to userspace on the fence_fd field as a
> > return arg for the QBUF call.
> > 
> > The fence is signaled on buffer_done(), when the job on the buffer is
> > finished.
> > 
> > v8:
> > 	- return 0 as fence_fd if OUT_FENCE flag not used (Mauro)
> > 	- fix crash when checking not using fences in vb2_buffer_done()
> > 
> > v7:
> > 	- merge patch that add the infrastructure to out-fences into
> > 	this one (Alex Courbot)
> > 	- Do not install the fd if there is no fence. (Alex Courbot)
> > 	- do not report error on requeueing, just WARN_ON_ONCE() (Hans)
> > 
> > v6
> > 	- get rid of the V4L2_EVENT_OUT_FENCE event. We always keep the
> > 	ordering in vb2 for queueing in the driver, so the event is not
> > 	necessary anymore and the out_fence_fd is sent back to userspace
> > 	on QBUF call return arg
> > 	- do not allow requeueing with out-fences, instead mark the buffer
> > 	with an error and wake up to userspace.
> > 	- send the out_fence_fd back to userspace on the fence_fd field
> > 
> > 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@collabora.com>
> > ---
> >  drivers/media/common/videobuf/videobuf2-core.c | 101 +++++++++++++++++++++++--
> >  drivers/media/common/videobuf/videobuf2-v4l2.c |  28 ++++++-
> >  include/media/videobuf2-core.h                 |  22 ++++++
> >  3 files changed, 140 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf/videobuf2-core.c b/drivers/media/common/videobuf/videobuf2-core.c
> > index 777e3a2bc746..1f30d9efb7c8 100644
> > --- a/drivers/media/common/videobuf/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf/videobuf2-core.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/freezer.h>
> >  #include <linux/kthread.h>
> > +#include <linux/sync_file.h>
> >  
> >  #include <media/videobuf2-core.h>
> >  #include <media/v4l2-mc.h>
> > @@ -357,6 +358,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 */
> > @@ -939,10 +941,22 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> >  	case VB2_BUF_STATE_QUEUED:
> >  		break;
> >  	case VB2_BUF_STATE_REQUEUEING:
> > +		/* Requeuing with explicit synchronization, spit warning */
> > +		WARN_ON_ONCE(vb->out_fence);
> > +
> >  		if (q->start_streaming_called)
> >  			__enqueue_in_driver(vb);
> > -		return;
> > +		break;
> >  	default:
> > +		if (vb->out_fence) {
> > +			if (state == VB2_BUF_STATE_ERROR)
> > +				dma_fence_set_error(vb->out_fence, -EFAULT);
> > +			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;
> > @@ -1341,6 +1355,65 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
> >  
> > +static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
> > +{
> > +	return "vb2_fence";
> > +}
> > +
> > +static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
> > +{
> > +	return "vb2_fence_timeline";
> > +}
> > +
> > +static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
> > +{
> > +	return true;
> > +}
> > +
> > +static const struct dma_fence_ops vb2_fence_ops = {
> > +	.get_driver_name = vb2_fence_get_driver_name,
> > +	.get_timeline_name = vb2_fence_get_timeline_name,
> > +	.enable_signaling = vb2_fence_enable_signaling,
> > +	.wait = dma_fence_default_wait,
> > +};
> > +
> > +int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index)
> > +{
> > +	struct vb2_buffer *vb;
> > +	u64 context;
> > +
> > +	vb = q->bufs[index];
> > +
> > +	vb->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> > +
> > +	if (call_qop(q, is_unordered, q))
> > +		context = dma_fence_context_alloc(1);
> > +	else
> > +		context = q->out_fence_context;
> > +
> > +	vb->out_fence = kzalloc(sizeof(*vb->out_fence), GFP_KERNEL);
> > +	if (!vb->out_fence)
> > +		return -ENOMEM;
> > +
> > +	dma_fence_init(vb->out_fence, &vb2_fence_ops, &q->out_fence_lock,
> > +		       context, 1);
> > +	if (!vb->out_fence) {
> > +		put_unused_fd(vb->out_fence_fd);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	vb->sync_file = sync_file_create(vb->out_fence);
> > +	if (!vb->sync_file) {
> > +		put_unused_fd(vb->out_fence_fd);
> > +		dma_fence_put(vb->out_fence);
> > +		vb->out_fence = NULL;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
> > +
> >  /*
> >   * vb2_start_streaming() - Attempt to start streaming.
> >   * @q:		videobuf2 queue
> > @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> 
> This function is now very confusing. The last argument is a fence, but it is set
> for both in and out fences, and the previous patch sets vb->in_fence to fence.
> Even though the fence passed in this case is an out fence.

It doesn't use it for both types of fence. vb2_core_qbuf() receives that
in-fence as parameter and assign it to vb->in_fence. That is all. Maybe
I should call the parameter in_fence to avoid confusion.

> 
> I think it would be much easier to understand if you add both an in_fence and
> out_fence argument to this function.
> 
> Is it possible to have both and in and out fence for the same buffer? I think
> it is and in that case you really need two fences.

Yes it is. I added infomation about this in the docs for the next series
of these patches.

> 
> >  	if (vb->in_fence) {
> >  		ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
> >  					     vb2_qbuf_fence_cb);
> > -		if (ret == -EINVAL) {
> > +		/* is the fence signaled? */
> > +		if (ret == -ENOENT) {
> > +			dma_fence_put(vb->in_fence);
> > +			vb->in_fence = NULL;
> > +		} else if (ret) {
> >  			spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> >  			goto err;
> > -		} else if (!ret) {
> > -			goto fill;
> >  		}
> > -
> > -		dma_fence_put(vb->in_fence);
> > -		vb->in_fence = NULL;
> >  	}
> >  
> > -fill:
> >  	/*
> >  	 * If already streaming and there is no fence to wait on
> >  	 * give the buffer to driver for processing.
> > @@ -1535,6 +1606,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> >  	if (pb)
> >  		call_void_bufop(q, fill_user_buffer, vb, pb);
> >  
> > +	if (vb->out_fence) {
> > +		fd_install(vb->out_fence_fd, vb->sync_file->file);
> > +		vb->sync_file = NULL;
> > +	}
> > +
> >  	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
> >  	return 0;
> >  
> > @@ -1802,6 +1878,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >  		spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> >  	}
> >  
> > +	/*
> > +	 * Renew out-fence context.
> > +	 */
> > +	if (!call_qop(q, is_unordered, q))
> > +		q->out_fence_context = dma_fence_context_alloc(1);
> 
> I don't think this is the right place. If a driver implements is_unordered, then
> the return value depends on the format. And that isn't locked in until buffers
> are allocated (i.e. reqbufs or create_bufs is called). So that's the moment that
> you can set this up.

Yes. That is a leftover from when I was thinking ordering by driver.
I'll fix it.

> 
> BTW: I noticed is_unordered() returned an int instead of a bool. I think a bool
> makes more sense.
> 
> > +
> >  	/*
> >  	 * Remove all buffers from videobuf's list...
> >  	 */
> > @@ -2134,6 +2216,9 @@ 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);
> > +	if (!call_qop(q, is_unordered, q))
> > +		q->out_fence_context = dma_fence_context_alloc(1);
> 
> Same here: at this moment you do not know if the queue is ordered or
> unordered.
> 
> > +	spin_lock_init(&q->out_fence_lock);
> >  
> >  	q->memory = VB2_MEMORY_UNKNOWN;
> >  
> > diff --git a/drivers/media/common/videobuf/videobuf2-v4l2.c b/drivers/media/common/videobuf/videobuf2-v4l2.c
> > index 0a41e3bb7733..f1291c25323f 100644
> > --- a/drivers/media/common/videobuf/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf/videobuf2-v4l2.c
> > @@ -179,12 +179,16 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> > -	    !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> > +	if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> >  		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> 
> Shouldn't this be b->fence_fd <= 0?

0 is a valid fd, so b-fence_fd < 0
> 
> > +		dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> > +		return -EINVAL;
> > +	}
> > +
> >  	return __verify_planes_array(q->bufs[b->index], b);
> >  }
> >  
> > @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
> >  	b->sequence = vbuf->sequence;
> >  	b->reserved = 0;
> >  
> > -	b->fence_fd = 0;
> > +	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> > +		b->fence_fd = vb->out_fence_fd;
> > +	} else {
> > +		b->fence_fd = 0;
> > +	}
> > +
> >  	if (vb->in_fence)
> >  		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
> >  	else
> > @@ -491,6 +500,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  	ret = __verify_planes_array(vb, b);
> >  	if (!ret)
> >  		vb2_core_querybuf(q, b->index, b);
> > +
> > +	/* Do not return the out-fence fd on querybuf */
> > +	if (vb->out_fence)
> > +		b->fence_fd = -1;
> 
> This does not hurt. We also return dmabuf fds in the same way in querybuf.
> 
> I would drop this.
>
> >  		}
> >  	}
> >  
> > +	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> > +		ret = vb2_setup_out_fence(q, b->index);
> > +		if (ret) {
> > +			dprintk(1, "failed to set up out-fence\n");
> > +			dma_fence_put(fence);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	return vb2_core_qbuf(q, b->index, b, fence);
> 
> Hmm. So if vb2_core_qbuf returns an error, we still have created the out fence?
> As mentioned above, it is very confusing that the same vb2_core_qbuf 'fence'
> argument is used for both in and out fences.

I'm confused on how you got this understanding that we are using the
fence argument for both. I wonder if I failing to see a mistake on my
side here.

It seems that one of your concerns here is how do we clean up the
out-fence if vb2_core_qbuf() fails. I thought I had this covered, I'll
add a task here to check on that.

Gustavo

Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Alexandre Courbot]

On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>  /*
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:         videobuf2 queue
> @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>         if (vb->in_fence) {
>                 ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
>                                              vb2_qbuf_fence_cb);
> -               if (ret == -EINVAL) {
> +               /* is the fence signaled? */
> +               if (ret == -ENOENT) {
> +                       dma_fence_put(vb->in_fence);
> +                       vb->in_fence = NULL;
> +               } else if (ret) {
>                         spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
>                         goto err;
> -               } else if (!ret) {
> -                       goto fill;
>                 }
> -
> -               dma_fence_put(vb->in_fence);
> -               vb->in_fence = NULL;

This chunk seems to deal with input fences, shouldn't it be part of
the previous patch instead of this one?

>
> -       if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> -           !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +       if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
>                 dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
>                 return -EINVAL;
>         }
>
> +       if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +               dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> +               return -EINVAL;
> +       }
> +

Same here?

>         return __verify_planes_array(q->bufs[b->index], b);
>  }
>
> @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>         b->sequence = vbuf->sequence;
>         b->reserved = 0;
>
> -       b->fence_fd = 0;
> +       if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +               b->fence_fd = vb->out_fence_fd;
> +       } else {
> +               b->fence_fd = 0;
> +       }

Sorry if this has already been discussed, but I don't remember the
outcome if it has.

I wonder if doing this here could not make out_fence_fd leak in
situations where we don't need/want it to. Let's take for instance a
multi-process user program. One process queues a buffer with an
OUT_FENCE and gets a valid fd in fence_fd upon return. Then the other
process performs a QUERYBUF and gets the same fence_fd - which is
invalid in its context. Would it not be preferable fill the out fence
information only when queuing buffers, since it is the only time where
we are guaranteed it will be usable by the caller?

Similarly, when a buffer is processed and user-space performs a DQBUF,
the V4L2_BUF_FLAG_OUT_FENCE will be set but fence_fd will be 0. Again,
limiting the return of out fence information to QBUF would prevent
this.

If we go that route, out_fence_fd could maybe become a local variable
of vb2_qbuf() instead of being a member of vb2_buffer, and would be
returned by vb2_setup_out_fence(). This would guarantee it does not
leak anywhere else.

Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

2018-01-15 Alexandre Courbot <acourbot@chromium.org>:

> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> >  /*
> >   * vb2_start_streaming() - Attempt to start streaming.
> >   * @q:         videobuf2 queue
> > @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> >         if (vb->in_fence) {
> >                 ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
> >                                              vb2_qbuf_fence_cb);
> > -               if (ret == -EINVAL) {
> > +               /* is the fence signaled? */
> > +               if (ret == -ENOENT) {
> > +                       dma_fence_put(vb->in_fence);
> > +                       vb->in_fence = NULL;
> > +               } else if (ret) {
> >                         spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
> >                         goto err;
> > -               } else if (!ret) {
> > -                       goto fill;
> >                 }
> > -
> > -               dma_fence_put(vb->in_fence);
> > -               vb->in_fence = NULL;
> 
> This chunk seems to deal with input fences, shouldn't it be part of
> the previous patch instead of this one?
> 
> >
> > -       if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> > -           !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> > +       if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> >                 dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
> >                 return -EINVAL;
> >         }
> >
> > +       if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> > +               dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> > +               return -EINVAL;
> > +       }
> > +
> 
> Same here?
> 
> >         return __verify_planes_array(q->bufs[b->index], b);
> >  }
> >
> > @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
> >         b->sequence = vbuf->sequence;
> >         b->reserved = 0;
> >
> > -       b->fence_fd = 0;
> > +       if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> > +               b->fence_fd = vb->out_fence_fd;
> > +       } else {
> > +               b->fence_fd = 0;
> > +       }
> 
> Sorry if this has already been discussed, but I don't remember the
> outcome if it has.
> 
> I wonder if doing this here could not make out_fence_fd leak in
> situations where we don't need/want it to. Let's take for instance a
> multi-process user program. One process queues a buffer with an
> OUT_FENCE and gets a valid fd in fence_fd upon return. Then the other
> process performs a QUERYBUF and gets the same fence_fd - which is
> invalid in its context. Would it not be preferable fill the out fence
> information only when queuing buffers, since it is the only time where
> we are guaranteed it will be usable by the caller?
> 
> Similarly, when a buffer is processed and user-space performs a DQBUF,
> the V4L2_BUF_FLAG_OUT_FENCE will be set but fence_fd will be 0. Again,
> limiting the return of out fence information to QBUF would prevent
> this.

Right. So in summary as this is something Hans commented on another
e-mail in this thread.

Your proposal is to only return the out_fence fd number on QBUF, right?
And DQBUF and QUERYBUF would only return -1 in the fence_fd field.

What I understood from Hans comment is that he is okay with sharing the
fd in such cases and v4l2 already does that for dmabuf fds.

I believe sharing is okay, as it will be either the same process or a
process we gave the device fd in the first place.

I'm not invested in any particular approach here. Thoughts?

> 
> If we go that route, out_fence_fd could maybe become a local variable
> of vb2_qbuf() instead of being a member of vb2_buffer, and would be
> returned by vb2_setup_out_fence(). This would guarantee it does not
> leak anywhere else.


Gustavo

[PATCH v7 6/6] [media] v4l: Document explicit synchronization behavior

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Add section to VIDIOC_QBUF about it

v5:
	- Remove V4L2_CAP_ORDERED
	- Add doc about V4L2_FMT_FLAG_UNORDERED

v4:
	- Document ordering behavior for in-fences
	- Document V4L2_CAP_ORDERED capability
	- Remove doc about OUT_FENCE event
	- Document immediate return of out-fence in QBUF

v3:
	- make the out_fence refer to the current buffer (Hans)
	- Note what happens when the IN_FENCE is not set (Hans)

v2:
	- mention that fences are files (Hans)
	- rework for the new API

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/vidioc-qbuf.rst     | 47 +++++++++++++++++++++++-
 Documentation/media/uapi/v4l/vidioc-querybuf.rst |  9 ++++-
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 9e448a4aa3aa..8809397fb110 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -54,7 +54,7 @@ When the buffer is intended for output (``type`` is
 or ``V4L2_BUF_TYPE_VBI_OUTPUT``) applications must also initialize the
 ``bytesused``, ``field`` and ``timestamp`` fields, see :ref:`buffer`
 for details. Applications must also set ``flags`` to 0. The
-``reserved2`` and ``reserved`` fields must be set to 0. When using the
+``reserved`` field must be set to 0. When using the
 :ref:`multi-planar API <planar-apis>`, the ``m.planes`` field must
 contain a userspace pointer to a filled-in array of struct
 :c:type:`v4l2_plane` and the ``length`` field must be set
@@ -118,6 +118,51 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
 The struct :c:type:`v4l2_buffer` structure is specified in
 :ref:`buffer`.
 
+Explicit Synchronization
+------------------------
+
+Explicit Synchronization allows us to control the synchronization of
+shared buffers from userspace by passing fences to the kernel and/or
+receiving them from it. Fences passed to the kernel are named in-fences and
+the kernel should wait on them to signal before using the buffer, i.e., queueing
+it to the driver. On the other side, the kernel can create out-fences for the
+buffers it queues to the drivers. Out-fences signal when the driver is
+finished with buffer, i.e., the buffer is ready. The fences are represented
+as a file and passed as a file descriptor to userspace.
+
+The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
+using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer flag and the `fence_fd` field. If
+an in-fence needs to be passed to the kernel, `fence_fd` should be set to the
+fence file descriptor number and the ``V4L2_BUF_FLAG_IN_FENCE`` should be set
+as well. Setting one but not the other will cause ``VIDIOC_QBUF`` to return
+with error. The fence_fd field will be ignored if the
+``V4L2_BUF_FLAG_IN_FENCE`` is not set.
+
+The videobuf2-core will guarantee that all buffers queued with in-fence will
+be queued to the drivers in the same order. Fence may signal out of order, so
+this guarantee at videobuf2 is necessary to not change ordering.
+
+If the in-fence signals with an error the videobuf2 won't queue the buffer to
+the driver, instead it will flag it with an error. And then wait for the
+previous buffer to complete before asking userspace dequeue the buffer with
+error - to make sure we deliver the buffers back in the correct order.
+
+To get an out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
+be set to ask for a fence to be attached to the buffer. The out-fence fd is
+sent to userspace as a ``VIDIOC_QBUF`` return argument on the `fence_fd` field.
+
+Note the the same `fence_fd` field is used for both sending the in-fence as
+input argument to receive the out-fence as a return argument.
+
+At streamoff the out-fences will either signal normally if the driver waits
+for the operations on the buffers to finish or signal with an error if the
+driver cancels the pending operations. Buffers with in-fences won't be queued
+to the driver if their fences signal. It will be cleaned up.
+
+The ``V4L2_FMT_FLAG_UNORDERED`` flag in ``VIDIOC_ENUM_FMT`` tells userspace
+that the current buffer queues is able to keep the ordering of buffers, i.e.,
+the dequeing of buffers will happen at the same order we queue them, with no
+reordering by the driver.
 
 Return Value
 ============
diff --git a/Documentation/media/uapi/v4l/vidioc-querybuf.rst b/Documentation/media/uapi/v4l/vidioc-querybuf.rst
index dd54747fabc9..df964c4d916b 100644
--- a/Documentation/media/uapi/v4l/vidioc-querybuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querybuf.rst
@@ -44,7 +44,7 @@ and the ``index`` field. Valid index numbers range from zero to the
 number of buffers allocated with
 :ref:`VIDIOC_REQBUFS` (struct
 :c:type:`v4l2_requestbuffers` ``count``) minus
-one. The ``reserved`` and ``reserved2`` fields must be set to 0. When
+one. The ``reserved`` field must be set to 0. When
 using the :ref:`multi-planar API <planar-apis>`, the ``m.planes``
 field must contain a userspace pointer to an array of struct
 :c:type:`v4l2_plane` and the ``length`` field has to be set
@@ -64,6 +64,13 @@ elements will be used instead and the ``length`` field of struct
 array elements. The driver may or may not set the remaining fields and
 flags, they are meaningless in this context.
 
+When using in-fences, the ``V4L2_BUF_FLAG_IN_FENCE`` will be set if the
+in-fence didn't signal at the time of the
+:ref:`VIDIOC_QUERYBUF`. The ``V4L2_BUF_FLAG_OUT_FENCE`` will be set if
+the user asked for an out-fence for the buffer, but the ``fence_fd``
+field will be set to -1. In case ``V4L2_BUF_FLAG_OUT_FENCE`` is not set
+``fence_fd`` will be set to 0 for backward compatibility.
+
 The struct :c:type:`v4l2_buffer` structure is specified in
 :ref:`buffer`.
 
-- 
2.14.3

Re: [PATCH v7 6/6] [media] v4l: Document explicit synchronization behavior

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Add section to VIDIOC_QBUF about it
> 
> v5:
> 	- Remove V4L2_CAP_ORDERED
> 	- Add doc about V4L2_FMT_FLAG_UNORDERED
> 
> v4:
> 	- Document ordering behavior for in-fences
> 	- Document V4L2_CAP_ORDERED capability
> 	- Remove doc about OUT_FENCE event
> 	- Document immediate return of out-fence in QBUF
> 
> v3:
> 	- make the out_fence refer to the current buffer (Hans)
> 	- Note what happens when the IN_FENCE is not set (Hans)
> 
> v2:
> 	- mention that fences are files (Hans)
> 	- rework for the new API
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst     | 47 +++++++++++++++++++++++-
>  Documentation/media/uapi/v4l/vidioc-querybuf.rst |  9 ++++-
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 9e448a4aa3aa..8809397fb110 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -54,7 +54,7 @@ When the buffer is intended for output (``type`` is
>  or ``V4L2_BUF_TYPE_VBI_OUTPUT``) applications must also initialize the
>  ``bytesused``, ``field`` and ``timestamp`` fields, see :ref:`buffer`
>  for details. Applications must also set ``flags`` to 0. The
> -``reserved2`` and ``reserved`` fields must be set to 0. When using the
> +``reserved`` field must be set to 0. When using the
>  :ref:`multi-planar API <planar-apis>`, the ``m.planes`` field must
>  contain a userspace pointer to a filled-in array of struct
>  :c:type:`v4l2_plane` and the ``length`` field must be set
> @@ -118,6 +118,51 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
>  The struct :c:type:`v4l2_buffer` structure is specified in
>  :ref:`buffer`.
>  
> +Explicit Synchronization
> +------------------------
> +
> +Explicit Synchronization allows us to control the synchronization of
> +shared buffers from userspace by passing fences to the kernel and/or
> +receiving them from it. Fences passed to the kernel are named in-fences and
> +the kernel should wait on them to signal before using the buffer, i.e., queueing
> +it to the driver.

I would drop ", i.e., queueing it to the driver"

On the other side, the kernel can create out-fences for the
> +buffers it queues to the drivers. Out-fences signal when the driver is
> +finished with buffer, i.e., the buffer is ready. The fences are represented
> +as a file and passed as a file descriptor to userspace.
> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer flag and the `fence_fd` field. If
> +an in-fence needs to be passed to the kernel, `fence_fd` should be set to the
> +fence file descriptor number and the ``V4L2_BUF_FLAG_IN_FENCE`` should be set
> +as well. Setting one but not the other will cause ``VIDIOC_QBUF`` to return
> +with error. The fence_fd field will be ignored if the

with -> with an

> +``V4L2_BUF_FLAG_IN_FENCE`` is not set.
> +
> +The videobuf2-core will guarantee that all buffers queued with in-fence will

with -> with an

> +be queued to the drivers in the same order. Fence may signal out of order, so

Fence -> Fences

> +this guarantee at videobuf2 is necessary to not change ordering.

Add some text here to make it clear that waiting for a buffer with an in-fence
will also block all buffers queued after that buffer.

> +
> +If the in-fence signals with an error the videobuf2 won't queue the buffer to
> +the driver, instead it will flag it with an error. And then wait for the
> +previous buffer to complete before asking userspace dequeue the buffer with
> +error - to make sure we deliver the buffers back in the correct order.

I think there is no need to describe the internal implementation. It is enough
to say that the buffer is marked with FLAG_ERROR if the in-fence signals an
error.

> +
> +To get an out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> +be set to ask for a fence to be attached to the buffer. The out-fence fd is
> +sent to userspace as a ``VIDIOC_QBUF`` return argument on the `fence_fd` field.
> +
> +Note the the same `fence_fd` field is used for both sending the in-fence as
> +input argument to receive the out-fence as a return argument.
> +
> +At streamoff the out-fences will either signal normally if the driver waits
> +for the operations on the buffers to finish or signal with an error if the
> +driver cancels the pending operations. Buffers with in-fences won't be queued
> +to the driver if their fences signal. It will be cleaned up.

It -> They

It should be clarified here if you can or cannot use both in and out fences for
the same buffer.

> +
> +The ``V4L2_FMT_FLAG_UNORDERED`` flag in ``VIDIOC_ENUM_FMT`` tells userspace
> +that the current buffer queues is able to keep the ordering of buffers, i.e.,
> +the dequeing of buffers will happen at the same order we queue them, with no
> +reordering by the driver.

You described what it means when this flags isn't set :-)

Anyway, I'd rephrase this:

... tells userspace that when using this format the order in which buffers are
dequeued can be different from the order in which they were queued.

You should add some additional text to that explaining why this is relevant
for fences.

>  
>  Return Value
>  ============
> diff --git a/Documentation/media/uapi/v4l/vidioc-querybuf.rst b/Documentation/media/uapi/v4l/vidioc-querybuf.rst
> index dd54747fabc9..df964c4d916b 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querybuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querybuf.rst
> @@ -44,7 +44,7 @@ and the ``index`` field. Valid index numbers range from zero to the
>  number of buffers allocated with
>  :ref:`VIDIOC_REQBUFS` (struct
>  :c:type:`v4l2_requestbuffers` ``count``) minus
> -one. The ``reserved`` and ``reserved2`` fields must be set to 0. When
> +one. The ``reserved`` field must be set to 0. When
>  using the :ref:`multi-planar API <planar-apis>`, the ``m.planes``
>  field must contain a userspace pointer to an array of struct
>  :c:type:`v4l2_plane` and the ``length`` field has to be set
> @@ -64,6 +64,13 @@ elements will be used instead and the ``length`` field of struct
>  array elements. The driver may or may not set the remaining fields and
>  flags, they are meaningless in this context.
>  
> +When using in-fences, the ``V4L2_BUF_FLAG_IN_FENCE`` will be set if the
> +in-fence didn't signal at the time of the
> +:ref:`VIDIOC_QUERYBUF`. The ``V4L2_BUF_FLAG_OUT_FENCE`` will be set if
> +the user asked for an out-fence for the buffer, but the ``fence_fd``
> +field will be set to -1. In case ``V4L2_BUF_FLAG_OUT_FENCE`` is not set

I don't think there is any reason not to return the out fence fd here.

> +``fence_fd`` will be set to 0 for backward compatibility.
> +
>  The struct :c:type:`v4l2_buffer` structure is specified in
>  :ref:`buffer`.
>  
> 

Regards,

	Hans

Re: [PATCH v7 0/6] V4L2 Explicit Synchronization

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

Le mercredi 10 janvier 2018 à 14:07 -0200, Gustavo Padovan a écrit :
> v7 bring a fix for a crash when not using fences and a uAPI fix.
> I've done a bit more of testing on it and also measured some
> performance. On a intel laptop a DRM<->V4L2 pipeline with fences is
> runnning twice as faster than the same pipeline with no fences.

What does it mean twice faster here ?

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v7 0/6] V4L2 Explicit Synchronization

[Gustavo Padovan]

2018-01-10 Nicolas Dufresne <nicolas@ndufresne.ca>:

> Le mercredi 10 janvier 2018 à 14:07 -0200, Gustavo Padovan a écrit :
> > v7 bring a fix for a crash when not using fences and a uAPI fix.
> > I've done a bit more of testing on it and also measured some
> > performance. On a intel laptop a DRM<->V4L2 pipeline with fences is
> > runnning twice as faster than the same pipeline with no fences.
> 
> What does it mean twice faster here ?

That capture then display on the screen for a given number of frames was
completing in about half of the time when using fences and passing then
to the other driver right away when they are received.

Gustavo