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

[Gustavo Padovan]

2018-01-15 Alexandre Courbot :

> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  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, >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(>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

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

[Gustavo Padovan]

2018-01-15 Alexandre Courbot :

> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  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, >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(>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

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

[Gustavo Padovan]

2018-01-12 Hans Verkuil :

> On 01/10/18 17:07, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > 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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -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(>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 = 

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

[Gustavo Padovan]

2018-01-12 Hans Verkuil :

> On 01/10/18 17:07, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > 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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -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(>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 

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  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, >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(>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

[Alexandre Courbot]

On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  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, >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(>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

[Hans Verkuil]

On 01/10/18 17:07, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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(>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];
> +

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 
> 
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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(>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);
> +

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

[Gustavo Padovan]

From: Gustavo Padovan 

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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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(>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 

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

[Gustavo Padovan]

From: Gustavo Padovan 

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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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(>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 =