Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754634AbeAOHNG (ORCPT + 1 other); Mon, 15 Jan 2018 02:13:06 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:46724 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754516AbeAOHM7 (ORCPT ); Mon, 15 Jan 2018 02:12:59 -0500 X-Google-Smtp-Source: ACJfBoueXtZjbUEBlUq+AYcx3jvzVuAgMME9GGEm95f6h8IO8A8Ejaaw461ZH5z3g+pXN8kmnBiiBw== MIME-Version: 1.0 In-Reply-To: <20180110160732.7722-6-gustavo@padovan.org> References: <20180110160732.7722-1-gustavo@padovan.org> <20180110160732.7722-6-gustavo@padovan.org> From: Alexandre Courbot Date: Mon, 15 Jan 2018 16:12:37 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF To: Gustavo Padovan Cc: Linux Media Mailing List , Hans Verkuil , Mauro Carvalho Chehab , Shuah Khan , Pawel Osciak , Sakari Ailus , Brian Starkey , Thierry Escande , linux-kernel@vger.kernel.org, Gustavo Padovan Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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, &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.