2018-01-15 Alexandre Courbot <acourbot@xxxxxxxxxxxx>:
> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@xxxxxxxxxxx> 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
